Re: [dpdk-dev] [dpdk-dev v2 1/2] ethdev: add new tunnel type for ecpri

["Kinsella, Ray" <ray.kinsella@intel.com>]

> -----Original Message-----
> From: Ferruh Yigit <ferruh.yigit@intel.com>
> Sent: Friday 8 January 2021 14:27
> To: Kinsella, Ray <ray.kinsella@intel.com>; Thomas Monjalon
> <thomas@monjalon.net>; Guo, Jia <jia.guo@intel.com>; Zhang, Qi Z
> <qi.z.zhang@intel.com>
> Cc: Wu, Jingjing <jingjing.wu@intel.com>; Yang, Qiming
> <qiming.yang@intel.com>; Wang, Haiyue <haiyue.wang@intel.com>;
> dev@dpdk.org; andrew.rybchenko@oktetlabs.ru; orika@nvidia.com;
> getelson@nvidia.com; Dodji Seketeli <dodji@redhat.com>
> Subject: Re: [dpdk-dev] [dpdk-dev v2 1/2] ethdev: add new tunnel type
> for ecpri
> 
> On 1/8/2021 12:38 PM, Kinsella, Ray wrote:
> >
> >
> >> -----Original Message-----
> >> From: Thomas Monjalon <thomas@monjalon.net>
> >> Sent: Friday 8 January 2021 10:24
> >> To: Guo, Jia <jia.guo@intel.com>; Zhang, Qi Z
> <qi.z.zhang@intel.com>;
> >> Yigit, Ferruh <ferruh.yigit@intel.com>
> >> Cc: Wu, Jingjing <jingjing.wu@intel.com>; Yang, Qiming
> >> <qiming.yang@intel.com>; Wang, Haiyue <haiyue.wang@intel.com>;
> >> dev@dpdk.org; andrew.rybchenko@oktetlabs.ru; orika@nvidia.com;
> >> getelson@nvidia.com; Dodji Seketeli <dodji@redhat.com>; Kinsella,
> Ray
> >> <ray.kinsella@intel.com>
> >> Subject: Re: [dpdk-dev] [dpdk-dev v2 1/2] ethdev: add new tunnel
> type
> >> for ecpri
> >>
> >> 08/01/2021 10:22, Ferruh Yigit:
> >>> On 1/7/2021 1:33 PM, Thomas Monjalon wrote:
> >>>> 07/01/2021 13:47, Zhang, Qi Z:
> >>>>> From: Thomas Monjalon <thomas@monjalon.net>
> >>>>>> 07/01/2021 10:32, Guo, Jia:
> >>>>>>> From: Thomas Monjalon <thomas@monjalon.net>
> >>>>>>>> Sorry, it is a nack.
> >>>>>>>> BTW, it is probably breaking the ABI because of
> >> RTE_TUNNEL_TYPE_MAX.
> >>>>>
> >>>>> Yes that may break the ABI but fortunately the checking-abi-
> >> compatibility tool shows negative :) , thanks Ferruh' s guide.
> >>>>> https://github.com/ferruhy/dpdk/actions/runs/468859673
> >>>>
> >>>> That's very strange. An enum value is changed.
> >>>> Why it is not flagged by libabigail?
> >>>
> >>> As long as the enum values not sent to the application and kept
> >> within
> >>> the library, changing their values shouldn't be problem.
> >>
> >> But RTE_TUNNEL_TYPE_MAX is part of lib/librte_ethdev/rte_ethdev.h so
> >> it is exposed to the application.
> >> I think it is a case of ABI breakage.
> >>
> >
> > Really a lot depends on context, Thomas is right it is hard to
> predict how these _MAX values are used.
> >
> > We have seen cases in the past where _MAX enumeration values have
> been used to size arrays the like - I don't immediately see that issue
> here. My understanding is that the only consumer of this enumeration is
> rte_eth_dev_udp_tunnel_port_add and rte_eth_dev_udp_tunnel_port_delete,
> right? On face value, impact looks negligible.
> >
> > I will take a look at why libabigail doesn't complain.

So I spent some time looking a bit closer at why libabigail didn't complain,
I am summarizing it is because there is no symbol that obviously uses enum rte_eth_tunnel_type.
The prot_type field in rte_eth_udp_tunnel is declared as a uint8_t, not as enum rte_eth_tunnel_type.

Is there a particular reason an enumerated field would be declared as unsigned int instead?

/**
 * UDP tunneling configuration.
 * Used to config the UDP port for a type of tunnel.
 * NICs need the UDP port to identify the tunnel type.
 * Normally a type of tunnel has a default UDP port, this structure can be used
 * in case if the users want to change or support more UDP port.
 */
struct rte_eth_udp_tunnel {
        uint16_t udp_port; /**< UDP port used for the tunnel. */
        uint8_t prot_type; /**< Tunnel type. Defined in rte_eth_tunnel_type. */
};

> 
> Application can use the enum, including MAX as they desire, we can't
> really assume anything there.
> 
> In previous case, library was providing an enum value back to
> application. And the problem was application can use those values
> blindly and new unexpected values may cause trouble.
> 
> For this case, even the application create a table with
> RTE_TUNNEL_TYPE_MAX size, library is not sending any type of this enum
> to application to cause any problem, at least abigail seems not able to
> finding any instance of it.

I agree - I think this has marginal risk.