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
_______________________________________________
dev mailing list
[email protected]
http://openvswitch.org/mailman/listinfo/dev
