[dpdk-dev,v4,2/2] ethdev: add sanity checks to functions

  The functions rte_eth_rx_queue_count and rte_eth_descriptor_done are
supported by very few PMDs. Therefore, it is best to check for support
for the functions in the ethdev library, so as to avoid run-time crashes
at run-time if the application goes to use those APIs. Similarly, the
port parameter should also be checked for validity.

Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>

---
 lib/librte_ether/rte_ethdev.h | 15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)
  

On Tue, 17 Nov 2015 12:21:07 +0000
Bruce Richardson <bruce.richardson@intel.com> wrote:

> The functions rte_eth_rx_queue_count and rte_eth_descriptor_done are
> supported by very few PMDs. Therefore, it is best to check for support
> for the functions in the ethdev library, so as to avoid run-time crashes
> at run-time if the application goes to use those APIs. Similarly, the
> port parameter should also be checked for validity.
> 
> Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
> 
> ---
>  lib/librte_ether/rte_ethdev.h | 15 +++++++--------
>  1 file changed, 7 insertions(+), 8 deletions(-)
> 
> diff --git a/lib/librte_ether/rte_ethdev.h b/lib/librte_ether/rte_ethdev.h
> index a00cd46..028be59 100644
> --- a/lib/librte_ether/rte_ethdev.h
> +++ b/lib/librte_ether/rte_ethdev.h
> @@ -2533,16 +2533,16 @@ rte_eth_rx_burst(uint8_t port_id, uint16_t queue_id,
>   * @param queue_id
>   *  The queue id on the specific port.
>   * @return
> - *  The number of used descriptors in the specific queue.
> + *  The number of used descriptors in the specific queue, or:
> + *     (-EINVAL) if *port_id* is invalid
> + *     (-ENOTSUP) if the device does not support this function
>   */
> -static inline uint32_t
> +static inline int
>  rte_eth_rx_queue_count(uint8_t port_id, uint16_t queue_id)
>  {
>  	struct rte_eth_dev *dev = &rte_eth_devices[port_id];
> -#ifdef RTE_LIBRTE_ETHDEV_DEBUG
> -	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, 0);
> -	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->rx_queue_count, 0);
> -#endif
> +	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -EINVAL);
> +	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->rx_queue_count, -ENOTSUP);
>          return (*dev->dev_ops->rx_queue_count)(dev, queue_id);
>  }
>  
> @@ -2559,15 +2559,14 @@ rte_eth_rx_queue_count(uint8_t port_id, uint16_t queue_id)
>   *  - (1) if the specific DD bit is set.
>   *  - (0) if the specific DD bit is not set.
>   *  - (-ENODEV) if *port_id* invalid.
> + *  - (-ENOTSUP) if the device does not support this function
>   */
>  static inline int
>  rte_eth_rx_descriptor_done(uint8_t port_id, uint16_t queue_id, uint16_t offset)
>  {
>  	struct rte_eth_dev *dev = &rte_eth_devices[port_id];
> -#ifdef RTE_LIBRTE_ETHDEV_DEBUG
>  	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
>  	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->rx_descriptor_done, -ENOTSUP);
> -#endif
>  	return (*dev->dev_ops->rx_descriptor_done)( \
>  		dev->data->rx_queues[queue_id], offset);
>  }

This breaks ABI since older application built with debug will try
and find the shared library entry for the routine.
  

