Re: [Qemu-devel] [PATCH V4 0/7] CAN bus support for QEMU (SJA1000 PCI so far)

["Philippe Mathieu-Daudé" <f4bug@amsat.org>]

Hi Pavel,

On 01/24/2018 05:22 PM, Pavel Pisa wrote:
> Hello everybody,
> 
> On Tuesday 23 of January 2018 22:42:31 Pavel Pisa wrote:
>> When Linux specific object file is linked in then some local
>> function needs to be called before QOM instances population.
>> I know how to do that GCC specific/non-portable way
>>
>> static void __attribute__((constructor)) can_socketcan_setup_variant(void)
>> {
>>
>> }
>>
>> but I expect that something like
>>
>> module_init()
>>
>> in can_socketcan.c should be used.
> 
> 
> I have experimented with code changes to get rid of stub for
> non Linux systems. type_init() is used because it is more
> portable than constructor attribute.
> 
> I have seen that a few other type_init-s do more
> than simple sequence of type_register_static().
> Is it acceptable to use type_init for registration
> to CAN core by function call for now? Conversion simplifies
> makefiles and unnecessary stub file is removed.
> 
> But I would use attribute if that solution is preferred because
> it is allways present on Linux where SocketCAN is used anyway
> and it is used in other Qemu subsystems as well.

using stubs/monitor.c as a template for stubs/can_host_variant.c doesn't
work?

> 
> ----------------------------------------------------------------
> Solution with attribute
> 
> #ifdef TOOLCHAIN_SUPPORTS_ATTRIBUTE_CONSTRUCTOR
> static void __attribute__((constructor))
> can_bus_socketcan_setup(void)
> {
>     can_bus_set_connect_to_host(can_bus_connect_to_host_socketcan);
> }
> #endif
> 
> ----------------------------------------------------------------
> Solution with type_init
> branch https://gitlab.fel.cvut.cz/canbus/qemu-canbus/tree/can-pci-socketcan-init
> 
> diff --git a/hw/can/can_socketcan.c b/hw/can/can_socketcan.c
> index 42099fb696..fb41853c2b 100644
> --- a/hw/can/can_socketcan.c
> +++ b/hw/can/can_socketcan.c
> @@ -309,5 +309,14 @@ static int can_bus_connect_to_host_socketcan(CanBusState *bus,
>      return 0;
>  }
>  
> -int (*can_bus_connect_to_host_variant)(CanBusState *bus, const char *name) =
> -        can_bus_connect_to_host_socketcan;
> +static void can_bus_socketcan_type_init(void)
> +{
> +    /*
> +     * There should be object registration when CanBusSocketcanConnectState
> +     * is converted into QOM object. Use for registration of host
> +     * can bus access for now.
> +     */
> +    can_bus_set_connect_to_host(can_bus_connect_to_host_socketcan);
> +}
> +
> +type_init(can_bus_socketcan_type_init);
> 
> 
> diff --git a/hw/can/Makefile.objs b/hw/can/Makefile.objs
> index 1ce9950de0..14c2369718 100644
> --- a/hw/can/Makefile.objs
> +++ b/hw/can/Makefile.objs
> @@ -2,11 +2,7 @@
>  
>  ifeq ($(CONFIG_CAN_BUS),y)
>  common-obj-y += can_core.o
> -ifeq ($(CONFIG_LINUX),y)
> -common-obj-y += can_socketcan.o
> -else
> -common-obj-y += can_host_stub.o
> -endif
> +common-obj-$(CONFIG_LINUX) += can_socketcan.o
>  common-obj-$(CONFIG_CAN_SJA1000) += can_sja1000.o
>  common-obj-$(CONFIG_CAN_PCI) += can_kvaser_pci.o
>  common-obj-$(CONFIG_CAN_PCI) += can_pcm3680_pci.o
> diff --git a/hw/can/can_core.c b/hw/can/can_core.c
> index 41c458c792..c14807b188 100644
> --- a/hw/can/can_core.c
> +++ b/hw/can/can_core.c
> @@ -34,6 +34,8 @@
>  static QTAILQ_HEAD(, CanBusState) can_buses =
>      QTAILQ_HEAD_INITIALIZER(can_buses);
>  
> +static can_bus_connect_to_host_t can_bus_connect_to_host_fnc;
> +
>  CanBusState *can_bus_find_by_name(const char *name, bool create_missing)
>  {
>      CanBusState *bus;
> @@ -127,10 +129,15 @@ int can_bus_client_set_filters(CanBusClientState *client,
>  
>  int can_bus_connect_to_host_device(CanBusState *bus, const char *name)
>  {
> -    if (can_bus_connect_to_host_variant == NULL) {
> +    if (can_bus_connect_to_host_fnc == NULL) {
>          error_report("CAN bus connect to host device is not "
>                       "supported on this system");
>          exit(1);
>      }
> -    return can_bus_connect_to_host_variant(bus, name);
> +    return can_bus_connect_to_host_fnc(bus, name);
> +}
> +
> +void can_bus_set_connect_to_host(can_bus_connect_to_host_t connect_fnc)
> +{
> +    can_bus_connect_to_host_fnc = connect_fnc;
>  }
> diff --git a/hw/can/can_host_stub.c b/hw/can/can_host_stub.c
> deleted file mode 100644
> index 748d25f995..0000000000
> --- a/hw/can/can_host_stub.c
> +++ /dev/null
> @@ -1,36 +0,0 @@
> ....
> ....
> ....
> -
> -
> -int (*can_bus_connect_to_host_variant)(CanBusState *bus, const char *name) =
> -        NULL;
> 
> diff --git a/include/can/can_emu.h b/include/can/can_emu.h
> index 85237ee3c9..7f0705e49f 100644
> --- a/include/can/can_emu.h
> +++ b/include/can/can_emu.h
> @@ -107,8 +107,9 @@ struct CanBusState {
>      QTAILQ_ENTRY(CanBusState) next;
>  };
>  
> -extern int (*can_bus_connect_to_host_variant)(CanBusState *bus,
> -                                              const char *name);
> +typedef int (*can_bus_connect_to_host_t)(CanBusState *bus, const char *name);
> +
> +void can_bus_set_connect_to_host(can_bus_connect_to_host_t connect_fnc);
>  
>  int can_bus_filter_match(struct qemu_can_filter *filter, qemu_canid_t can_id);
> ----------------------------------------------------------------
> 
> Best wishes,
> 
> Pavel
>