[ovs-dev] [patch v2 0/3] conntrack: Alg improvements.
Aaron Conole
aconole at redhat.com
Wed Nov 29 20:33:12 UTC 2017
Ben Pfaff <blp at ovn.org> writes:
> On Mon, Nov 27, 2017 at 06:11:42PM -0500, Aaron Conole wrote:
>> Darrell Ball <dlu998 at 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 <aconole at 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.
More information about the dev
mailing list