On Fri, Sep 28, 2012 at 12:49:16PM +0200, Giuseppe Lettieri wrote: > we have finally found the time to finish work on this patch. We have > addressed all the issues that were mentioned in the previous mails, > plus some more that showed up later. We now pass all unit tests on > both Linux and FreeBSD and we can more confidently offer the patch > to your attention, for possible inclusion. As usual, any comment > would be much appreciated.
This is looking good. I read through some of the code and I have a few comments. I see a number of XXX comments still left. Some of them, I guess, can just be removed, but others I wonder about, and it would be nice to have your opinion. dp_netdev_notifier_init() and netdev_dummy_listen() both call pipe() then set_nonblocking() on each fd. I see the same pattern in a few other places in the tree. Perhaps we should have a helper in socket-util.c to eliminate some redundancy. I see "sizeof(fds) / sizeof(fds[0])" in a few places. We have ARRAY_SIZE in util.h for that. In dp_netdev_notifier_ack(), dp_netdev_notifier_ack1(), and dp_netdev_notifier_notify(), I think that the system calls should retry on EINTR. That would be cautious practice, anyhow. I think that there are now synchronization issues in vlog. The vlog_write_file() implementation assumes a single-threaded model (worker_request() isn't thread-safe). We could introduce per-thread state so that only the main thread of a process uses worker_request() and other threads call write() directly from vlog_write_file() (since write() should be thread-safe). In dpif_netdev_exit_hook(), I'd be inclined to just use an ack and skip the pthread_cancel(). Thread cancellation always seems like a big hammer that is best avoided when one can. (Another approach would be to close the write side of the notification pipe. Then the read side would get EOF after draining any remaining acks, an unmistakable sign.) Does that give you enough to think about for a while? I'll read the next version of the patch in more detail. Thanks, Ben. _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
