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

[Joe Stringer]

On 18 September 2015 at 09:47, Daniele Di Proietto
<diproiettod at vmware.com> wrote:
> 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;

Thanks Daniele, I took these fragments as-is.


>>@@ -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 for the feedback, will do another once-over on the corners of
the testsuite like these.