Re: [PATCH net-next v4] openvswitch: enable NSH support

[Jiri Benc <jbenc-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>]

On Mon, 21 Aug 2017 09:04:30 +0000, Jan Scheurich wrote:
> The second member of the union should be a variable length array []
> of struct nsh_md2_tlv
> 
> struct nsh_hdr {
>     ovs_be16 ver_flags_ttl_len;
>     uint8_t mdtype;
>     uint8_t np;
>     ovs_16aligned_be32 path_hdr;
>     union {
>         struct nsh_md1_ctx md1;
>         struct nsh_md2_tlv md2[];

I'm not that sure about this. With each member of md2 having
a different size, you can't use md2 as an array. However, if it was
declared as an array, it might encourage such (wrong) usage.

In particular, nsh_hdr->md2[1] is always wrong.

It seems better to not declare md2 as an array.

>     };
> };
> 
> That was the original design before Ben removed it due to missing
> support in Microsoft compiler. For the Kernel datapath we should go
> back to that. 
> 
> I wonder about the possible 16-bit alignment of the 32-bit fields,
> though. How is that handled in the kernel?

get_unaligned_*

> Also struct nsh_md1_ctx
> has 32-bit members, which might not be 32-bit aligned in the packet.

I don't see that happening, it seems the header before md1 is 8 bytes
and sizeof(md1) is 32 bytes? And for md2, the standard mandates that
the md2 size is a multiply of 4 bytes, too.

 Jiri