> 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.
Thanks Ben. > > 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; > So this was introduced in commit 1ae83bb20677b42d63dbb2140fa8ed3144c6260f: netdev-tc-offloads: Add support for action set. This was pushed just after I had submitted the pull request (but wasn't actually part of the pull request patches). It's strange though, I ran sparse before submitting the pull request and it came back clean. Checking after the merge and I don't see the warning you see. (Normally I would not submit a pull request if a sparse warning occurred). What version of sparse are you using out of interest? I'm using sparse-0.5.0-10. > 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? Sure, will be happy to review, but would also be good to get someone with tc-offload experience to sign off also. Roi has also submitted a patch to fix the issue, maybe Roi could review below also? https://mail.openvswitch.org/pipermail/ovs-dev/2017-November/341111.html > > 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; > } > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
