Re: [Qemu-devel] [PATCH V4 2/7] CAN bus support to connect bust to Linux host SocketCAN interface.

[Philippe Mathieu-Daudé]

On 01/15/2018 06:29 PM, Pavel Pisa wrote:
> Hello Philippe,
> 
> thanks for review.
> 
> I have updated patch series in can-pci branch in
> 
>   https://gitlab.fel.cvut.cz/canbus/qemu-canbus
> 
> I would wait some day if there is some remark
> from other developers and socket ones especially.

I'll have a look.

> 
> On Monday 15 of January 2018 03:55:00 Philippe Mathieu-Daudé wrote:
>> Hi Pavel,
>>
>> I'm CC'ing the QEMU Sockets maintainer to ask them a quick review of the
>> socket part.
>>
>> On 01/14/2018 05:14 PM, address@hidden wrote:
>>> From: Pavel Pisa <address@hidden>
> 
>> Please move those checks out of the function, to call them once at build
>> time and not at runtime.
>>
>> /* Check that QEMU and Linux kernel flags encoding matches */
>> QEMU_BUILD_BUG_ON(QEMU_CAN_EFF_FLAG == CAN_EFF_FLAG);
>> QEMU_BUILD_BUG_ON(QEMU_CAN_RTR_FLAG == CAN_RTR_FLAG);
>> QEMU_BUILD_BUG_ON(QEMU_CAN_ERR_FLAG == CAN_ERR_FLAG);
>> QEMU_BUILD_BUG_ON(QEMU_CAN_INV_FILTER == CAN_INV_FILTER);
>> QEMU_BUILD_BUG_ON(offsetof(qemu_can_frame, data)
>>                   == offsetof(struct can_frame, data));
> 
> moved
> 
>>> +
>>> +    qemu_log_lock();
>>> +    qemu_log("[cansocketcan]: %03X [%01d] %s %s",
>>> +             msg->can_id & QEMU_CAN_EFF_MASK,
>>> +             msg->can_dlc,
>>> +             msg->can_id & QEMU_CAN_EFF_FLAG ? "EFF" : "SFF",
>>> +             msg->can_id & QEMU_CAN_RTR_FLAG ? "RTR" : "DAT");
>>> +
>>> +    for (i = 0; i < msg->can_dlc; i++) {
>>> +        qemu_log(" %02X", msg->data[i]);
>>> +    }
>>> +    qemu_log("\n");
>>
>> I'd rather use tracepoints, but since this is restricted by DEBUG_CAN
>> this doesn't bother the user console, so ok.
>>
>>> +    qemu_log_flush();
>>> +    qemu_log_unlock();
>>> +}
> 
> Trace events seems as nice feature. But simple text printf
> like output has been enough till now for CAN testing.
> There is no debugging output in production build.
> So I would add tracing infrastructure later if needed.

They are as useful as console printf, but less invasive and more
powerful: you can use a much precise timing, select which set of events
to display without having to recompile.
The default backend behaves as console printf.

You should try them :)

>>> +    /* open socket */
>>> +    s = socket(PF_CAN, SOCK_RAW, CAN_RAW);
>>
>> I never used it, but I think QEMU uses his socket API: "qemu/sockets.h"
> 
> The SocketCAN host connection code is Linux specific,
> but I can switch to qemu_socket() if it is preferred.
> But address family has to be from Linux header file anyway.

qemu_socket() sockets are heavily tested and already solve many things,
like async I/O and error handling.

Regards,

Phil.