[2/2] ethdev: make rte_eth_is_valid_owner_id return bool

Message ID 20180816224409.5719-3-stephen@networkplumber.org (mailing list archive)
State Superseded, archived
Delegated to: Ferruh Yigit
Headers
Series ethdev: minor ownership changes |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/Intel-compilation success Compilation OK

Commit Message

Stephen Hemminger Aug. 16, 2018, 10:44 p.m. UTC
  Function is boolean so use that.

Signed-off-by: Stephen Hemminger <sthemmin@microsoft.com>
---
 lib/librte_ethdev/rte_ethdev.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)
  

Comments

Matan Azrad Aug. 21, 2018, 10:20 a.m. UTC | #1
From: Stephen Hemminger
> Function is boolean so use that.

Ethdev is not using bool type, see also:
rte_eth_dev_is_valid_port
rte_eth_dev_is_removed
rte_eth_dev_pool_ops_supported

I think it should be a full solution to all.
 
> Signed-off-by: Stephen Hemminger <sthemmin@microsoft.com>
> ---
>  lib/librte_ethdev/rte_ethdev.c | 8 +++-----
>  1 file changed, 3 insertions(+), 5 deletions(-)
> 
> diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
> index f09bf8bc8b01..f0336736b7c1 100644
> --- a/lib/librte_ethdev/rte_ethdev.c
> +++ b/lib/librte_ethdev/rte_ethdev.c
> @@ -389,13 +389,11 @@ rte_eth_dev_is_valid_port(uint16_t port_id)
>  		return 1;
>  }
> 
> -static int
> +static bool
>  rte_eth_is_valid_owner_id(uint64_t owner_id)  {
> -	if (owner_id == RTE_ETH_DEV_NO_OWNER ||
> -	    rte_eth_dev_shared_data->next_owner_id <= owner_id)
> -		return 0;
> -	return 1;
> +	return !(owner_id == RTE_ETH_DEV_NO_OWNER ||
> +		 rte_eth_dev_shared_data->next_owner_id <= owner_id);
>  }
> 
>  uint64_t
> --
> 2.18.0
  
Stephen Hemminger Aug. 21, 2018, 3:06 p.m. UTC | #2
On Tue, 21 Aug 2018 10:20:43 +0000
Matan Azrad <matan@mellanox.com> wrote:

> From: Stephen Hemminger
> > Function is boolean so use that.  
> 
> Ethdev is not using bool type, see also:
> rte_eth_dev_is_valid_port
> rte_eth_dev_is_removed
> rte_eth_dev_pool_ops_supported
> 
> I think it should be a full solution to all.
>  
> > Signed-off-by: Stephen Hemminger <sthemmin@microsoft.com>

I didn't want change type of visible (exported by ABI) functions.
  
Matan Azrad Aug. 21, 2018, 3:48 p.m. UTC | #3
Hi

From: Stephen Hemminger
> On Tue, 21 Aug 2018 10:20:43 +0000
> Matan Azrad <matan@mellanox.com> wrote:
> 
> > From: Stephen Hemminger
> > > Function is boolean so use that.
> >
> > Ethdev is not using bool type, see also:
> > rte_eth_dev_is_valid_port
> > rte_eth_dev_is_removed
> > rte_eth_dev_pool_ops_supported
> >
> > I think it should be a full solution to all.
> >
> > > Signed-off-by: Stephen Hemminger <sthemmin@microsoft.com>
> 
> I didn't want change type of visible (exported by ABI) functions.
> 
Since ethdev now is not using bool type I think it's better not to change it only for this API.
  
Stephen Hemminger Aug. 21, 2018, 6:31 p.m. UTC | #4
On Tue, 21 Aug 2018 15:48:19 +0000
Matan Azrad <matan@mellanox.com> wrote:

> Hi
> 
> From: Stephen Hemminger
> > On Tue, 21 Aug 2018 10:20:43 +0000
> > Matan Azrad <matan@mellanox.com> wrote:
> >   
> > > From: Stephen Hemminger  
> > > > Function is boolean so use that.  
> > >
> > > Ethdev is not using bool type, see also:
> > > rte_eth_dev_is_valid_port
> > > rte_eth_dev_is_removed
> > > rte_eth_dev_pool_ops_supported
> > >
> > > I think it should be a full solution to all.
> > >  
> > > > Signed-off-by: Stephen Hemminger <sthemmin@microsoft.com>  
> > 
> > I didn't want change type of visible (exported by ABI) functions.
> >   
> Since ethdev now is not using bool type I think it's better not to change it only for this API.

I hate to pick nits but there is already a bool usage in internal
function (static) in ethdev.


static bool
is_allocated(const struct rte_eth_dev *ethdev)
{
	return ethdev->data->name[0] != '\0';
}

Using bool functions doesn't really generate different code. It is is more
about using modern C conventions.
  
Matan Azrad Aug. 26, 2018, 7:49 a.m. UTC | #5
From: Stephen Hemminger 
> On Tue, 21 Aug 2018 15:48:19 +0000
> Matan Azrad <matan@mellanox.com> wrote:
> 
> > Hi
> >
> > From: Stephen Hemminger
> > > On Tue, 21 Aug 2018 10:20:43 +0000
> > > Matan Azrad <matan@mellanox.com> wrote:
> > >
> > > > From: Stephen Hemminger
> > > > > Function is boolean so use that.
> > > >
> > > > Ethdev is not using bool type, see also:
> > > > rte_eth_dev_is_valid_port
> > > > rte_eth_dev_is_removed
> > > > rte_eth_dev_pool_ops_supported
> > > >
> > > > I think it should be a full solution to all.
> > > >
> > > > > Signed-off-by: Stephen Hemminger <sthemmin@microsoft.com>
> > >
> > > I didn't want change type of visible (exported by ABI) functions.
> > >
> > Since ethdev now is not using bool type I think it's better not to change it
> only for this API.
> 
> I hate to pick nits but there is already a bool usage in internal function (static)
> in ethdev.
> 
> 
> static bool
> is_allocated(const struct rte_eth_dev *ethdev) {
> 	return ethdev->data->name[0] != '\0';
> }
> 
> Using bool functions doesn't really generate different code. It is is more
> about using modern C conventions.

Agree, but I think it should be the same API at least as  rte_eth_dev_is_valid_port, just for ethdev convention.

Let's give to the maintainer the decision.
  

Patch

diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
index f09bf8bc8b01..f0336736b7c1 100644
--- a/lib/librte_ethdev/rte_ethdev.c
+++ b/lib/librte_ethdev/rte_ethdev.c
@@ -389,13 +389,11 @@  rte_eth_dev_is_valid_port(uint16_t port_id)
 		return 1;
 }
 
-static int
+static bool
 rte_eth_is_valid_owner_id(uint64_t owner_id)
 {
-	if (owner_id == RTE_ETH_DEV_NO_OWNER ||
-	    rte_eth_dev_shared_data->next_owner_id <= owner_id)
-		return 0;
-	return 1;
+	return !(owner_id == RTE_ETH_DEV_NO_OWNER ||
+		 rte_eth_dev_shared_data->next_owner_id <= owner_id);
 }
 
 uint64_t