[dpdk-dev,1/1] eal: return true or false from lcore role check function
Checks
Commit Message
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(-)
Comments
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>
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).
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
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.
> -----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?
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 :)
@@ -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)
@@ -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);