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

Ben Pfaff blp at nicira.com
Tue Jun 18 21:48:54 UTC 2013


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.



More information about the dev mailing list