[ovs-dev] [megaflow v5 2/2] ovs-dpctl: Add mega flow support

Andy Zhou azhou at nicira.com
Tue Jun 18 22:51:09 UTC 2013


Thanks for the review. The changes will be rolled into V6 of the patch, to
be sent out soon.


On Tue, Jun 18, 2013 at 2:48 PM, Ben Pfaff <blp at nicira.com> wrote:

> On Mon, Jun 17, 2013 at 07:51:01AM -0700, Andy Zhou wrote:
> > Added support to allow mega flow specified and displayed. ovs-dpctl tool
> > is mainly used as debugging tool.
> >
> > This patch also implements the low level user space routines to send
> > and receive mega flow netlink messages. Those netlink suppor
> > routines are required for forthcoming user space mega flow patches.
> >
> > Added a unit test to test parsing and display of mega flows.
> >
> > Ethan contributed the ovs-dpctl mega flow output function.
> >
> > Co-authored-by: Ethan Jackson <ethan at nicira.com>
> > Signed-off-by: Ethan Jackson <ethan at nicira.com>
> > Signed-off-by: Andy Zhou <azhou at nicira.com>
>
> GCC complains:
>
>     ../lib/odp-util.c: In function ‘parse_odp_key_mask_attr’:
>     ../lib/odp-util.c:2151: error: ‘encap_mask’ may be used uninitialized
> in this function
>     ../lib/odp-util.c: In function ‘format_odp_key_attr’:
>     ../lib/odp-util.c:909: error: ‘is_exact’ may be used uninitialized in
> this function
>
>
> In lib/dpif-linux.c, there is a missing space before the ':' in the
> conditional operator:
>
> @@ -939,10 +944,14 @@ dpif_linux_flow_dump_next(const struct dpif *dpif_
> OVS_UNUSED, void *state_,
>      }
>      if (key) {
>          *key = state->flow.key;
>          *key_len = state->flow.key_len;
>      }
> +    if (mask) {
> +        *mask = state->flow.mask;
> +        *mask_len = state->flow.mask ? state->flow.mask_len: 0;
> +    }
>
> format_mpls() in odp-util.c has a style typo here:
> +    }else {
>
> In format_odp_key_attr(), is_exact is left uninitialized if attr ==
> OVS_KEY_ATTR_TUNNEL but tun_mask.flags != (...long expression...).
>
> I think that an odp_mask_attr_is_exact() function is warranted and
> would make the code more readable.  You'd end up with
>         if (ma && odp_mask_attr_is_exact(ma)) {
>             ma = NULL;
>         }
> which is easy to understand.
>
> This code here in format_odp_key_attr() looks a little awkward to me:
>     {
>         bool bad_key_len, bad_mask_len;
>         expected_len = odp_flow_key_attr_len(nl_attr_type(a));
>         bad_key_len = (expected_len != -2 && nl_attr_get_size(a) !=
> expected_len);
>
>         if (ma) {
>             expected_len = odp_flow_key_attr_len(nl_attr_type(ma));
>             bad_mask_len = (expected_len != -2 && nl_attr_get_size(ma) !=
> expected_len);
>         } else {
>             bad_mask_len = false;
>         }
>
> How about this instead:
>     expected_len = odp_flow_key_attr_len(nl_attr_type(a));
>     if (expected_len != -2) {
>         bool bad_key_len = nl_attr_get_size(a) != expected_len;
>         bool bad_mask_len = ma && nl_attr_get_size(ma) != expected_len;
>
> I think that's all.
>
> Thanks,
>
> Ben.
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openvswitch.org/pipermail/ovs-dev/attachments/20130618/27da3e1d/attachment-0003.html>


More information about the dev mailing list