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

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