Re: [dpdk-dev] [PATCH v7 1/5] lib/librte_ether: change function name of tunnel port config

["Lu, Wenzhuo" <wenzhuo.lu@intel.com>]

Hi Thomas,

> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> Sent: Wednesday, March 9, 2016 7:35 AM
> To: Lu, Wenzhuo
> Cc: dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v7 1/5] lib/librte_ether: change function name
> of tunnel port config
> 
> 2016-03-04 10:35, Wenzhuo Lu:
> > The names of function for tunnel port configuration are not accurate.
> > They're tunnel_add/del, better change them to tunnel_port_add/del.
> 
> As a lot of ethdev API, it is really badly documented.
> 
> Please explain why this renaming and let's try to reword the
> doxygen:
>  * Add UDP tunneling port of an Ethernet device for filtering a specific
>  * tunneling packet by UDP port number.
As we discussed before, these APIs only change the UDP port value of the tunnel. 
But according to their names, seems like they're trying to add/delete a whole tunnel.
The names don't tell us what the functions really do, so we want to change the names.

> 
> Please what are the values of
> struct rte_eth_udp_tunnel {
>     uint16_t udp_port;
>     uint8_t prot_type;
> };
> When I see an API struct without any comment, I feel it must be dropped.
I'm confused.  I don't do anything about this structure. You want me to add more comments for it?

> 
> By the way, it is yet another filtering API, so it must be totally reworked.
Not quite understand. I only try to change the name. If rework needed, could we do this with a new patch?

> 
> > As it may be an ABI change if change the names directly, the new
> > functions are added but not remove the old ones. The old ones will be
> > removed in the next release after an ABI change announcement.
> 
> Please make the announce in this patch.
Sure, I'll do that.

> 
> > --- a/lib/librte_ether/rte_ethdev.h
> > +++ b/lib/librte_ether/rte_ethdev.h
> > @@ -3403,6 +3415,9 @@ rte_eth_dev_rss_hash_conf_get(uint8_t port_id,
> > int  rte_eth_dev_udp_tunnel_add(uint8_t port_id,
> >  			   struct rte_eth_udp_tunnel *tunnel_udp);
> 
> You must deprecate this one and put a comment above with something like
> @see rte_eth_dev_udp_tunnel_port_add.
I'll add this. Thanks.

> 
> > +int
> > +rte_eth_dev_udp_tunnel_port_add(uint8_t port_id,
> > +				struct rte_eth_udp_tunnel *tunnel_udp);
> 
> You must move it below the doxygen comment.
OK. Honestly, I'd like to act like that. But seems the style is to put the function above the comment.(Don't know the reason.)
It'll be a little strange if I do something different.

> >
> >   /**
> >   * Detete UDP tunneling port configuration of Ethernet device @@
> > -3420,6 +3435,9 @@ rte_eth_dev_udp_tunnel_add(uint8_t port_id,  int
> > rte_eth_dev_udp_tunnel_delete(uint8_t port_id,
> >  			      struct rte_eth_udp_tunnel *tunnel_udp);
> > +int
> > +rte_eth_dev_udp_tunnel_port_delete(uint8_t port_id,
> > +				   struct rte_eth_udp_tunnel *tunnel_udp);
> 
> idem
> 
> > --- a/lib/librte_ether/rte_ether_version.map
> > +++ b/lib/librte_ether/rte_ether_version.map
> > @@ -114,6 +114,8 @@ DPDK_2.2 {
> >  	rte_eth_tx_queue_setup;
> >  	rte_eth_xstats_get;
> >  	rte_eth_xstats_reset;
> > +	rte_eth_dev_udp_tunnel_port_add;
> > +	rte_eth_dev_udp_tunnel_port_delete;
> >
> >  	local: *;
> >  };
> 
> Panu already made a comment about adding a new section for 16.04.
Thanks for the info. Let me follow it.