Ben Pfaff <b...@ovn.org> writes: > On Mon, Nov 27, 2017 at 06:11:42PM -0500, Aaron Conole wrote: >> Darrell Ball <dlu...@gmail.com> writes: >> >> > Some refactoring of alg support is done. >> > Also algs are disabled by default unless an alg specifier >> > is supplied; this allows for enhanced security and matches >> > later kernels. >> > Another change to allow for non-standard alg conntrol >> > port specification. >> > >> > >> > Darrell Ball (3): >> > conntrack: Refactor algs. >> > conntrack: Allow specified alg port numbers. >> > conntrack: Disable algs by default. >> > >> > lib/conntrack.c | 168 >> > ++++++++++++++++++++++++++++++++++--------------- >> > lib/conntrack.h | 8 +-- >> > lib/dpif-netdev.c | 4 +- >> > tests/test-conntrack.c | 6 +- >> > 4 files changed, 127 insertions(+), 59 deletions(-) >> >> For the series - >> >> Acked-by: Aaron Conole <acon...@redhat.com> > > Hi Aaron. Thank you for reviewing--it is time consuming work and does > not get enough credit. Can you describe your review process a little?
Sure. For this, the original work was at: https://mail.openvswitch.org/pipermail/ovs-dev/2017-November/341035.html https://mail.openvswitch.org/pipermail/ovs-dev/2017-November/341037.html Darrel and I had some discussions there, and I asked for this to be split out. Additionally, I had noticed while reviewing that it didn't seem non-standard ports were covered for helpers, so asked that such a change be folded in (and Darrell did). For the code: I applied the code, and didn't see anything obviously wrong at the end (although I did the whole series at once, in the v2 rather than doing patch-at-a-time). I could still read through the conntrack code, and presumably I am able to follow what is happening. Additionally, once applied, I ran `./utilities/checkpatch.py -3` to ensure that nothing slipped by. For the functionality: I ran the l7 tests via `make check-system-userspace`, and saw that the l7 ftp tests were passing. This will require installation of pyftpdlib and tftpy to execute properly (otherwise they'll be skipped). Those tests are explicitly using 'alg=', so should still pass (and they do). I didn't check anything with manual sets of flows. I will do that now, though since outlining this makes me realize I never checked the 'negative' case (meaning assume that no alg= is given, ensure that the helper really isn't assigned). I can report back on that tomorrow, if it makes sense. > I'm trying to figure out whether I need to do a detailed review myself > or just take your acks. Well, I have never heard anyone say fewer eyes was better. > Thanks, > > Ben. _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
