[dpdk-dev] [PATCH 0/1] eal: return true or false from lcore role check function

[dpdk-dev] [PATCH 0/1] eal: return true or false from lcore role check function

[Erik Gabriel Carrillo]

This patch updates the rte_lcore_has_role() function so that it returns
true/false instead of success/failure, which seems more intuitive given the
function name.

There is currently one call site for rte_lcore_has_role() and the caller uses
it (incorrectly) as though it returns true/false.  The call site can be updated
to use the existing definition instead, but this change can avoid similar
misuses in the future.

Erik Gabriel Carrillo (1):
  eal: return true or false from lcore role check function

 lib/librte_eal/common/eal_common_thread.c | 5 +----
 lib/librte_eal/common/include/rte_lcore.h | 2 +-
 2 files changed, 2 insertions(+), 5 deletions(-)

-- 
2.6.4

[dpdk-dev] [PATCH 1/1] eal: return true or false from lcore role check function

[Erik Gabriel Carrillo]

Update rte_lcore_has_role() so that it returns true/false instead of
success/failure.

Fixes: 78666372fa2b ("eal: add function to check lcore role")

Signed-off-by: Erik Gabriel Carrillo <erik.g.carrillo@intel.com>
---
 lib/librte_eal/common/eal_common_thread.c | 5 +----
 lib/librte_eal/common/include/rte_lcore.h | 2 +-
 2 files changed, 2 insertions(+), 5 deletions(-)

diff --git a/lib/librte_eal/common/eal_common_thread.c b/lib/librte_eal/common/eal_common_thread.c
index 55e9696..28ee6d0 100644
--- a/lib/librte_eal/common/eal_common_thread.c
+++ b/lib/librte_eal/common/eal_common_thread.c
@@ -59,12 +59,9 @@ rte_lcore_has_role(unsigned int lcore_id, enum rte_lcore_role_t role)
 	struct rte_config *cfg = rte_eal_get_configuration();
 
 	if (lcore_id >= RTE_MAX_LCORE)
-		return -EINVAL;
-
-	if (cfg->lcore_role[lcore_id] == role)
 		return 0;
 
-	return -EINVAL;
+	return cfg->lcore_role[lcore_id] == role;
 }
 
 int eal_cpuset_socket_id(rte_cpuset_t *cpusetp)
diff --git a/lib/librte_eal/common/include/rte_lcore.h b/lib/librte_eal/common/include/rte_lcore.h
index c89e6ba..fba04f1 100644
--- a/lib/librte_eal/common/include/rte_lcore.h
+++ b/lib/librte_eal/common/include/rte_lcore.h
@@ -271,7 +271,7 @@ int rte_thread_setname(pthread_t id, const char *name);
  * @param role
  *   The role to be checked against.
  * @return
- *   On success, return 0; otherwise return a negative value.
+ *   True if the given core has the specified role; false otherwise.
  */
 int
 rte_lcore_has_role(unsigned int lcore_id, enum rte_lcore_role_t role);
-- 
2.6.4

Re: [dpdk-dev] [PATCH 1/1] eal: return true or false from lcore role check function

[Pavan Nikhilesh]

On Wed, Jan 03, 2018 at 12:43:35PM -0600, Erik Gabriel Carrillo wrote:
> Update rte_lcore_has_role() so that it returns true/false instead of
> success/failure.
>
> Fixes: 78666372fa2b ("eal: add function to check lcore role")
>
> Signed-off-by: Erik Gabriel Carrillo <erik.g.carrillo@intel.com>
> ---
>  lib/librte_eal/common/eal_common_thread.c | 5 +----
>  lib/librte_eal/common/include/rte_lcore.h | 2 +-
>  2 files changed, 2 insertions(+), 5 deletions(-)
>
> diff --git a/lib/librte_eal/common/eal_common_thread.c b/lib/librte_eal/common/eal_common_thread.c
> index 55e9696..28ee6d0 100644
> --- a/lib/librte_eal/common/eal_common_thread.c
> +++ b/lib/librte_eal/common/eal_common_thread.c
> @@ -59,12 +59,9 @@ rte_lcore_has_role(unsigned int lcore_id, enum rte_lcore_role_t role)
>  	struct rte_config *cfg = rte_eal_get_configuration();
>
>  	if (lcore_id >= RTE_MAX_LCORE)
> -		return -EINVAL;
> -
> -	if (cfg->lcore_role[lcore_id] == role)
>  		return 0;
>
> -	return -EINVAL;
> +	return cfg->lcore_role[lcore_id] == role;
>  }
>
>  int eal_cpuset_socket_id(rte_cpuset_t *cpusetp)
> diff --git a/lib/librte_eal/common/include/rte_lcore.h b/lib/librte_eal/common/include/rte_lcore.h
> index c89e6ba..fba04f1 100644
> --- a/lib/librte_eal/common/include/rte_lcore.h
> +++ b/lib/librte_eal/common/include/rte_lcore.h
> @@ -271,7 +271,7 @@ int rte_thread_setname(pthread_t id, const char *name);
>   * @param role
>   *   The role to be checked against.
>   * @return
> - *   On success, return 0; otherwise return a negative value.
> + *   True if the given core has the specified role; false otherwise.
>   */
>  int
>  rte_lcore_has_role(unsigned int lcore_id, enum rte_lcore_role_t role);
> --
> 2.6.4
>

