Re: [PATCH v3 1/6] ethdev: add devop to check removal status

[Matan Azrad <matan@mellanox.com>]

Hi

> -----Original Message-----
> From: Gaëtan Rivet [mailto:gaetan.rivet@6wind.com]
> Sent: Wednesday, December 20, 2017 12:13 AM
> To: Thomas Monjalon <thomas@monjalon.net>
> Cc: Matan Azrad <matan@mellanox.com>; Stephen Hemminger
> <stephen@networkplumber.org>; Adrien Mazarguil
> <adrien.mazarguil@6wind.com>; dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v3 1/6] ethdev: add devop to check removal
> status
> 
> On Tue, Dec 19, 2017 at 09:51:10PM +0100, Thomas Monjalon wrote:
> > 19/12/2017 18:24, Matan Azrad:
> > > HI
> > >
> > > > -----Original Message-----
> > > > From: Stephen Hemminger [mailto:stephen@networkplumber.org]
> > > > Sent: Tuesday, December 19, 2017 7:20 PM
> > > > To: Matan Azrad <matan@mellanox.com>
> > > > Cc: Adrien Mazarguil <adrien.mazarguil@6wind.com>; Thomas
> Monjalon
> > > > <thomas@monjalon.net>; Gaetan Rivet <gaetan.rivet@6wind.com>;
> > > > dev@dpdk.org
> > > > Subject: Re: [dpdk-dev] [PATCH v3 1/6] ethdev: add devop to check
> > > > removal status
> > > >
> > > > On Tue, 19 Dec 2017 17:10:10 +0000 Matan Azrad
> > > > <matan@mellanox.com> wrote:
> > > >
> > > > >  int
> > > > > +rte_eth_dev_is_removed(uint16_t port_id) {
> > > > > +	struct rte_eth_dev *dev;
> > > > > +	int ret;
> > > > > +
> > > > > +	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, 0);
> > > > > +
> > > > > +	dev = &rte_eth_devices[port_id];
> > > > > +
> > > > > +	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops-
> >is_removed, 0);
> > > > > +
> > > > > +	if (dev->state == RTE_ETH_DEV_REMOVED)
> > > > > +		return 1;
> > > > > +
> > > > > +	ret = dev->dev_ops->is_removed(dev);
> > > > > +	if (ret != 0)
> > > > > +		dev->state = RTE_ETH_DEV_REMOVED;
> > > > > +
> > > > > +	return ret;
> > > > > +}
> > > > > +
> > > >
> > > > This looks good.
> > > > May be a candidate to use bool instead of int for return value?
> > >
> > > Yes, I thought about it but didn't see any precedence for bool usage in
> ethdev APIs.
> > > Guys, what do you think?
> >
> > I think this function can return error, isn't it?
> > (look at macros *_OR_ERR_RET used in the function)
> >
> 
> But those macros are used to return 0.
> 
> While I think I see a logic behind it, I think it is surprising the API user, which is
> not ideal.
> 

The logic behind it is that "is" semantic is question which expects to yes\no answer.
Therefore, user who uses this API just expects to either True or False return value and doesn't need to check more errors options like -ENODEV or -ENOTSUP.
I decided that the return value will be only 0 or 1 to make it easier to user:
Removed - 1,
Present - 0,
No support - it makes sense that PMD which doesn't implement "is_removed" devop means that its underlying devices are not removable so they are always present and this function should return '0' for it.
No port - I think that '0' here will be better that '1'.

Stephen suggestion to replace '0' and '1' by 'false' and 'true' makes sense but I decided not to do it like this because of the next ideas:
1. No precedence for bool value in ethdev APIs.
2. Maybe it will be problematic to use *OR_ERR_RET defines to return bool value. 

I hope that this explanation helps you.

Thanks, 
Matan.
   
  


   
> --
> Gaëtan Rivet
> 6WIND