On Tue, Nov 17, 2015 at 07:53:09AM -0800, Stephen Hemminger wrote:
> On Tue, 17 Nov 2015 12:21:07 +0000
> Bruce Richardson <bruce.richardson@intel.com> wrote:
> 
> > The functions rte_eth_rx_queue_count and rte_eth_descriptor_done are
> > supported by very few PMDs. Therefore, it is best to check for support
> > for the functions in the ethdev library, so as to avoid run-time crashes
> > at run-time if the application goes to use those APIs. Similarly, the
> > port parameter should also be checked for validity.
> > 
> > Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
> > 
> > ---
> >  lib/librte_ether/rte_ethdev.h | 15 +++++++--------
> >  1 file changed, 7 insertions(+), 8 deletions(-)
> > 
> > diff --git a/lib/librte_ether/rte_ethdev.h b/lib/librte_ether/rte_ethdev.h
> > index a00cd46..028be59 100644
> > --- a/lib/librte_ether/rte_ethdev.h
> > +++ b/lib/librte_ether/rte_ethdev.h
> > @@ -2533,16 +2533,16 @@ rte_eth_rx_burst(uint8_t port_id, uint16_t queue_id,
> >   * @param queue_id
> >   *  The queue id on the specific port.
> >   * @return
> > - *  The number of used descriptors in the specific queue.
> > + *  The number of used descriptors in the specific queue, or:
> > + *     (-EINVAL) if *port_id* is invalid
> > + *     (-ENOTSUP) if the device does not support this function
> >   */
> > -static inline uint32_t
> > +static inline int
> >  rte_eth_rx_queue_count(uint8_t port_id, uint16_t queue_id)
> >  {
> >  	struct rte_eth_dev *dev = &rte_eth_devices[port_id];
> > -#ifdef RTE_LIBRTE_ETHDEV_DEBUG
> > -	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, 0);
> > -	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->rx_queue_count, 0);
> > -#endif
> > +	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -EINVAL);
> > +	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->rx_queue_count, -ENOTSUP);
> >          return (*dev->dev_ops->rx_queue_count)(dev, queue_id);
> >  }
> >  
> > @@ -2559,15 +2559,14 @@ rte_eth_rx_queue_count(uint8_t port_id, uint16_t queue_id)
> >   *  - (1) if the specific DD bit is set.
> >   *  - (0) if the specific DD bit is not set.
> >   *  - (-ENODEV) if *port_id* invalid.
> > + *  - (-ENOTSUP) if the device does not support this function
> >   */
> >  static inline int
> >  rte_eth_rx_descriptor_done(uint8_t port_id, uint16_t queue_id, uint16_t offset)
> >  {
> >  	struct rte_eth_dev *dev = &rte_eth_devices[port_id];
> > -#ifdef RTE_LIBRTE_ETHDEV_DEBUG
> >  	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
> >  	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->rx_descriptor_done, -ENOTSUP);
> > -#endif
> >  	return (*dev->dev_ops->rx_descriptor_done)( \
> >  		dev->data->rx_queues[queue_id], offset);
> >  }
> 
> This breaks ABI since older application built with debug will try
> and find the shared library entry for the routine.

Ok, so assuming we care about the ABI for debug builds, is it enough to just
push a patch with a deprecation notice for this for 2.2, or do I need to see about
doing a new patchset with the NEXT_ABI macros included in it? My preference is
obviously for the former.

/Bruce
  

2015-11-24 14:56, Bruce Richardson:
> On Tue, Nov 17, 2015 at 07:53:09AM -0800, Stephen Hemminger wrote:
> > On Tue, 17 Nov 2015 12:21:07 +0000
> > Bruce Richardson <bruce.richardson@intel.com> wrote:
> > > -static inline uint32_t
> > > +static inline int

Are we talking about this change only?
Or the move in the first patch from .c to .h?

[...]
> > This breaks ABI since older application built with debug will try
> > and find the shared library entry for the routine.
> 
> Ok, so assuming we care about the ABI for debug builds,

The return type is not only for debug build?

> is it enough to just push a patch with a deprecation notice for this for 2.2,

The ABI is already broken for ethdev in 2.2.
So the symbol move should not hurt more.
And the API change (return type) should not be a big deal,
but at least an API change notification is required in the release notes.
Other opinion?

> or do I need to see about doing a new patchset with the NEXT_ABI macros
> included in it? My preference is obviously for the former.

No NEXT_ABI is required when ABI is already broken IMHO.
  