LGTM.

Acked-by: Pavan Nikhilesh <pbhagavatula@caviumnetworks.com>

Re: [dpdk-dev] [PATCH 1/1] eal: return true or false from lcore role check function

[Aaron Conole]

Hi Erik,

Erik Gabriel Carrillo <erik.g.carrillo@intel.com> writes:

> Update rte_lcore_has_role() so that it returns true/false instead of
> success/failure.
>
> Fixes: 78666372fa2b ("eal: add function to check lcore role")
>
> Signed-off-by: Erik Gabriel Carrillo <erik.g.carrillo@intel.com>
> ---

I believe this breaks the published abi - Success is now 'true', and
failure is 'false';  previously success would be 0 == false.  You'll
need to invert the test, or note that the abi is breaking (since
semantically any caller will need to invert the test).

Re: [dpdk-dev] [PATCH 1/1] eal: return true or false from lcore role check function

[Carrillo, Erik G]

Hi Aaron,

> -----Original Message-----
> From: Aaron Conole [mailto:aconole@redhat.com]
> Sent: Tuesday, January 9, 2018 10:45 AM
> To: Carrillo, Erik G <erik.g.carrillo@intel.com>
> Cc: pbhagavatula@caviumnetworks.com; Van Haaren, Harry
> <harry.van.haaren@intel.com>; dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH 1/1] eal: return true or false from lcore role
> check function
> 
> Hi Erik,
> 
> Erik Gabriel Carrillo <erik.g.carrillo@intel.com> writes:
> 
> > Update rte_lcore_has_role() so that it returns true/false instead of
> > success/failure.
> >
> > Fixes: 78666372fa2b ("eal: add function to check lcore role")
> >
> > Signed-off-by: Erik Gabriel Carrillo <erik.g.carrillo@intel.com>
> > ---
> 
> I believe this breaks the published abi - Success is now 'true', and failure is
> 'false';  previously success would be 0 == false.  You'll need to invert the test,
> or note that the abi is breaking (since semantically any caller will need to
> invert the test).

Good point.  Though it seems like an API change rather than an ABI change to me, would it still be handled the same way in terms of notice?  Also,  the ABI policy states, "ABI breakage due to changes such as reorganizing public structure fields for aesthetic or readability purposes should be avoided."   Perhaps I should go with an alternate patch that fixes the caller.

Thanks,
Erik

Re: [dpdk-dev] [PATCH 1/1] eal: return true or false from lcore role check function

[Thomas Monjalon]

12/01/2018 00:09, Carrillo, Erik G:
> Hi Aaron,
> 
> From: Aaron Conole [mailto:aconole@redhat.com]
> > 
> > Hi Erik,
> > 
> > Erik Gabriel Carrillo <erik.g.carrillo@intel.com> writes:
> > 
> > > Update rte_lcore_has_role() so that it returns true/false instead of
> > > success/failure.
> > >
> > > Fixes: 78666372fa2b ("eal: add function to check lcore role")
> > >
> > > Signed-off-by: Erik Gabriel Carrillo <erik.g.carrillo@intel.com>
> > > ---
> > 
> > I believe this breaks the published abi - Success is now 'true', and failure is
> > 'false';  previously success would be 0 == false.  You'll need to invert the test,
> > or note that the abi is breaking (since semantically any caller will need to
> > invert the test).
> 
> Good point.  Though it seems like an API change rather than an ABI change to me, would it still be handled the same way in terms of notice?  Also,  the ABI policy states, "ABI breakage due to changes such as reorganizing public structure fields for aesthetic or readability purposes should be avoided."   Perhaps I should go with an alternate patch that fixes the caller.

