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 <[email protected]>
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
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
