[ovs-dev] OVS DPDK: dpdk_merge pull request

Ben Pfaff blp at ovn.org
Mon Nov 20 17:13:44 UTC 2017


On Fri, Nov 17, 2017 at 07:27:51PM +0000, Stokes, Ian wrote:
> The following changes since commit 1ae83bb20677b42d63dbb2140fa8ed3144c6260f:
> 
>   netdev-tc-offloads: Add support for action set (2017-11-16 08:10:29 -0800)
> 
> are available in the git repository at:
> 
>   https://github.com/istokes/ovs dpdk_merge
> 
> for you to fetch changes up to af5b0dad30359d14d6412eb80e81783a83a678ce:
> 
>   netdev-dpdk: Fix mempool creation with large MTU. (2017-11-17 16:26:33 +0000)

Thanks a lot.  I merged this branch into master.

This yields a new "sparse" warning:
    ../lib/netdev-tc-offloads.c:584:20: warning: Variable length array is used.
because of this declaration in parse_put_flow_set_masked_action():
    char *set_buff[set_len], *set_data, *set_mask;

It may or may not be worth fixing that.  In favor of fixing it is
keeping OVS sparse warning free, but against it is that the replacement
would probably be malloc(), which is slower.

However, looking a bit closer (now that I've already done the merge), I
think there is a bug here.  set_buff[] is declared as char
*set_buff[set_len], which has size (set_len * sizeof(char *)), but only
set_len bytes of it are used.  I think that the right declaration would
be more like uint8_t set_buff[set_len];, although that would lack the
proper alignment for struct nl_attr.

Maybe something like this would be the appropriate fix for both issues.
Will you please review it?

Thanks,

Ben.

diff --git a/lib/netdev-tc-offloads.c b/lib/netdev-tc-offloads.c
index 781d849e4a87..d6b61f6360d0 100644
--- a/lib/netdev-tc-offloads.c
+++ b/lib/netdev-tc-offloads.c
@@ -581,7 +581,9 @@ parse_put_flow_set_masked_action(struct tc_flower *flower,
                                  bool hasmask)
 {
     static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 20);
-    char *set_buff[set_len], *set_data, *set_mask;
+    uint64_t set_stub[1024 / 8];
+    struct ofpbuf set_buf = OFPBUF_STUB_INITIALIZER(set_stub);
+    char *set_data, *set_mask;
     char *key = (char *) &flower->rewrite.key;
     char *mask = (char *) &flower->rewrite.mask;
     const struct nlattr *attr;
@@ -589,8 +591,7 @@ parse_put_flow_set_masked_action(struct tc_flower *flower,
     size_t size;
 
     /* copy so we can set attr mask to 0 for used ovs key struct members  */
-    memcpy(set_buff, set, set_len);
-    attr = (struct nlattr *) set_buff;
+    attr = ofpbuf_put(&set_buf, set, set_len);
 
     type = nl_attr_type(attr);
     size = nl_attr_get_size(attr) / 2;
@@ -600,6 +601,7 @@ parse_put_flow_set_masked_action(struct tc_flower *flower,
     if (type >= ARRAY_SIZE(set_flower_map)
         || !set_flower_map[type][0].size) {
         VLOG_DBG_RL(&rl, "unsupported set action type: %d", type);
+        ofpbuf_uninit(&set_buf);
         return EOPNOTSUPP;
     }
 
@@ -632,9 +634,11 @@ parse_put_flow_set_masked_action(struct tc_flower *flower,
     if (hasmask && !is_all_zeros(set_mask, size)) {
         VLOG_DBG_RL(&rl, "unsupported sub attribute of set action type %d",
                     type);
+        ofpbuf_uninit(&set_buf);
         return EOPNOTSUPP;
     }
 
+    ofpbuf_uninit(&set_buf);
     return 0;
 }
 


More information about the dev mailing list