Hi Ferruh, <snip> > >>>>> 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.zh...@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? > > > > This seems to be the existing naming convention. > > > >>> > >>> 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 > > > > I don't think changing the name convention at this point is a good idea. > > I am not suggesting changing existing ones, for the sake compatibility. > But that can also be an option, since these are PMD specific API, I expect > usage will be limited and these does not carry as high standard as library > APIs.
These API's are already in use with users. > > It would be better to remain consistent with the existing naming > convention. > > Existing i40e ones added this way to be compatible with existing ixgbe ones. > But I don't think we need to follow old usage with new APIs. > > > Otherwise both naming conventions will exist for the rte_pmd_i40e_* > API's. > > It can be for a while, later all can be fixed. If you think proposed > convention is > not better, that is something else. I don't see anything to be gained by using two naming conventions side by side. I don't have a strong view on which name convention is better, however the existing naming convention is what is in use with users at present. It is also the naming convention used librte_ether. Changing API naming conventions is something that should probably be discussed in a separate thread, as it can have a wider impact than the i40e and ixgbe PMD's. > > > > > >>>>> rte_pmd_i40e_vf_vlan_untag_drop_set(), and perhaps "set" can be > >>> removed? > >>>>> > >>>>>> + > >>>>>> #endif /* _PMD_I40E_H_ */ > >>>>>> > >>>> Regards, Bernard.
