> -----Original Message----- > From: Ferruh Yigit <ferruh.yi...@intel.com> > Sent: Friday 8 January 2021 14:27 > To: Kinsella, Ray <ray.kinse...@intel.com>; Thomas Monjalon > <tho...@monjalon.net>; Guo, Jia <jia....@intel.com>; Zhang, Qi Z > <qi.z.zh...@intel.com> > Cc: Wu, Jingjing <jingjing...@intel.com>; Yang, Qiming > <qiming.y...@intel.com>; Wang, Haiyue <haiyue.w...@intel.com>; > dev@dpdk.org; andrew.rybche...@oktetlabs.ru; or...@nvidia.com; > getel...@nvidia.com; Dodji Seketeli <do...@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 <tho...@monjalon.net> > >> Sent: Friday 8 January 2021 10:24 > >> To: Guo, Jia <jia....@intel.com>; Zhang, Qi Z > <qi.z.zh...@intel.com>; > >> Yigit, Ferruh <ferruh.yi...@intel.com> > >> Cc: Wu, Jingjing <jingjing...@intel.com>; Yang, Qiming > >> <qiming.y...@intel.com>; Wang, Haiyue <haiyue.w...@intel.com>; > >> dev@dpdk.org; andrew.rybche...@oktetlabs.ru; or...@nvidia.com; > >> getel...@nvidia.com; Dodji Seketeli <do...@redhat.com>; Kinsella, > Ray > >> <ray.kinse...@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 <tho...@monjalon.net> > >>>>>> 07/01/2021 10:32, Guo, Jia: > >>>>>>> From: Thomas Monjalon <tho...@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.