[ovs-dev] [PATCH 5/5] lib: Add support for tftp ct helper.

[Joe Stringer]

On 21 December 2016 at 17:06, Daniele Di Proietto <diproiettod at ovn.org> wrote:
> 2016-12-20 13:28 GMT-08:00 Joe Stringer <joe at ovn.org>:
>> The kernel datapath provides support for TFTP helpers, so add support
>> for this ALG to the commandline and OpenFlow encoding/decoding.
>>
>> Signed-off-by: Joe Stringer <joe at ovn.org>
>
> Do you want to add a test to tests/ofp-actions.at to parse the new alg?
>
> Do you want to add a test to tests/odp.at to parse the new alg? Maybe
> it's not important because for odp flows it's just a string.
>
> I think we should add a check in ofpact_check__() to enforce that
> alg=tftp is only used on udp flows, like commit
> c9f7de8c2b3f("ofp-actions: Check that 'alg=ftp' matches on TCP.")
>
> I have a few more comments inline, otherwise:
>
> Acked-by: Daniele Di Proietto <diproiettod at vmware.com>

Thanks for the feedback, I agree. The first four patches had minimal
feedback so I applied it and pushed them to master.

I'll send a v2 for this patch to address your feedback.

>> diff --git a/include/windows/netinet/in.h b/include/windows/netinet/in.h
>> index e4169994b14f..bae9f8ceecc5 100644
>> --- a/include/windows/netinet/in.h
>> +++ b/include/windows/netinet/in.h
>> @@ -19,5 +19,6 @@
>>
>>  #define IPPROTO_GRE 47
>>  #define IPPORT_FTP 21
>> +#define IPPORT_TFTP 69
>
> This works for sparse and for windows but fails for FreeBSD.
>
> IPPORT_FTP is also defined in include/openvswitch/ofp-actions.h.  Can
> we define IPPORT_TFTP there as well?  I'm not sure if we want to leave
> it also in windows and sparse headers.

If it's defined there, do we need it in the sparse/windows headers?

>> diff --git a/tests/atlocal.in b/tests/atlocal.in
>> index 1353b46fd1ef..d03d5f3767b7 100644
>> --- a/tests/atlocal.in
>> +++ b/tests/atlocal.in
>> @@ -117,12 +117,24 @@ if test "$IS_WIN32" = "yes"; then
>>      HAVE_PYTHON3="no"
>>  fi
>>
>> -if test "$HAVE_PYTHON" = "yes" \
>> -   && test "x`$PYTHON $abs_top_srcdir/tests/test-l7.py --help | grep 'ftp'`" != x; then
>> -    HAVE_PYFTPDLIB="yes"
>> -else
>> -    HAVE_PYFTPDLIB="no"
>> -fi
>> +FindL7Lib()
>> +{
>> +    set +x
>> +    var=HAVE_`echo "$1" | tr '[a-z]' '[A-Z]'`
>> +    if test "$HAVE_PYTHON" = "yes"; then
>> +        result=$($PYTHON $abs_top_srcdir/tests/test-l7.py --help | grep "$1")
>> +        if test "x${result}" != x; then
>> +            eval ${var}="yes"
>> +        else
>> +            eval ${var}="no"
>> +        fi
>> +    else
>> +        eval ${var}="no"
>> +    fi
>> +}
>> +
>> +FindL7Lib ftp
>> +FindL7Lib tftp
>
> I think I'd prefer find_l7_lib over camel case.
>
> Maybe this could be addressed in a separate patch, but I don't see the
> need to do these checks at build time.  Can we do them at runtime?

I agree this makes more sense and would resolve the atlocal update issue.

>> diff --git a/tests/test-l7.py b/tests/test-l7.py
>> index aed34f4114d0..24255a2efdcb 100755
>> --- a/tests/test-l7.py
>> +++ b/tests/test-l7.py
>> @@ -48,17 +48,35 @@ def get_ftpd():
>>      return server
>>
>>
>> +def get_tftpd():
>> +    try:
>> +        from tftpy import TftpServer, TftpShared
>> +
>> +        class OVSTFTPServer(TftpServer):
>> +            def __init__(self, listen, handler=None):
>> +                (ip, port) = listen
>> +                self.ip = ip
>> +                self.port = port
>> +                TftpServer.__init__(self, tftproot='./')
>> +
>> +            def serve_forever(self):
>> +                self.listen(self.ip, self.port)
>> +        server = [OVSTFTPServer, None, TftpShared.DEF_TFTP_PORT]
>> +    except ImportError:
>> +        server = None
>> +        pass
>> +    return server
>> +
>> +
>>  def main():
>>      SERVERS = {
>>          'http': [TCPServer, SimpleHTTPRequestHandler, 80],
>>          'http6': [TCPServerV6, SimpleHTTPRequestHandler, 80],
>> +        'ftp': get_ftpd(),
>> +        'tftp': get_tftpd(),
>>      }
>>
>> -    ftpd = get_ftpd()
>> -    if ftpd is not None:
>> -        SERVERS['ftp'] = ftpd
>> -
>> -    protocols = [srv for srv in SERVERS]
>> +    protocols = [srv for srv in SERVERS if SERVERS[srv] is not None]
>
> Minor: there's a check below that checks if args.proto is not in
> SERVERS and fails.  Should we check now if args.proto is not in
> protocols?  Otherwise, when tftpy is not installed we will fail with
> an exception instead of exiting.

Yes, that's the right way to do this.