On 21 December 2016 at 17:06, Daniele Di Proietto <diproiet...@ovn.org> wrote: > 2016-12-20 13:28 GMT-08:00 Joe Stringer <j...@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 <j...@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 <diproiet...@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. _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev