| Message ID | 20190517160402.28095-1-aconole@redhat.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [ovs-dev] conntrack: add display support for sctp | expand |
Bleep bloop. Greetings Aaron Conole, I am a robot and I have tried out your patch.
Thanks for your contribution.
I encountered some error that I wasn't expecting. See the details below.
build:
mv -f $depbase.Tpo $depbase.Plo
libtool: compile: gcc -std=gnu99 -DHAVE_CONFIG_H -I. -I ./include -I ./include -I ./lib -I ./lib -Wstrict-prototypes -Wall -Wextra -Wno-sign-compare -Wpointer-arith -Wformat -Wformat-security -Wswitch-enum -Wunused-parameter -Wbad-function-cast -Wcast-align -Wstrict-prototypes -Wold-style-definition -Wmissing-prototypes -Wmissing-field-initializers -fno-strict-aliasing -Wshadow -Werror -Werror -g -O2 -MT lib/netdev-tc-offloads.lo -MD -MP -MF lib/.deps/netdev-tc-offloads.Tpo -c lib/netdev-tc-offloads.c -o lib/netdev-tc-offloads.o
depbase=`echo lib/netlink-conntrack.lo | sed 's|[^/]*$|.deps/&|;s|\.lo$||'`;\
/bin/sh ./libtool --tag=CC --mode=compile gcc -std=gnu99 -DHAVE_CONFIG_H -I. -I ./include -I ./include -I ./lib -I ./lib -Wstrict-prototypes -Wall -Wextra -Wno-sign-compare -Wpointer-arith -Wformat -Wformat-security -Wswitch-enum -Wunused-parameter -Wbad-function-cast -Wcast-align -Wstrict-prototypes -Wold-style-definition -Wmissing-prototypes -Wmissing-field-initializers -fno-strict-aliasing -Wshadow -Werror -Werror -g -O2 -MT lib/netlink-conntrack.lo -MD -MP -MF $depbase.Tpo -c -o lib/netlink-conntrack.lo lib/netlink-conntrack.c &&\
mv -f $depbase.Tpo $depbase.Plo
libtool: compile: gcc -std=gnu99 -DHAVE_CONFIG_H -I. -I ./include -I ./include -I ./lib -I ./lib -Wstrict-prototypes -Wall -Wextra -Wno-sign-compare -Wpointer-arith -Wformat -Wformat-security -Wswitch-enum -Wunused-parameter -Wbad-function-cast -Wcast-align -Wstrict-prototypes -Wold-style-definition -Wmissing-prototypes -Wmissing-field-initializers -fno-strict-aliasing -Wshadow -Werror -Werror -g -O2 -MT lib/netlink-conntrack.lo -MD -MP -MF lib/.deps/netlink-conntrack.Tpo -c lib/netlink-conntrack.c -o lib/netlink-conntrack.o
In file included from lib/netlink-conntrack.h:22:0,
from lib/netlink-conntrack.c:19:
lib/ct-dpif.h:105:39: error: redeclaration of enumerator 'SCTP_CONNTRACK_SHUTDOWN_ACK_SENT'
#define SCTP_CONNTRACK_HEARTBEAT_SENT SCTP_CONNTRACK_SHUTDOWN_ACK_SENT+1
^
In file included from lib/netlink-conntrack.c:27:0:
/usr/include/linux/netfilter/nf_conntrack_sctp.h:15:2: note: previous definition of 'SCTP_CONNTRACK_SHUTDOWN_ACK_SENT' was here
SCTP_CONNTRACK_SHUTDOWN_ACK_SENT,
^
In file included from lib/netlink-conntrack.h:22:0,
from lib/netlink-conntrack.c:19:
lib/ct-dpif.h:105:71: error: expected ',' or '}' before '+' token
#define SCTP_CONNTRACK_HEARTBEAT_SENT SCTP_CONNTRACK_SHUTDOWN_ACK_SENT+1
^
make[2]: *** [lib/netlink-conntrack.lo] Error 1
make[2]: Leaving directory `/var/lib/jenkins/jobs/upstream_build_from_pw/workspace'
make[1]: *** [all-recursive] Error 1
make[1]: Leaving directory `/var/lib/jenkins/jobs/upstream_build_from_pw/workspace'
make: *** [all] Error 2
Please check this out. If you feel there has been an error, please email aconole@bytheb.org
Thanks,
0-day Robot
On Fri, May 17, 2019 at 6:05 PM Aaron Conole <aconole@redhat.com> wrote: > Currently, only the netlink datapath supports SCTP connection tracking, > but at least this removes the warning message that will pop up when > running something like: > > ovs-appctl dpctl/dump-conntrack > > This doesn't impact any conntrack functionality, just the display. > > Signed-off-by: Aaron Conole <aconole@redhat.com> > --- > I did the heartbeat states by hardcoding the values in the ENUM since > they are fixed in the linux UAPI, and have existed for a long time, > but it could also make sense to rework this patch so that those states > are omitted by the ifdef rather than defining them (I'll submit a v2 > if that's preferred approach - but this seems like the more flexible > option). > > acinclude.m4 | 12 +++++++ > configure.ac | 1 + > lib/ct-dpif.c | 19 +++++++++++ > lib/ct-dpif.h | 30 ++++++++++++++++++ > lib/netlink-conntrack.c | 70 ++++++++++++++++++++++++++++++++++++++++- > 5 files changed, 131 insertions(+), 1 deletion(-) > > [snip] @@ -497,6 +503,16 @@ ct_dpif_format_protoinfo_tcp_verbose(struct ds *ds, > tcp_flags); > } > > +static void > +ct_dpif_format_protoinfo_sctp(struct ds *ds, > + const struct ct_dpif_protoinfo *protoinfo) > +{ > + ct_dpif_format_enum(ds, "state=", protoinfo->sctp.state, > + ct_dpif_sctp_state_string); > + ds_put_format(ds, ",vtag_orig=%u,vtag_reply=%u", > protoinfo->sctp.vtag_orig, > + protoinfo->sctp.vtag_reply); > PRIu32 for both of them?
David Marchand <david.marchand@redhat.com> writes: > On Fri, May 17, 2019 at 6:05 PM Aaron Conole <aconole@redhat.com> wrote: > > Currently, only the netlink datapath supports SCTP connection tracking, > but at least this removes the warning message that will pop up when > running something like: > > ovs-appctl dpctl/dump-conntrack > > This doesn't impact any conntrack functionality, just the display. > > Signed-off-by: Aaron Conole <aconole@redhat.com> > --- > I did the heartbeat states by hardcoding the values in the ENUM since > they are fixed in the linux UAPI, and have existed for a long time, > but it could also make sense to rework this patch so that those states > are omitted by the ifdef rather than defining them (I'll submit a v2 > if that's preferred approach - but this seems like the more flexible > option). > > acinclude.m4 | 12 +++++++ > configure.ac | 1 + > lib/ct-dpif.c | 19 +++++++++++ > lib/ct-dpif.h | 30 ++++++++++++++++++ > lib/netlink-conntrack.c | 70 ++++++++++++++++++++++++++++++++++++++++- > 5 files changed, 131 insertions(+), 1 deletion(-) > > [snip] > > @@ -497,6 +503,16 @@ ct_dpif_format_protoinfo_tcp_verbose(struct ds *ds, > tcp_flags); > } > > +static void > +ct_dpif_format_protoinfo_sctp(struct ds *ds, > + const struct ct_dpif_protoinfo *protoinfo) > +{ > + ct_dpif_format_enum(ds, "state=", protoinfo->sctp.state, > + ct_dpif_sctp_state_string); > + ds_put_format(ds, ",vtag_orig=%u,vtag_reply=%u", protoinfo->sctp.vtag_orig, > + protoinfo->sctp.vtag_reply); > > PRIu32 for both of them? Sure - that makes sense. I'll spin a v3.
diff --git a/acinclude.m4 b/acinclude.m4 index b532a4579..ca770ec81 100644 --- a/acinclude.m4 +++ b/acinclude.m4 @@ -212,6 +212,18 @@ AC_DEFUN([OVS_CHECK_LINUX_TC], [ [Define to 1 if TCA_SKBEDIT_FLAGS is available.])]) ]) +dnl OVS_CHECK_LINUX_SCTP_CT +dnl +dnl Checks for kernels which need additional SCTP state +AC_DEFUN([OVS_CHECK_LINUX_SCTP_CT], [ + AC_COMPILE_IFELSE([ + AC_LANG_PROGRAM([#include <linux/netfilter/nf_conntrack_sctp.h], [ + int x = SCTP_CONNTRACK_HEARTBEAT_SENT; + ])], + [AC_DEFINE([HAVE_SCTP_CONNTRACK_HEARTBEATS], [1], + [Define to 1 if SCTP_CONNTRACK_HEARTBEAT_SENT is available.])]) +]) + dnl OVS_FIND_DEPENDENCY(FUNCTION, SEARCH_LIBS, NAME_TO_PRINT) dnl dnl Check for a function in a library list. diff --git a/configure.ac b/configure.ac index 505e3d041..2dbe9a917 100644 --- a/configure.ac +++ b/configure.ac @@ -186,6 +186,7 @@ AC_ARG_VAR(KARCH, [Kernel Architecture String]) AC_SUBST(KARCH) OVS_CHECK_LINUX OVS_CHECK_LINUX_TC +OVS_CHECK_LINUX_SCTP_CT OVS_CHECK_DPDK OVS_CHECK_PRAGMA_MESSAGE AC_SUBST([OVS_CFLAGS]) diff --git a/lib/ct-dpif.c b/lib/ct-dpif.c index b2c9b4309..17e348ea9 100644 --- a/lib/ct-dpif.c +++ b/lib/ct-dpif.c @@ -428,6 +428,12 @@ const char *ct_dpif_tcp_state_string[] = { #undef CT_DPIF_TCP_STATE }; +const char *ct_dpif_sctp_state_string[] = { +#define CT_DPIF_SCTP_STATE(STATE) [CT_DPIF_SCTP_STATE_##STATE] = #STATE, + CT_DPIF_SCTP_STATES +#undef CT_DPIF_SCTP_STATE +}; + static void ct_dpif_format_enum__(struct ds *ds, const char *title, unsigned int state, const char *names[], unsigned int max) @@ -497,6 +503,16 @@ ct_dpif_format_protoinfo_tcp_verbose(struct ds *ds, tcp_flags); } +static void +ct_dpif_format_protoinfo_sctp(struct ds *ds, + const struct ct_dpif_protoinfo *protoinfo) +{ + ct_dpif_format_enum(ds, "state=", protoinfo->sctp.state, + ct_dpif_sctp_state_string); + ds_put_format(ds, ",vtag_orig=%u,vtag_reply=%u", protoinfo->sctp.vtag_orig, + protoinfo->sctp.vtag_reply); +} + static void ct_dpif_format_protoinfo(struct ds *ds, const char *title, const struct ct_dpif_protoinfo *protoinfo, @@ -514,6 +530,9 @@ ct_dpif_format_protoinfo(struct ds *ds, const char *title, ct_dpif_format_protoinfo_tcp(ds, protoinfo); } break; + case IPPROTO_SCTP: + ct_dpif_format_protoinfo_sctp(ds, protoinfo); + break; } if (title) { ds_put_cstr(ds, ")"); diff --git a/lib/ct-dpif.h b/lib/ct-dpif.h index 0151cfea4..34e41ed30 100644 --- a/lib/ct-dpif.h +++ b/lib/ct-dpif.h @@ -101,6 +101,31 @@ enum ct_dpif_tcp_flags { #undef CT_DPIF_TCP_FLAG }; +#ifndef HAVE_SCTP_CONNTRACK_HEARTBEATS +#define SCTP_CONNTRACK_HEARTBEAT_SENT SCTP_CONNTRACK_SHUTDOWN_ACK_SENT+1 +#define SCTP_CONNTRACK_HEARTBEAT_ACKED SCTP_CONNTRACK_SHUTDOWN_ACK_SENT+2 +#endif + +extern const char *ct_dpif_sctp_state_string[]; + +#define CT_DPIF_SCTP_STATES \ + CT_DPIF_SCTP_STATE(CLOSED) \ + CT_DPIF_SCTP_STATE(COOKIE_WAIT) \ + CT_DPIF_SCTP_STATE(COOKIE_ECHOED) \ + CT_DPIF_SCTP_STATE(ESTABLISHED) \ + CT_DPIF_SCTP_STATE(SHUTDOWN_SENT) \ + CT_DPIF_SCTP_STATE(SHUTDOWN_RECD) \ + CT_DPIF_SCTP_STATE(SHUTDOWN_ACK_SENT) \ + CT_DPIF_SCTP_STATE(HEARTBEAT_SENT) \ + CT_DPIF_SCTP_STATE(HEARTBEAT_ACKED) \ + CT_DPIF_SCTP_STATE(MAX_NUM) + +enum ct_dpif_sctp_state { +#define CT_DPIF_SCTP_STATE(STATE) CT_DPIF_SCTP_STATE_##STATE, + CT_DPIF_SCTP_STATES +#undef CT_DPIF_SCTP_STATE +}; + struct ct_dpif_protoinfo { uint16_t proto; /* IPPROTO_* */ union { @@ -112,6 +137,11 @@ struct ct_dpif_protoinfo { uint8_t flags_orig; uint8_t flags_reply; } tcp; + struct { + uint8_t state; + uint32_t vtag_orig; + uint32_t vtag_reply; + } sctp; }; }; diff --git a/lib/netlink-conntrack.c b/lib/netlink-conntrack.c index 42be1d9ce..7631ba5d5 100644 --- a/lib/netlink-conntrack.c +++ b/lib/netlink-conntrack.c @@ -717,6 +717,73 @@ nl_ct_parse_protoinfo_tcp(struct nlattr *nla, return parsed; } +/* Translate netlink SCTP state to CT_DPIF_SCTP state. */ +static uint8_t +nl_ct_sctp_state_to_dpif(uint8_t state) +{ +#ifdef _WIN32 + /* For now, return the CT_DPIF_SCTP state. Not sure what windows does. */ + return state; +#else + switch (state) { + case SCTP_CONNTRACK_COOKIE_WAIT: + return CT_DPIF_SCTP_STATE_COOKIE_WAIT; + case SCTP_CONNTRACK_COOKIE_ECHOED: + return CT_DPIF_SCTP_STATE_COOKIE_ECHOED; + case SCTP_CONNTRACK_ESTABLISHED: + return CT_DPIF_SCTP_STATE_ESTABLISHED; + case SCTP_CONNTRACK_SHUTDOWN_SENT: + return CT_DPIF_SCTP_STATE_SHUTDOWN_SENT; + case SCTP_CONNTRACK_SHUTDOWN_RECD: + return CT_DPIF_SCTP_STATE_SHUTDOWN_RECD; + case SCTP_CONNTRACK_SHUTDOWN_ACK_SENT: + return CT_DPIF_SCTP_STATE_SHUTDOWN_ACK_SENT; + case SCTP_CONNTRACK_HEARTBEAT_SENT: + return CT_DPIF_SCTP_STATE_HEARTBEAT_SENT; + case SCTP_CONNTRACK_HEARTBEAT_ACKED: + return CT_DPIF_SCTP_STATE_HEARTBEAT_ACKED; + case SCTP_CONNTRACK_CLOSED: + /* Fall Through. */ + case SCTP_CONNTRACK_NONE: + /* Fall Through. */ + default: + return CT_DPIF_SCTP_STATE_CLOSED; + } +#endif +} + +static bool +nl_ct_parse_protoinfo_sctp(struct nlattr *nla, + struct ct_dpif_protoinfo *protoinfo) +{ + static const struct nl_policy policy[] = { + [CTA_PROTOINFO_SCTP_STATE] = { .type = NL_A_U8, .optional = false }, + [CTA_PROTOINFO_SCTP_VTAG_ORIGINAL] = { .type = NL_A_U32, + .optional = false }, + [CTA_PROTOINFO_SCTP_VTAG_REPLY] = { .type = NL_A_U32, + .optional = false }, + }; + struct nlattr *attrs[ARRAY_SIZE(policy)]; + bool parsed; + + parsed = nl_parse_nested(nla, policy, attrs, ARRAY_SIZE(policy)); + if (parsed) { + protoinfo->proto = IPPROTO_SCTP; + + protoinfo->sctp.state = nl_ct_sctp_state_to_dpif( + nl_attr_get_u8(attrs[CTA_PROTOINFO_SCTP_STATE])); + protoinfo->sctp.vtag_orig = nl_attr_get_u32( + attrs[CTA_PROTOINFO_SCTP_VTAG_ORIGINAL]); + protoinfo->sctp.vtag_reply = nl_attr_get_u32( + attrs[CTA_PROTOINFO_SCTP_VTAG_REPLY]); + } else { + VLOG_ERR_RL(&rl, "Could not parse nested SCTP protoinfo options. " + "Possibly incompatible Linux kernel version."); + } + + return parsed; +} + static bool nl_ct_parse_protoinfo(struct nlattr *nla, struct ct_dpif_protoinfo *protoinfo) { @@ -737,7 +804,8 @@ nl_ct_parse_protoinfo(struct nlattr *nla, struct ct_dpif_protoinfo *protoinfo) parsed = nl_ct_parse_protoinfo_tcp(attrs[CTA_PROTOINFO_TCP], protoinfo); } else if (attrs[CTA_PROTOINFO_SCTP]) { - VLOG_WARN_RL(&rl, "SCTP protoinfo not yet supported!"); + parsed = nl_ct_parse_protoinfo_sctp(attrs[CTA_PROTOINFO_SCTP], + protoinfo); } else { VLOG_WARN_RL(&rl, "Empty protoinfo!"); }
Currently, only the netlink datapath supports SCTP connection tracking, but at least this removes the warning message that will pop up when running something like: ovs-appctl dpctl/dump-conntrack This doesn't impact any conntrack functionality, just the display. Signed-off-by: Aaron Conole <aconole@redhat.com> --- I did the heartbeat states by hardcoding the values in the ENUM since they are fixed in the linux UAPI, and have existed for a long time, but it could also make sense to rework this patch so that those states are omitted by the ifdef rather than defining them (I'll submit a v2 if that's preferred approach - but this seems like the more flexible option). acinclude.m4 | 12 +++++++ configure.ac | 1 + lib/ct-dpif.c | 19 +++++++++++ lib/ct-dpif.h | 30 ++++++++++++++++++ lib/netlink-conntrack.c | 70 ++++++++++++++++++++++++++++++++++++++++- 5 files changed, 131 insertions(+), 1 deletion(-)