struct ovs_action_encap_nsh is the only one way we transfer all the data for encap_nsh, netlink allows variable attribute, so I don't think we break netlink convention or abuse this variable feature.
Even if we bring nested attributes to handle this, OVS_ACTION_ATTR_ENCAP_NSH is still length-variable, OVS_NSH_ATTR_MD2 is also length-variable (it can be from 0 to 248), so I don't think such way can avoid the issue you're addressing. The result will be worse, it will make many difficulties when we transfer all the data for encap_nsh between OVS' components. -----Original Message----- From: Ben Pfaff [mailto:b...@ovn.org] Sent: Thursday, August 10, 2017 4:54 AM To: Yang, Yi Y <yi.y.y...@intel.com> Cc: Jan Scheurich <jan.scheur...@ericsson.com>; d...@openvswitch.org; netdev@vger.kernel.org; Jiri Benc <jb...@redhat.com>; da...@davemloft.net; Zoltán Balogh <zoltan.bal...@ericsson.com> Subject: Re: [ovs-dev] [PATCH net-next] openvswitch: add NSH support On Wed, Aug 09, 2017 at 08:12:36PM +0000, Yang, Yi Y wrote: > Ben, do you mean we bring two new attributes (OVS_NSH_ATTR_MD1 and > OVS_NSH_ATTR_MD2) and embed one of them in OVS_ACTION_ATTR_ENCAP_NSH? Yes. > Anyway we need to use a struct or something else to transfer those > metadata between functions, how do you think we can handle this > without metadata in struct ovs_action_encap_nsh? I mean how we handle > the arguments for function encap_nsh. I guess that a pointer to the embedded nlattr with type OVS_NSH_ATTR_MD1 or OVS_NSH_ATTR2 should work OK. Keep in mind that I'm not a kernel-side maintainer of any kind. I am only passing along what I've perceived to be common Netlink protocol design patterns. > -----Original Message----- > From: netdev-ow...@vger.kernel.org > [mailto:netdev-ow...@vger.kernel.org] On Behalf Of Ben Pfaff > Sent: Thursday, August 10, 2017 2:09 AM > To: Yang, Yi Y <yi.y.y...@intel.com> > Cc: Jan Scheurich <jan.scheur...@ericsson.com>; d...@openvswitch.org; > netdev@vger.kernel.org; Jiri Benc <jb...@redhat.com>; > da...@davemloft.net; Zoltán Balogh <zoltan.bal...@ericsson.com> > Subject: Re: [ovs-dev] [PATCH net-next] openvswitch: add NSH support > > On Wed, Aug 09, 2017 at 09:41:51AM +0000, Yang, Yi Y wrote: > > Hi, Jan > > > > I have worked out a patch, will send it quickly for Ben. In > > addition, I also will send out a patch to change encap_nsh > > &decap_nsh to push_nsh and pop_nsh. Per comments from all the > > people, we all agreed to do so :-) > > > > diff --git a/datapath/linux/compat/include/linux/openvswitch.h > > b/datapath/linux/compat/include/linux/openvswitch.h > > index bc6c94b..4936c12 100644 > > --- a/datapath/linux/compat/include/linux/openvswitch.h > > +++ b/datapath/linux/compat/include/linux/openvswitch.h > > @@ -793,7 +793,7 @@ struct ovs_action_push_eth { > > struct ovs_key_ethernet addresses; }; > > > > -#define OVS_ENCAP_NSH_MAX_MD_LEN 16 > > +#define OVS_ENCAP_NSH_MAX_MD_LEN 248 > > /* > > * struct ovs_action_encap_nsh - %OVS_ACTION_ATTR_ENCAP_NSH > > * @flags: NSH header flags. > > @@ -809,7 +809,7 @@ struct ovs_action_encap_nsh { > > uint8_t mdlen; > > uint8_t np; > > __be32 path_hdr; > > - uint8_t metadata[OVS_ENCAP_NSH_MAX_MD_LEN]; > > + uint8_t metadata[]; > > }; > > This brings the overall format of ovs_action_encap_nsh to: > > struct ovs_action_encap_nsh { > uint8_t flags; > uint8_t mdtype; > uint8_t mdlen; > uint8_t np; > __be32 path_hdr; > uint8_t metadata[]; > }; > > This is an unusual format for a Netlink attribute. More commonly, one would > put variable-length data into an attribute of its own, which allows that data > to be handled using the regular Netlink means. Then the mdlen and metadata > members could be removed, since they would be part of the additional > attribute, and one might expect the mdtype member to be removed as well since > each type of metadata would be in a different attribute type. > > So, a format closer to what I expect to see in Netlink is something > like > this: > > /** > * enum ovs_nsh_attr - Metadata attributes for %OVS_ACTION_ENCAP_NSH action. > * > * @OVS_NSH_ATTR_MD1: Contains 16-byte NSH type-1 metadata. > * @OVS_NSH_ATTR_MD2: Contains 0- to 255-byte variable-length NSH > type-2 > * metadata. */ > enum ovs_nsh_attr { > OVS_NSH_ATTR_MD1, > OVS_NSH_ATTR_MD2 > }; > > /* > * struct ovs_action_encap_nsh - %OVS_ACTION_ATTR_ENCAP_NSH > * > * @path_hdr: NSH service path id and service index. > * @flags: NSH header flags. > * @np: NSH next_protocol: Inner packet type. > * > * Followed by either %OVS_NSH_ATTR_MD1 or %OVS_NSH_ATTR_MD2 attribute. > */ > struct ovs_action_encap_nsh { > __be32 path_hdr; > uint8_t flags; > uint8_t np; > };
