Re: [dpdk-dev] [PATCH v5 01/11] ethdev: add extensions attributes to IPv6 item

[Dekel Peled <dekelp@nvidia.com>]

Thanks, PSB.

> -----Original Message-----
> From: Thomas Monjalon <thomas@monjalon.net>
> Sent: Monday, October 12, 2020 11:42 PM
> To: Ori Kam <orika@nvidia.com>; Dekel Peled <dekelp@nvidia.com>
> Cc: ferruh.yigit@intel.com; arybchenko@solarflare.com;
> konstantin.ananyev@intel.com; olivier.matz@6wind.com;
> wenzhuo.lu@intel.com; beilei.xing@intel.com;
> bernard.iremonger@intel.com; Matan Azrad <matan@nvidia.com>; Shahaf
> Shuler <shahafs@nvidia.com>; Slava Ovsiienko <viacheslavo@nvidia.com>;
> dev@dpdk.org; Asaf Penso <asafp@nvidia.com>
> Subject: Re: [dpdk-dev] [PATCH v5 01/11] ethdev: add extensions attributes
> to IPv6 item
> 
> 12/10/2020 12:43, Dekel Peled:
> > - * Note: IPv6 options are handled by dedicated pattern items, see
> > - * RTE_FLOW_ITEM_TYPE_IPV6_EXT.
> > + * Dedicated flags indicate existence of specific extension headers.
> > + * Every type of extension header can use a dedicated pattern item,
> > + or
> > + * the generic item RTE_FLOW_ITEM_TYPE_IPV6_EXT.
> 
> I don't understand this last sentence.

I'll rephrase.

> 
> >   */
> >  struct rte_flow_item_ipv6 {
> >  	struct rte_ipv6_hdr hdr; /**< IPv6 header definition. */
> > +	uint32_t hop_ext_exist:1;
> > +	/**< Hop-by-Hop Options extension header exists. */
> > +	uint32_t rout_ext_exist:1;
> 
> "rout" looks weird. Would be "route" appropriate?

I'll change to "route".

> 
> > +	/**< Routing extension header exists. */
> > +	uint32_t frag_ext_exist:1;
> > +	/**< Fragment extension header exists. */
> > +	uint32_t auth_ext_exist:1;
> > +	/**< Authentication extension header exists. */
> > +	uint32_t esp_ext_exist:1;
> > +	/**< Encapsulation Security Payload extension header exists. */
> > +	uint32_t dest_ext_exist:1;
> > +	/**< Destination Options extension header exists. */
> > +	uint32_t mobil_ext_exist:1;
> > +	/**< Mobility extension header exists. */
> > +	uint32_t hip_ext_exist:1;
> > +	/**< Host Identity Protocol extension header exists. */
> > +	uint32_t shim6_ext_exist:1;
> > +	/**< Shim6 Protocol extension header exists. */
> 
> About the field names, the "_exist" suffix is pretty clear, but without being
> able to say why, I feel it is a strange name.
> I was thinking about renaming the fields with a "has_" prefix.
> Does it look better?

I'm afraid a "has_" prefix doesn't look appropriate IMHO.
I still prefer the "_exist" suffix.

>