Re: [PATCH 1/2] net/i40e: enable VF untag drop

["Zhang, Qi Z" <qi.z.zhang@intel.com>]

> -----Original Message-----
> From: Yigit, Ferruh
> Sent: Tuesday, March 14, 2017 9:30 PM
> To: Zhang, Qi Z <qi.z.zhang@intel.com>; Wu, Jingjing
> <jingjing.wu@intel.com>; Zhang, Helin <helin.zhang@intel.com>
> Cc: dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH 1/2] net/i40e: enable VF untag drop
> 
> On 3/9/2017 3:24 AM, Zhang, Qi Z wrote:
> > Hi Ferruh:
> >
> >> -----Original Message-----
> >> From: Yigit, Ferruh
> >> Sent: Tuesday, March 7, 2017 6:51 PM
> >> To: Zhang, Qi Z <qi.z.zhang@intel.com>; Wu, Jingjing
> >> <jingjing.wu@intel.com>; Zhang, Helin <helin.zhang@intel.com>
> >> Cc: dev@dpdk.org
> >> Subject: Re: [dpdk-dev] [PATCH 1/2] net/i40e: enable VF untag drop
> >>
> >> On 3/3/2017 1:59 AM, Qi Zhang wrote:
> >>> Add a new private API to support the untag drop enable/disable for
> >>> specific VF.
> >>>
> >>> Signed-off-by: Qi Zhang <qi.z.zhang@intel.com>
> >>> ---
> >>>  drivers/net/i40e/i40e_ethdev.c  | 49
> >>> +++++++++++++++++++++++++++++++++++++++++
> >>>  drivers/net/i40e/rte_pmd_i40e.h | 18 +++++++++++++++
> >>
> >> Shared library is giving build error because of API is missing in
> >> *version.map file
> >>
> >>>  2 files changed, 67 insertions(+)
> >>>
> >>
> >> <...>
> >>
> >>> diff --git a/drivers/net/i40e/rte_pmd_i40e.h
> >>> b/drivers/net/i40e/rte_pmd_i40e.h index a0ad88c..895e2cc 100644
> >>> --- a/drivers/net/i40e/rte_pmd_i40e.h
> >>> +++ b/drivers/net/i40e/rte_pmd_i40e.h
> >>> @@ -332,4 +332,22 @@ int rte_pmd_i40e_get_vf_stats(uint8_t port,
> >>> int rte_pmd_i40e_reset_vf_stats(uint8_t port,
> >>>  				uint16_t vf_id);
> >>>
> >>> +/**
> >>> + * Enable/Disable VF untag drop
> >>> + *
> >>> + * @param port
> >>> + *    The port identifier of the Ethernet device.
> >>> + * @param vf_id
> >>> + *    VF on witch to enable/disable
> >>> + * @param on
> >>> + *    Enable or Disable
> >>> + * @retura
> >>
> >> @return
> >>
> >>> + *  - (0) if successful.
> >>> + *  -(-ENODEVE) if *port* invalid
> >>> + *  -(-EINVAL) if bad parameter.
> >>> + */
> >>> +int rte_pmd_i40e_set_vf_vlan_untag_drop(uint8_t port,
> >>> +					uint16_t vf_id,
> >>> +					uint8_t on);
> >>
> >> As discussed previously, I believe it is good to keep following syntax in
> API:	
> >> <name_space>_<object>_<action>, for this API it becomes:
> > I think, current naming rule is <name_space>_<action>_<object> right?
> 
> Overall, I am not aware of any defined naming rule, I am for defining one.
> 
> > See below
> > 		rte_pmd_i40e_set_vf_vlan_anti_spoof;
> >         rte_pmd_i40e_set_vf_vlan_filter;
> >         rte_pmd_i40e_set_vf_vlan_insert;
> >         rte_pmd_i40e_set_vf_vlan_stripq;
> >         rte_pmd_i40e_set_vf_vlan_tag;
> > so what's wrong with this 
> 
> This breaks hierarchical approach, if you think API name as tree. Easier to
> see this when you sort the APIs, ns_set_x, ns_reset_x, ns_del_x will spread
> to different locations.
I agree with your point, I had thought your concern is only about this patch, but actually it's not.
> 
> This looks OK when you work on one type of object already, but with all APIs
> in concern, I believe object based grouping is better than action based
> grouping.

> 
> And why do you think above one is better? Again, as long as one is agreed on,
I don't, sorry for make you misunderstand
> 
> >>
> >> rte_pmd_i40e_vf_vlan_untag_drop_set(), and perhaps "set" can be
> removed?
> >>
> >>> +
> >>>  #endif /* _PMD_I40E_H_ */
> >>>
> >