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, p...@cmp.felk.cvut.cz wrote: >>> From: Pavel Pisa <p...@cmp.felk.cvut.cz> > >> 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.