On 7/16/17, 11:25 PM, "Ilya Maximets" <i.maxim...@samsung.com> 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
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Reply via email to