[ovs-dev] [PATCHv2 1/6] Add support for connection tracking.

[Daniele Di Proietto]

Hi Joe,

thanks for sending this!

While doing some testing with my userspace connection tracker
on top of your series I encountered some small issues that
I was hoping you could squash in before pushing it to master.

None of the comments is supposed to be a blocker, we can address
them after merging, as far as I'm concerned.

>@@ -2103,6 +2148,22 @@ mf_from_tun_flags_string(const char *s, ovs_be16
>*flagsp, ovs_be16 *maskp)
>                           htons(FLOW_TNL_PUB_F_MASK), maskp);
> }
>
>+static char *
>+mf_from_ct_state_string(const char *s, ovs_be16 *flagsp, ovs_be16 *maskp)
>+{
>+    ovs_be16 flags, mask;
>+    char *error;
>+
>+    error = parse_mf_flags(s, packet_ct_state_to_string, "ct_state",
>&flags,
>+                           htons(CS_SUPPORTED_MASK), &mask);
>+    if (!error) {
>+        *flagsp = flags;
>+        *maskp = mask;
>+    }
>+
>+    return error;
>+}
>+
> /* Parses 's', a string value for field 'mf', into 'value' and 'mask'.
>Returns
>  * NULL if successful, otherwise a malloc()'d string describing the
>error. */
> char *

I get a warning with GCC 4.9.2 for this function. It's complaining that the
'flags' variable may be used without initialization. Rewriting it like
this suppresses the warning for me:

static char *
mf_from_ct_state_string(const char *s, ovs_be16 *flagsp, ovs_be16 *maskp)
{
    return parse_mf_flags(s, packet_ct_state_to_string, "ct_state", flagsp,
                          htons(CS_SUPPORTED_MASK), maskp);
}


>@@ -2057,6 +2180,24 @@ format_odp_key_attr(const struct nlattr *a, const
>struct nlattr *ma,
>         }
>         break;
>
>+    case OVS_KEY_ATTR_CT_STATE:
>+        if (!is_exact) {
>+            format_flags_masked(ds, NULL, packet_ct_state_to_string,
>+                                nl_attr_get_u8(a), nl_attr_get_u8(ma),
>+                                UINT8_MAX);
>+        } else {
>+            format_flags(ds, packet_ct_state_to_string,
>+                         nl_attr_get_u8(a), ',');
>+        }
>+        break;
>+
>+    case OVS_KEY_ATTR_CT_ZONE:
>+        if (verbose || !mask_empty(ma)) {
>+            ds_put_format(ds, "%"PRIx16, nl_attr_get_u16(a));
>+        }
>+        break;


Is there a particular reasons why we're not printing the mask here? When I
dump flows using the verbose option and I see 'ct_zone(0)' I assume that
we're not wildcarding the zone.

Also shouldn't we prefix PRIx16 with "#" (like we're doing elsewhere in the
function)?

Do you think this is acceptable?

    case OVS_KEY_ATTR_CT_ZONE:
        if (verbose || !mask_empty(ma)) {
            ds_put_format(ds, "%#"PRIx16, nl_attr_get_u16(a));
            if (!is_exact) {
                ds_put_format(ds, "/%#"PRIx16, nl_attr_get_u16(ma));
            }
        }
        break;

>@@ -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),/' odp-base.txt

It seems that this regex doesn't actually change the flows.

How about like this? I also had to remove the comma at the end.

sed 's/\(eth([[^)]]*)\)/\1,ct_state(+trk)/' odp-base.txt

I would also introduce a ct_zone() attribute here, to test the above
change.

Also, the next commits should introduce hex numbers with lowercase letters
instead of uppercase

>+
>+ echo
>  echo '# Valid forms with IP first fragment.'
> sed -n 's/,frag=no),/,frag=first),/p' odp-base.txt

I'm also getting a test failure (ofproto-dpif - in place modification),
because formatting odp flows with the verbose flag introduces new
attributes.

Thanks,

Daniele