Thanks for the reviews Joe! Comments below. > On Mar 9, 2016, at 7:47 PM, Joe Stringer <joe@xxxxxxx> wrote: > > Hi Jarno, > > Thanks for working on this. Mostly just a few style things around #ifdefs below. > > On 9 March 2016 at 15:10, Jarno Rajahalme <jarno@xxxxxxx> wrote: >> Extend OVS conntrack interface to cover NAT. New nested >> OVS_CT_ATTR_NAT attribute may be used to include NAT with a CT action. >> A bare OVS_CT_ATTR_NAT only mangles existing and expected connections. >> If OVS_NAT_ATTR_SRC or OVS_NAT_ATTR_DST is included within the nested >> attributes, new (non-committed/non-confirmed) connections are mangled >> according to the rest of the nested attributes. >> >> The corresponding OVS userspace patch series includes test cases (in >> tests/system-traffic.at) that also serve as example uses. >> >> This work extends on a branch by Thomas Graf at >> https://github.com/tgraf/ovs/tree/nat. > > Thomas, I guess there was not signoff in these patches so Jarno does > not have your signoff in this patch. > >> Signed-off-by: Jarno Rajahalme <jarno@xxxxxxx> >> --- >> v9: Fixed module dependencies. >> >> include/uapi/linux/openvswitch.h | 49 ++++ >> net/openvswitch/Kconfig | 3 +- >> net/openvswitch/conntrack.c | 523 +++++++++++++++++++++++++++++++++++++-- >> net/openvswitch/conntrack.h | 3 +- >> 4 files changed, 551 insertions(+), 27 deletions(-) > > <snip> > >> diff --git a/net/openvswitch/Kconfig b/net/openvswitch/Kconfig >> index cd5fd9d..23471a4 100644 >> --- a/net/openvswitch/Kconfig >> +++ b/net/openvswitch/Kconfig >> @@ -6,7 +6,8 @@ config OPENVSWITCH >> tristate "Open vSwitch" >> depends on INET >> depends on !NF_CONNTRACK || \ >> - (NF_CONNTRACK && (!NF_DEFRAG_IPV6 || NF_DEFRAG_IPV6)) >> + (NF_CONNTRACK && ((!NF_DEFRAG_IPV6 || NF_DEFRAG_IPV6) && \ >> + (!NF_NAT || NF_NAT))) > > Whitespace. > Fixed. >> select LIBCRC32C >> select MPLS >> select NET_MPLS_GSO >> diff --git a/net/openvswitch/conntrack.c b/net/openvswitch/conntrack.c >> index 5711f80..6455237 100644 >> --- a/net/openvswitch/conntrack.c >> +++ b/net/openvswitch/conntrack.c > > <snip> > >> struct ovs_ct_len_tbl { >> - size_t maxlen; >> - size_t minlen; >> + int maxlen; >> + int minlen; >> }; > > Are these changed for a specific reason, or just to use INT_MAX rather > than SIZE_MAX in ovs_ct_len_tbl? > ‘maxlen’ and ‘minlen’ are compared against the values returned by nla_len(), which returns an int: net/netlink.h:static inline int nla_len(const struct nlattr *nla) so I figured it is better to have these as ints, too. >> /* Metadata mark for masked write to conntrack mark */ >> @@ -42,15 +52,29 @@ struct md_labels { >> struct ovs_key_ct_labels mask; >> }; >> >> +#ifdef CONFIG_NF_NAT_NEEDED >> +enum ovs_ct_nat { >> + OVS_CT_NAT = 1 << 0, /* NAT for committed connections only. */ >> + OVS_CT_SRC_NAT = 1 << 1, /* Source NAT for NEW connections. */ >> + OVS_CT_DST_NAT = 1 << 2, /* Destination NAT for NEW connections. */ >> +}; >> +#endif > > Here... > >> /* Conntrack action context for execution. */ >> struct ovs_conntrack_info { >> struct nf_conntrack_helper *helper; >> struct nf_conntrack_zone zone; >> struct nf_conn *ct; >> u8 commit : 1; >> +#ifdef CONFIG_NF_NAT_NEEDED >> + u8 nat : 3; /* enum ovs_ct_nat */ >> +#endif > > and here.. I wonder if we can trim more of these #ifdefs, for > readability and more compiler coverage if the feature is disabled. > Trimmed this and other #ifdefs as you suggested, and it still compiles when NAT is disabled. Just posted the v10, which I hope will be the final version :-) Jarno -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
