diff mbox series

eal: fix argument to rte_bsf32_safe

Message ID 20210713201221.301248-1-stephen@networkplumber.org (mailing list archive)
State Superseded, archived
Delegated to: David Marchand
Headers show
Series eal: fix argument to rte_bsf32_safe | expand

Checks

Context Check Description
ci/iol-intel-Performance fail Performance Testing issues
ci/intel-Testing success Testing PASS
ci/iol-testing fail Testing issues
ci/iol-intel-Functional success Functional Testing PASS
ci/iol-abi-testing success Testing PASS
ci/github-robot success github build: passed
ci/Intel-compilation success Compilation OK
ci/checkpatch success coding style OK

Commit Message

Stephen Hemminger July 13, 2021, 8:12 p.m. UTC
The first argument to rte_bsf32_safe was incorrectly declared as
a 64 bit value. This function only correctly handles on 32 bit values
and the underlying function rte_bsf32 only accepts 32 bit values.
This was introduced when the safe version was added and probably cause
by copy/paste from the 64 bit version.

The bug passed silently under the radar until some other code was
built with -Wall and -Wextra in C++ and C++ complains about the
missing cast.

Yes, this is a API signature change, but the original code was wrong.
It is an inline so not an ABI change.

Fixes: 4e261f551986 ("eal: add 64-bit bsf and 32-bit safe bsf functions")
Cc: anatoly.burakov@intel.com
Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 lib/eal/include/rte_common.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Tyler Retzlaff July 19, 2021, 5:15 p.m. UTC | #1
On Tue, Jul 13, 2021 at 01:12:21PM -0700, Stephen Hemminger wrote:
> The first argument to rte_bsf32_safe was incorrectly declared as
> a 64 bit value. This function only correctly handles on 32 bit values
> and the underlying function rte_bsf32 only accepts 32 bit values.
> This was introduced when the safe version was added and probably cause
> by copy/paste from the 64 bit version.

there are multiple errors in this family of functions [1] both in usage
and signatures. we previously discussed rolling all fixes up into a single
patch and announcing an api break.

a doc patch was submitted as per the process documented for breaking api
but received no replies [2]

i have a full patch that corrects the whole family if you would like to
take it instead. contact me offline if you are interested.

1. http://mails.dpdk.org/archives/dev/2021-March/201590.html
2. http://mails.dpdk.org/archives/dev/2021-March/201868.html

the change stand-alone is correct so

Acked-By: Tyler Retzlaff <roretzla@linux.microsoft.com>

> 
> The bug passed silently under the radar until some other code was
> built with -Wall and -Wextra in C++ and C++ complains about the
> missing cast.
> 
> Yes, this is a API signature change, but the original code was wrong.
> It is an inline so not an ABI change.
> 
> Fixes: 4e261f551986 ("eal: add 64-bit bsf and 32-bit safe bsf functions")
> Cc: anatoly.burakov@intel.com
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> ---
>  lib/eal/include/rte_common.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/lib/eal/include/rte_common.h b/lib/eal/include/rte_common.h
> index d5a32c66a5fe..99eb5f1820ae 100644
> --- a/lib/eal/include/rte_common.h
> +++ b/lib/eal/include/rte_common.h
> @@ -623,7 +623,7 @@ rte_bsf32(uint32_t v)
>   *     Returns 0 if ``v`` was 0, otherwise returns 1.
>   */
>  static inline int
> -rte_bsf32_safe(uint64_t v, uint32_t *pos)
> +rte_bsf32_safe(uint32_t v, uint32_t *pos)
>  {
>  	if (v == 0)
>  		return 0;
> -- 
> 2.30.2
Stephen Hemminger July 19, 2021, 10 p.m. UTC | #2
On Mon, 19 Jul 2021 10:15:34 -0700
Tyler Retzlaff <roretzla@linux.microsoft.com> wrote:

> On Tue, Jul 13, 2021 at 01:12:21PM -0700, Stephen Hemminger wrote:
> > The first argument to rte_bsf32_safe was incorrectly declared as
> > a 64 bit value. This function only correctly handles on 32 bit values
> > and the underlying function rte_bsf32 only accepts 32 bit values.
> > This was introduced when the safe version was added and probably cause
> > by copy/paste from the 64 bit version.  
> 
> there are multiple errors in this family of functions [1] both in usage
> and signatures. we previously discussed rolling all fixes up into a single
> patch and announcing an api break.
> 
> a doc patch was submitted as per the process documented for breaking api
> but received no replies [2]
> 
> i have a full patch that corrects the whole family if you would like to
> take it instead. contact me offline if you are interested.
> 
> 1. http://mails.dpdk.org/archives/dev/2021-March/201590.html
> 2. http://mails.dpdk.org/archives/dev/2021-March/201868.html
> 
> the change stand-alone is correct so
> 
> Acked-By: Tyler Retzlaff <roretzla@linux.microsoft.com>

Thanks, I think the larger set should go into 21.11 where API/ABI break
would be ok. My bit was all about fixing the bug where current code
breaks C++ users.
Thomas Monjalon July 20, 2021, 1:26 p.m. UTC | #3
20/07/2021 00:00, Stephen Hemminger:
> On Mon, 19 Jul 2021 10:15:34 -0700
> Tyler Retzlaff <roretzla@linux.microsoft.com> wrote:
> 
> > On Tue, Jul 13, 2021 at 01:12:21PM -0700, Stephen Hemminger wrote:
> > > The first argument to rte_bsf32_safe was incorrectly declared as
> > > a 64 bit value. This function only correctly handles on 32 bit values
> > > and the underlying function rte_bsf32 only accepts 32 bit values.
> > > This was introduced when the safe version was added and probably cause
> > > by copy/paste from the 64 bit version.  
> > 
> > there are multiple errors in this family of functions [1] both in usage
> > and signatures. we previously discussed rolling all fixes up into a single
> > patch and announcing an api break.
> > 
> > a doc patch was submitted as per the process documented for breaking api
> > but received no replies [2]
> > 
> > i have a full patch that corrects the whole family if you would like to
> > take it instead. contact me offline if you are interested.
> > 
> > 1. http://mails.dpdk.org/archives/dev/2021-March/201590.html
> > 2. http://mails.dpdk.org/archives/dev/2021-March/201868.html
> > 
> > the change stand-alone is correct so
> > 
> > Acked-By: Tyler Retzlaff <roretzla@linux.microsoft.com>
> 
> Thanks, I think the larger set should go into 21.11 where API/ABI break
> would be ok. My bit was all about fixing the bug where current code
> breaks C++ users.

Shouldn't we have a note in the API changes section of the release notes?
diff mbox series

Patch

diff --git a/lib/eal/include/rte_common.h b/lib/eal/include/rte_common.h
index d5a32c66a5fe..99eb5f1820ae 100644
--- a/lib/eal/include/rte_common.h
+++ b/lib/eal/include/rte_common.h
@@ -623,7 +623,7 @@  rte_bsf32(uint32_t v)
  *     Returns 0 if ``v`` was 0, otherwise returns 1.
  */
 static inline int
-rte_bsf32_safe(uint64_t v, uint32_t *pos)
+rte_bsf32_safe(uint32_t v, uint32_t *pos)
 {
 	if (v == 0)
 		return 0;