Re: [PATCH nf-next v9 8/8] openvswitch: Interface with NAT.

Thanks for the reviews Joe! Comments below.

On Mar 9, 2016, at 7:47 PM, Joe Stringer [off-list ref] 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 [off-list ref] wrote:

quoted

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.

quoted

Signed-off-by: Jarno Rajahalme <redacted>
---
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>

quoted

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.

quoted

       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>

quoted

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.

quoted

/* 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...

quoted

/* 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@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html