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

[Pavel Pisa]

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.

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.

> > +    /* 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.

Best wishes,

Pavel