Hi Joe, On 14/12/2015 23:06, "Joe Stringer" <j...@ovn.org> wrote:
>On 2 December 2015 at 11:46, Daniele Di Proietto <diproiet...@vmware.com> >wrote: >> The goal of this series is to introduce two dpctl command to interact >> with the Linux kernel connection tracker. The same infrastructure >> will be used by the userspace connection tracker. >> >> First, it defines some structures and some formatting routines >>(ct-dpif). >> >> Then, it adds some code to transform the netlink conntrack format >> into the OVS specific structure (netlink-conntrack) >> >> Some function pointers are added into dpif-provider to implement >>conntrack >> flushing and dumping. >> >> dpif-netlink implements the new dpif-provider interface using >> netlink-conntrack. >> >> New functions are added in ct-dpif to implement dumping and flushing. >> >> The dpctl commands are finally added to the dpctl module, and they're >> used by the system testsuite. >> >> Finally, a test module (test-netlink-conntrack) is added to allow the >>use >> of the netlink-conntrack API without a datapath. > >Apologies for the long round-trip time. Thanks a lot for this work! >Overall it looks pretty good to me. The last patch doesn't apply any >more, but it's pretty trivial and looks fine. Yes, it was a simple rebase. > >I've assembled some various small pieces of (very minor) feedback in a >branch here: >https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_joestringe >r_openvswitch_tree_review_appctl-5Fv3&d=BQIBaQ&c=Sqcl0Ez6M0X8aeM67LKIiDJAX >VeAw-YihVMNtXt-uEs&r=SmB5nZacmXNq0gKCC1s_Cw5yUNjxgD4v5kJqZ2uWLlE&m=N5l92mo >s8xNOluz9CfWtdwHmo4cepsxhlXrIsHNIYG0&s=wioCt_uCJ50RQDBYD3xdw2HnJ4oGMwNBGt8 >IYwkst-o&e= Thanks a lot for all this feedback. I've incorporated everything in the appropriate commits. >When I run ovs-dpctl --help, I don't see the new commands listed. I >briefly poked around, but didn't see where it is these are picked up >from. Could you take a look? You're right, I forgot to update usage() in utilities/ovs-dpctl.c. >Do you think that these changes will build fine on Windows and *BSD? >You could use Appveyor to at least check the build on Windows. I guess >you may already have some kind of BSD setup. Maybe it doesn't make a >difference. I was testing on BSD, but not on windows. I found one problem with appveyor: dpif-netlink was assuming that netlink-conntrack was always available, but it's compiled only on Linux. I had to introduce a couple of #ifdef __linux__ (as discussed offline) to fix the build on windows. >I'd like to see this also backported to branch-2.5, given that it >would be an invaluable debugging tool for the new conntrack >functionality. Absolutely, I'll push that to master and branch-2.5 in a minute. >Acked-by: Joe Stringer <j...@ovn.org> Thanks for the detailed review! _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
