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

["Yang, Yi" <yi.y.yang-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>]

On Mon, Aug 21, 2017 at 11:18:54AM +0200, Jiri Benc wrote:
> On Mon, 21 Aug 2017 16:39:01 +0800, Yang, Yi wrote:
> > Anyway, we need to keep the code in userspace consistent with the one in
> > kernel as possible as, otherwise it will be a burden for developer, I
> > know userspace has different coding standard from kernel, this will make
> > developer painful if we have two sets of code although they have same
> > functionality.
> 
> I'm sorry, I don't get this. What's wrong with having __u8[] as the
> last member of the struct? That's C99. It's 18 years old standard.
> We're using that throughout our uAPI. Why that should be a problem for
> any user space program?

The issue is it is used union in

struct nsh_hdr {
    ovs_be16 ver_flags_ttl_len;
    uint8_t md_type;
    uint8_t next_proto;
    ovs_16aligned_be32 path_hdr;
    union {
        struct nsh_md1_ctx md1;
        struct nsh_md2_tlv md2;
    };
};

in Linux kernel build, it complained it, I changed it to

struct nsh_hdr {
    ovs_be16 ver_flags_ttl_len;
    uint8_t md_type;
    uint8_t next_proto;
    ovs_16aligned_be32 path_hdr;
    union {
        struct nsh_md1_ctx md1;
        struct nsh_md2_tlv md2[0];
    };
};

It is ok, but for Microsoft compiler, it isn't allowed there is struct
nsh_md2_tlv md2[0] in a union, that is Ben Pfaff's hack :-)

> 
> > > MPLS supports GSO and needs this for segmentation. I don't see anything
> > > GSO related in this patch.
> > > 
> > > How do you plan to address GSO, anyway?
> > 
> > No plan to do that, I'm not an expert on this, we can remove it if
> > you're very sure it is necessary.
> 
> Without GSO, I don't see any use for inner_protocol.
> 
> However, don't you need to software segment the packet if it's GSO
> before pushing the NSH header?
> 
> And wouldn't it be better to implement GSO for NSH, anyway?

I don't know how we can support this, is it a must-have thing?

> 
> > To make sure we make agreement, please confirm if this one is ok?
> > 
> > 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;
> >     };
> > };
> > 
> > Or it will be better if you can provide your preferred version here.
> 
> I don't really care that much about the names if it's clear what they
> mean. I was merely commenting on the inconsistency which looked weird.
> Whether it's md_type or mdtype, I don't have a preference (does not
> mean others won't, though :-)). Just pick one and stick to it, as far
> as I'm concerned.

But struct nsh_hdr had different struct from struct ovs_key_nsh, we
have no way to make them completely same, do you mean we should use the
same name if they are same fields and represent the same thing?

> 
>  Jiri