On 18 September 2015 at 09:47, Daniele Di Proietto
<diproiet...@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.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
