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

[Paolo Bonzini <pbonzini@redhat.com>]

On 23/01/2018 22:42, Pavel Pisa wrote:
> Do you think QOM based? I would like it to be implemented
> that way but I need some assistance where to look how this
> object kind should be implemented and from which base object
> inherit from. But I prefer to left that for later work.
> 
> I would definitely like to use some mechanism which allows
> to get rid of externally visible pointer and need to assign
> it in the stub. It has been my initial idea and your review
> sumbled across this hack as well. But I need suggestion what
> is the preferred way for QEMU.

The best way would be a QOM object.  That is, you would do

   -object can-bus,id=canbus0
   -object can-host-socketcan,id=can0-host,canbus=canbus0,if=can0
   -device kvaser_pci,canbus=canbus0

In the current version, it's not clear to me:

* what it means if multiple controllers have the same canbus

* what it means if multiple controllers with the same canbus have
different host interfaces

Separating the QOM objects is a bit more work, but it makes the
semantics clearer.  The classes would be:

- can-bus and an abstract class can-host, which would inherit directly
from TYPE_OBJECT and implement TYPE_USER_CREATABLE

- can-host-socketcan, which would inherit from can-host (and take the
TYPE_USER_CREATABLE implementation from there)

while CanBusClientState and CanBusClientInfo need not be QOMified.

can-host's class structure would define a function pointer corresponding
to what you have now for the function pointer, more or less---except
that allocation is handled by QOM and the method only has to do the
connection.  You would have something like this:

static void can_host_disconnect(CANHost *ch, Error **errp)
{
    CANHostClass *chc = CAN_HOST_GET_CLASS(ch);

    chc->disconnect(ch);
}

static void can_host_connect(CANHost *ch, Error **errp)
{
    CANHostClass *chc = CAN_HOST_GET_CLASS(ch);
    Error *local_err = NULL;

    chc->connect(ch, &local_err);
    if (local_err) {
        error_propagate(errp, local_err);
        return;
    }

    can_bus_insert_client(ch->bus, &ch->bus_client, local_err);
    if (local_err) {
        can_host_disconnect(ch);
        error_propagate(errp, local_err);
        return;
    }
}

and TYPE_USER_CREATABLE's "complete" method would simply invoke
can_host_connect.  For can-host-socketcan, chc->connect would be
assigned something like this:

static void can_host_socketcan_connect(CANHost *ch, Error **errp)
{
    CANHostSocketCAN *chs = CAN_HOST_SOCKETCAN(ch);

    s = socket(PF_CAN, SOCK_RAW, CAN_RAW);
    if (s < 0) {
        error_setg_errno(errp, errno "CAN_RAW socket create failed");
        return;
    }

    addr.can_family = AF_CAN;
    memset(&ifr.ifr_name, 0, sizeof(ifr.ifr_name));
    strcpy(ifr.ifr_name, chs->host_dev_name);
    if (ioctl(s, SIOCGIFINDEX, &ifr) < 0) {
        error_setg_errno(errp, "host interface %s not available",
                         chs->host_dev_name);
        goto fail;
    }
    addr.can_ifindex = ifr.ifr_ifindex;
    ....
}

In particular, note the difference in error reporting with
error_report/exit vs. error_setg/error_propagate.  Any call to "exit" is
probably grounds for instant rejection of your patch. :)  This also
means that you have to check for leaks in the failure paths, such as
forgetting to close the PF_CAN socket.

Thanks,

Paolo

> 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.
> 
> Problem is that there is not module_init
> type for plain function in include/qemu/module.h
> 
>     MODULE_INIT_BLOCK,
>     MODULE_INIT_OPTS,
>     MODULE_INIT_QOM,
>     MODULE_INIT_TRACE,
>     MODULE_INIT_MAX
> 
> I expect that QOM object would solve that in future
> but I would be happy to left it simple for now.
> 
> What is preferred solution there?
> 
>> I'd still avoid using directly the socket() syscall and use the QEMU
>> socket API instead (also suggested by Daniel).
> 
> I have already switched to qemu_socket(), implementation
> looks fine and I have tested that it works.
> I have tested functionality and updated can-pci branch.
> 
>> I have been thinking a bit about how to test some frame operations
>> (rather than the PCI devices) and the Linux vcan driver might be a good
>> option (Virtual Local CAN Interface). This is also useful to test this
>> series without having CAN hardware. How to use vcan might be worth his
>> own paragraph in docs/can.txt.
>>
>> Do you think some of your tests can be added in the QEMU test suite
>> (qtests)?
> 
> I have added some more infomation into docs file
> 
> + The CAN interface of the host system has to be configured for proper
> + bitrate and set up. Configuration is not propagated from emulated
> + devices through bus to the physical host device. Example configuration
> + for 1 Mbit/s
> +
> +   ip link set can0 type can bitrate 1000000
> +   ip link set can0 up
> +
> + Virtual (host local only) can interface can be used on the host
> + side instead of physical interface
> +
> +   ip link add dev can0 type vcan
> +
> + The CAN interface on the host side can be used to analyze CAN
> + traffic with "candump" command which is included in "can-utils".
> +
> +   candump can0
> 
> As for the automatic testing, iproute2 tools are required
> on host and guest side (considering use of Linux)
> and kernel with CAN drivers support.
> Root access is required on the host side to setup CAN
> interface. Some simple tool is required. It can be based
> on can-utils code or our older canping code for example.
> 
> Best wishes,
> 
> Pavel
>