[ovs-dev] [PATCHv4 06/11] Add support for connection tracking.

Daniele Di Proietto diproiettod at vmware.com
Wed Oct 7 18:11:02 UTC 2015


Hi Joe,

I have a couple of minor comments inline

On 02/10/2015 22:16, "Joe Stringer" <joestringer at nicira.com> wrote:

>This patch adds a new action and fields to OVS that allow connection
>tracking to be performed. This support works in conjunction with the
>Linux kernel support merged into the Linux-4.3 development cycle.
[...]
>diff --git a/lib/odp-util.c b/lib/odp-util.c
>index c173623..a570afa 100644
>--- a/lib/odp-util.c
>+++ b/lib/odp-util.c
[...]
>@@ -2605,6 +2790,25 @@ scan_tcp_flags(const char *s, ovs_be16 *key,
>ovs_be16 *mask)
> }
> 
> static int
>+scan_ct_state(const char *s, uint32_t *key, uint32_t *mask)
>+{
>+    uint32_t flags, fmask;
>+    int n;
>+
>+    n = parse_flags(s, ct_state_to_string, ')', NULL, NULL, &flags,
>+                    CS_SUPPORTED_MASK, mask ? &fmask : NULL);
>+

Should we scan these flags as OVS_CS_F* (the odp format)?
Here they're treated as CS_* (the OpenFlow format).

In this case it could be something like

n = parse_flags(s, odp_ct_state_to_string, ')', NULL, NULL, &flags,
                ovs_to_odp_ct_state(CS_SUPPORTED_MASK, false),
                mask ? &fmask : NULL);



>+    if (n >= 0) {
>+        *key = flags;
>+        if (mask) {
>+            *mask = fmask;
>+        }
>+        return n;
>+    }
>+    return 0;
>+}
>+
>+static int
> scan_frag(const char *s, uint8_t *key, uint8_t *mask)
> {
>     int n;
[...]
>
> 
>diff --git a/tests/odp.at b/tests/odp.at
>index 61edde3..16d7411 100644
>--- a/tests/odp.at
>+++ b/tests/odp.at
>@@ -86,6 +86,11 @@ sed '/bos=0/{
> s/^/ODP_FIT_TOO_LITTLE: /
> }' < odp-in.txt > odp-out.txt
> 
>+dnl Some fields are always printed for this test, because wildcards
>aren't
>+dnl specified. We can skip these.
>+sed -i 's/\(skb_mark(0)\),\(ct\)/\1,ct_state(0),ct_zone(0),\2/'
>odp-out.txt
>+sed -i 
>'s/\(skb_mark([[^)]]*)\),\(recirc\)/\1,ct_state(0),ct_zone(0),\2/'
>odp-out.txt
>+
> AT_CHECK_UNQUOTED([ovstest test-odp parse-keys < odp-in.txt], [0], [`cat
>odp-out.txt`
> ])
> AT_CLEANUP
>@@ -153,6 +158,10 @@
>s/\(eth([[^)]]*)\),*/\1,eth_type(0x8100),vlan(vid=99,pcp=7),encap(/
> s/$/)/' odp-base.txt
> 
>  echo
>+ echo '# Valid forms with conntrack fields.'
>+ sed 's/\(eth([[^)]]*)\),/\1,ct_state(+trk),ct_zone(5/0xff),/'
>odp-base.txt

I think we should escape the forward slash and use a hex prefix for the
zone

...ct_zone(0x5\/0xff)...


With this fix, the testcase should detect the above issue.

Thanks!




More information about the dev mailing list