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