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.
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.
By the way, it is yet another filtering API, so it must be totally reworked.
> 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.
> --- 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.
> +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.
>
> /**
> * 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.
