On Fri, Dec 08, 2017 at 10:04:22PM +0800, Yi Yang wrote: > This patch changes OVS_KEY_ATTR_NSH > to nested attribute and adds three new NSH sub attribute keys: > > OVS_NSH_KEY_ATTR_BASE: for length-fixed NSH base header > OVS_NSH_KEY_ATTR_MD1: for length-fixed MD type 1 context > OVS_NSH_KEY_ATTR_MD2: for length-variable MD type 2 metadata > > Its intention is to align to NSH kernel implementation. > > NSH match fields, set and PUSH_NSH action all use the below > nested attribute format: > > OVS_KEY_ATTR_NSH begin > OVS_NSH_KEY_ATTR_BASE > OVS_NSH_KEY_ATTR_MD1 > OVS_KEY_ATTR_NSH end > > or > > OVS_KEY_ATTR_NSH begin > OVS_NSH_KEY_ATTR_BASE > OVS_NSH_KEY_ATTR_MD2 > OVS_KEY_ATTR_NSH end > > In addition, NSH encap and decap actions are renamed as push_nsh > and pop_nsh to meet action naming convention. > > Signed-off-by: Yi Yang <yi.y.y...@intel.com>
This fails to build with Clang (and, I would guess, MSVC): ../lib/odp-execute.c:497:21: error: fields must have a constant size: 'variable length array in structure' extension will never be supported "sparse" issues some warnings. This one is probably sensible: ../lib/odp-util.c:1884:32: warning: incorrect type in argument 1 (different base types) ../lib/odp-util.c:1884:32: expected unsigned int [unsigned] [usertype] x ../lib/odp-util.c:1884:32: got restricted ovs_be32 [assigned] [usertype] spi These I don't understand. https://marc.info/?l=linux-sparse&m=110218288411207 suggests that they might be a sparse bug: ../lib/odp-util.c:7123:32: warning: crazy programmer ../lib/odp-util.c:7123:43: warning: crazy programmer ../lib/odp-util.c:7123:22: warning: crazy programmer In odp_execute_actions(), this looks bogus: there is nothing to guarantee that 'buffer' is properly aligned for struct nsh_hdr. + uint8_t buffer[NSH_HDR_MAX_LEN]; + struct nsh_hdr *nsh_hdr = ALIGNED_CAST(struct nsh_hdr *, buffer); Similarly in format_odp_action(). In nsh_key_to_attr(), can this: + for (int i = 0; i < 4; i++) { + md1.context[i] = nsh->context[i]; + } + nl_msg_put_unspec(buf, OVS_NSH_KEY_ATTR_MD1, &md1, sizeof md1); be written as just this? + nl_msg_put_unspec(buf, OVS_NSH_KEY_ATTR_MD1, nsh->context, + sizeof nsh->context); and similarly in a second place. In parse_odp_push_nsh_action(), the idea of xmalloc()'ing a stub is weird. A stub should be a local array. There are many examples in the tree. Please don't check a pointer for null before calling free(): + if (metadata != NULL) { + free(metadata); } _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev