On 7/16/17, 11:25 PM, "Ilya Maximets" <[email protected]> wrote:
> ALG infra is added with support for FTP and TFTP.
> Both V4 and V6 are supported. Also, NAT is supported.
>
> Three passive ftp system tests are added to complete testing
> coverage of ftp for the userspace datapath, as the existing
> coverage of passive ftp was limited to one part of one test
> for V4 only.
> Another system test is added covering tftp with NAT which
> was not previously exercised.
>
> v5->v6: Re-instated include inadvertently removed.
> Improve 2 of the new system tests in terms of
> potential races.
>
> v4->v5: Address Ben's code review comments.
> First 3 patches were committed.
>
> v3->v4: Fix tftp with NAT.
> Add a system test covering tftp with NAT.
>
> v2->v3: Fix v4 passive ftp with NAT.
> Fix V6 passive ftp; parse check was broken.
> Add 3 tests covering v4/v6 passive ftp to
> complete ALG coverage in the system tests.
>
> Code review caught a memory leak of the alg
> string such as "ftp" that could occurs during
> nat tuple exhaustion. This is a pathological
> user error case whose fix was tested by
> instrumentated simulation.
> Code review also pointed out that a connection
> context copy was unclear; this was moved to the
> caller where all allocation and error cleanup is
> done.
> Added several lock annotations that were missing
> from the original conntrack code and nat code.
> Other review comments were fixed.
>
> v1->v2:
> Mostly the addition of V6 FTP and TFTP support.
>
> Removed define for unused FTP server port 20.
>
> Add overflow checks for port numbers.
>
> Instead of bypassing FTP bounce exploit with
> auto-correct, explicitly flag packet as invalid.
>
> Seq number overflow and underflow checks added.
>
> Darrell Ball (5):
> Userspace Datapath: Add ALG infra and FTP.
> Userspace Datapath: Add TFTP support.
> System tests: Enable ALGs for userspace.
> System tests: Add 4 new ftp and tftp tests.
> NEWS: Announce userspace datapath ALG support.
Hi Darrell,
This is not a full review. I just wanted to ask you to rename the patches
and stop using 'Userspace Datapath' or 'System tests' as a module name for
changes localized to only one particular module.
I commented on this already.
The ‘area’ field is fiexible; ‘Area’ does not have to be a file name, although
it can be.
This is confirmed by the submitting-patches.rst document and related discussion
with
those who developed the guidelines.
I am using Userspace Datapath to be consistent with the Linux kernel, which is
using ‘Datapath’ for all datapath changes, including conntrack and also
Windows, which is using
‘Datapath-Windows’, for all datapath changes, including Conntrack.
On the other hand, I consider conntrack to be not the best choice, although
others are free to use it.
The reason is ‘conntrack’ is ambiguous; there is a kernel file with the exact
same name,
conntrack.c and a windows file named Conntrack.c.
I already raised this issue for 'dpdk' prefix previously here:
https://urldefense.proofpoint.com/v2/url?u=https-3A__mail.openvswitch.org_pipermail_ovs-2Ddev_2017-2DJuly_335139.html&d=DwICaQ&c=uilaK90D4TOVoH58JNXRgQ&r=BVhFA09CGX7JQ5Ih-uZnsw&m=k-1rcsJmn7U9n0rvB0yCMWjS3FiNfG52fiCwj3VQF3E&s=p2L3sQwNXBllmKqJ44iynpfpAsQEGsexTJ_N6-GhuE4&e=
I mentioned to Sugesh that module naming is flexible and he is free to choose a
reasonable name for individual patches per submitting-patches.rst
So, about current patch-set:
The first two patches are localized to lib/conntrack* files and should
have 'conntrack' prefix as a module name. 3rd and 4th patches should have
'system-userspace-macros' and 'system-traffic' module names respectively.
Same answer as above.
The ‘Area’ name in the past for these test files has used ‘Tests’,
‘System-Tests’,
‘System-Macros’ and ‘System-Traffic’ and some others.
I prefer ‘System-Tests’ as ‘Tests’ is too general IMO and ‘System-Tests’
correctly identifies an
‘Area of the code’ with a common infra and theme, spread out over a few files.
However, I cannot enforce the ‘System-Tests’ area name.
Such names will be more accurate and conformed to existing commits and
contribution guide.
Best regards, Ilya Maximets.
>
> NEWS | 1 +
> include/sparse/netinet/in.h | 1 +
> lib/conntrack-private.h | 35 +-
> lib/conntrack.c | 1088
+++++++++++++++++++++++++++++++++++---
> lib/conntrack.h | 10 +-
> tests/system-traffic.at | 242 +++++++++
> tests/system-userspace-macros.at | 7 +-
> 7 files changed, 1306 insertions(+), 78 deletions(-)
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