On Tue, Nov 24, 2015 at 04:29:12PM +0100, Thomas Monjalon wrote:
> 2015-11-24 14:56, Bruce Richardson:
> > On Tue, Nov 17, 2015 at 07:53:09AM -0800, Stephen Hemminger wrote:
> > > On Tue, 17 Nov 2015 12:21:07 +0000
> > > Bruce Richardson <bruce.richardson@intel.com> wrote:
> > > > -static inline uint32_t
> > > > +static inline int
> 
> Are we talking about this change only?
> Or the move in the first patch from .c to .h?
> 

The move is the ABI breaker.

> [...]
> > > This breaks ABI since older application built with debug will try
> > > and find the shared library entry for the routine.
> > 
> > Ok, so assuming we care about the ABI for debug builds,
> 
> The return type is not only for debug build?
> 
> > is it enough to just push a patch with a deprecation notice for this for 2.2,
> 
> The ABI is already broken for ethdev in 2.2.
> So the symbol move should not hurt more.
> And the API change (return type) should not be a big deal,
> but at least an API change notification is required in the release notes.
> Other opinion?

Ok, it makes sense.

> 
> > or do I need to see about doing a new patchset with the NEXT_ABI macros
> > included in it? My preference is obviously for the former.
> 
> No NEXT_ABI is required when ABI is already broken IMHO.

If ethdev ABI is already broken, then sure, this additional break for debug
build is no big deal, I think.

I can do a respin of these two patches to include an API note for release notes.
However, I see now that I also need to remove the functions from the map file.
I could do with some help to make sure I do this correctly though. Reading through
the doc on ABI versionning, it looks like I should completely move all existing
functions from the existing release versions and move them to a new 2.2 section,
dropping the four now-inline functions along the way. Is this the correct thing
to do?

/Bruce
  

2015-11-24 15:45, Bruce Richardson:
> On Tue, Nov 24, 2015 at 04:29:12PM +0100, Thomas Monjalon wrote:
> > 2015-11-24 14:56, Bruce Richardson:
> > > On Tue, Nov 17, 2015 at 07:53:09AM -0800, Stephen Hemminger wrote:
> > > > On Tue, 17 Nov 2015 12:21:07 +0000
> > > > Bruce Richardson <bruce.richardson@intel.com> wrote:
> > > > > -static inline uint32_t
> > > > > +static inline int
> > 
> > Are we talking about this change only?
> > Or the move in the first patch from .c to .h?
> > 
> 
> The move is the ABI breaker.
> 
> > [...]
> > > > This breaks ABI since older application built with debug will try
> > > > and find the shared library entry for the routine.
> > > 
> > > Ok, so assuming we care about the ABI for debug builds,
> > 
> > The return type is not only for debug build?
> > 
> > > is it enough to just push a patch with a deprecation notice for this for 2.2,
> > 
> > The ABI is already broken for ethdev in 2.2.
> > So the symbol move should not hurt more.
> > And the API change (return type) should not be a big deal,
> > but at least an API change notification is required in the release notes.
> > Other opinion?
> 
> Ok, it makes sense.
> 
> > 
> > > or do I need to see about doing a new patchset with the NEXT_ABI macros
> > > included in it? My preference is obviously for the former.
> > 
> > No NEXT_ABI is required when ABI is already broken IMHO.
> 
> If ethdev ABI is already broken, then sure, this additional break for debug
> build is no big deal, I think.
> 
> I can do a respin of these two patches to include an API note for release notes.
> However, I see now that I also need to remove the functions from the map file.
> I could do with some help to make sure I do this correctly though. Reading through
> the doc on ABI versionning, it looks like I should completely move all existing
> functions from the existing release versions and move them to a new 2.2 section,
> dropping the four now-inline functions along the way. Is this the correct thing
> to do?

I think yes.
Removing some symbols means rewriting the symbol map from scratch.
But we never did it yet.