Most of the times, an API change is an ABI change.
Please make a deprecation notice.

Re: [dpdk-dev] [PATCH 1/1] eal: return true or false from lcore role check function

[Carrillo, Erik G]

> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas@monjalon.net]
> Sent: Thursday, January 11, 2018 5:17 PM
> To: Carrillo, Erik G <erik.g.carrillo@intel.com>
> Cc: dev@dpdk.org; Aaron Conole <aconole@redhat.com>;
> pbhagavatula@caviumnetworks.com; Van Haaren, Harry
> <harry.van.haaren@intel.com>
> Subject: Re: [dpdk-dev] [PATCH 1/1] eal: return true or false from lcore role
> check function
> 
> 12/01/2018 00:09, Carrillo, Erik G:
> > Hi Aaron,
> >
> > From: Aaron Conole [mailto:aconole@redhat.com]
> > >
> > > Hi Erik,
> > >
> > > Erik Gabriel Carrillo <erik.g.carrillo@intel.com> writes:
> > >
> > > > Update rte_lcore_has_role() so that it returns true/false instead
> > > > of success/failure.
> > > >
> > > > Fixes: 78666372fa2b ("eal: add function to check lcore role")
> > > >
> > > > Signed-off-by: Erik Gabriel Carrillo <erik.g.carrillo@intel.com>
> > > > ---
> > >
> > > I believe this breaks the published abi - Success is now 'true', and
> > > failure is 'false';  previously success would be 0 == false.  You'll
> > > need to invert the test, or note that the abi is breaking (since
> > > semantically any caller will need to invert the test).
> >
> > Good point.  Though it seems like an API change rather than an ABI change
> to me, would it still be handled the same way in terms of notice?  Also,  the
> ABI policy states, "ABI breakage due to changes such as reorganizing public
> structure fields for aesthetic or readability purposes should be avoided."
> Perhaps I should go with an alternate patch that fixes the caller.
> 
> Most of the times, an API change is an ABI change.
> Please make a deprecation notice.

Ok, thanks Thomas - will do.  Should I mark the above patch as "deferred" for the time being?

Re: [dpdk-dev] [PATCH 1/1] eal: return true or false from lcore role check function

[Thomas Monjalon]

12/01/2018 19:01, Carrillo, Erik G:
> From: Thomas Monjalon [mailto:thomas@monjalon.net]
> > 12/01/2018 00:09, Carrillo, Erik G:
> > > Hi Aaron,
> > >
> > > From: Aaron Conole [mailto:aconole@redhat.com]
> > > >
> > > > Hi Erik,
> > > >
> > > > Erik Gabriel Carrillo <erik.g.carrillo@intel.com> writes:
> > > >
> > > > > Update rte_lcore_has_role() so that it returns true/false instead
> > > > > of success/failure.
> > > > >
> > > > > Fixes: 78666372fa2b ("eal: add function to check lcore role")
> > > > >
> > > > > Signed-off-by: Erik Gabriel Carrillo <erik.g.carrillo@intel.com>
> > > > > ---
> > > >
> > > > I believe this breaks the published abi - Success is now 'true', and
> > > > failure is 'false';  previously success would be 0 == false.  You'll
> > > > need to invert the test, or note that the abi is breaking (since
> > > > semantically any caller will need to invert the test).
> > >
> > > Good point.  Though it seems like an API change rather than an ABI change
> > to me, would it still be handled the same way in terms of notice?  Also,  the
> > ABI policy states, "ABI breakage due to changes such as reorganizing public
> > structure fields for aesthetic or readability purposes should be avoided."
> > Perhaps I should go with an alternate patch that fixes the caller.
> > 
> > Most of the times, an API change is an ABI change.
> > Please make a deprecation notice.
> 
> Ok, thanks Thomas - will do.  Should I mark the above patch as "deferred" for the time being?

Yes, thanks
All deferred patches are set to New when starting a new release cycle.
So it should not be lost :)