diff mbox series

[v2] doc: propose correction rte_{bsf, fls} inline functions type use

Message ID 1615836878-23213-1-git-send-email-roretzla@linux.microsoft.com (mailing list archive)
State Superseded, archived
Delegated to: David Marchand
Headers
Series [v2] doc: propose correction rte_{bsf, fls} inline functions type use |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/Intel-compilation success Compilation OK
ci/travis-robot success travis build: passed
ci/github-robot success github build: passed
ci/intel-Testing success Testing PASS

Commit Message

Tyler Retzlaff March 15, 2021, 7:34 p.m. UTC 
  The proposal has resulted from request to review [1] the following
functions where there appeared to be inconsistency in return type
or parameter type selections for the following inline functions.

rte_bsf32()
rte_bsf32_safe()
rte_bsf64()
rte_bsf64_safe()
rte_fls_u32()
rte_fls_u64()
rte_log2_u32()
rte_log2_u64()

[1] http://mails.dpdk.org/archives/dev/2021-March/201590.html

Signed-off-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
---
 doc/guides/rel_notes/deprecation.rst | 7 +++++++
 1 file changed, 7 insertions(+)

Comments

Thomas Monjalon Oct. 25, 2021, 7:14 p.m. UTC | #1 
15/03/2021 20:34, Tyler Retzlaff:
> The proposal has resulted from request to review [1] the following
> functions where there appeared to be inconsistency in return type
> or parameter type selections for the following inline functions.
> 
> rte_bsf32()
> rte_bsf32_safe()
> rte_bsf64()
> rte_bsf64_safe()
> rte_fls_u32()
> rte_fls_u64()
> rte_log2_u32()
> rte_log2_u64()
> 
> [1] http://mails.dpdk.org/archives/dev/2021-March/201590.html
> 
> Signed-off-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
> ---
> --- a/doc/guides/rel_notes/deprecation.rst
> +++ b/doc/guides/rel_notes/deprecation.rst
> +* eal: Fix inline function return and parameter types for rte_{bsf,fls}
> +  inline functions to be consistent.
> +  Change ``rte_bsf32_safe`` parameter ``v`` from ``uint64_t`` to ``uint32_t``.
> +  Change ``rte_bsf64`` return type to  ``uint32_t`` instead of ``int``.
> +  Change ``rte_fls_u32`` return type to ``uint32_t`` instead of ``int``.
> +  Change ``rte_fls_u64`` return type to ``uint32_t`` instead of ``int``.

It seems we completely forgot this.
How critical is it?
Morten Brørup Oct. 26, 2021, 7:45 a.m. UTC | #2 
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Thomas Monjalon
> Sent: Monday, 25 October 2021 21.14
> 
> 15/03/2021 20:34, Tyler Retzlaff:
> > The proposal has resulted from request to review [1] the following
> > functions where there appeared to be inconsistency in return type
> > or parameter type selections for the following inline functions.
> >
> > rte_bsf32()
> > rte_bsf32_safe()
> > rte_bsf64()
> > rte_bsf64_safe()
> > rte_fls_u32()
> > rte_fls_u64()
> > rte_log2_u32()
> > rte_log2_u64()
> >
> > [1] http://mails.dpdk.org/archives/dev/2021-March/201590.html
> >
> > Signed-off-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
> > ---
> > --- a/doc/guides/rel_notes/deprecation.rst
> > +++ b/doc/guides/rel_notes/deprecation.rst
> > +* eal: Fix inline function return and parameter types for
> rte_{bsf,fls}
> > +  inline functions to be consistent.
> > +  Change ``rte_bsf32_safe`` parameter ``v`` from ``uint64_t`` to
> ``uint32_t``.
> > +  Change ``rte_bsf64`` return type to  ``uint32_t`` instead of
> ``int``.
> > +  Change ``rte_fls_u32`` return type to ``uint32_t`` instead of
> ``int``.
> > +  Change ``rte_fls_u64`` return type to ``uint32_t`` instead of
> ``int``.
> 
> It seems we completely forgot this.
> How critical is it?

Not updating has near zero effect on bug probability: Incorrectly returning signed int instead of unsigned int is extremely unlikely to cause problems.

Updating has near zero performance improvement: The unnecessary expansion of a parameter value from 32 to 64 bits is cheap.

The update's only tangible benefit is API consistency. :-)

-Morten
Tyler Retzlaff Nov. 11, 2021, 4:15 a.m. UTC | #3 
On Tue, Oct 26, 2021 at 09:45:20AM +0200, Morten Brørup wrote:
> > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Thomas Monjalon
> > Sent: Monday, 25 October 2021 21.14
> > 
> > 15/03/2021 20:34, Tyler Retzlaff:
> > > The proposal has resulted from request to review [1] the following
> > > functions where there appeared to be inconsistency in return type
> > > or parameter type selections for the following inline functions.
> > >
> > > rte_bsf32()
> > > rte_bsf32_safe()
> > > rte_bsf64()
> > > rte_bsf64_safe()
> > > rte_fls_u32()
> > > rte_fls_u64()
> > > rte_log2_u32()
> > > rte_log2_u64()
> > >
> > > [1] http://mails.dpdk.org/archives/dev/2021-March/201590.html
> > >
> > > Signed-off-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
> > > ---
> > > --- a/doc/guides/rel_notes/deprecation.rst
> > > +++ b/doc/guides/rel_notes/deprecation.rst
> > > +* eal: Fix inline function return and parameter types for
> > rte_{bsf,fls}
> > > +  inline functions to be consistent.
> > > +  Change ``rte_bsf32_safe`` parameter ``v`` from ``uint64_t`` to
> > ``uint32_t``.
> > > +  Change ``rte_bsf64`` return type to  ``uint32_t`` instead of
> > ``int``.
> > > +  Change ``rte_fls_u32`` return type to ``uint32_t`` instead of
> > ``int``.
> > > +  Change ``rte_fls_u64`` return type to ``uint32_t`` instead of
> > ``int``.
> > 
> > It seems we completely forgot this.
> > How critical is it?
> 

our organization as a matter of internal security policy requires these
sorts of things to be fixed. while i didn't see any bugs in the dpdk
code there is an opportunity for users of these functions to
accidentally write code that is prone to integer and buffer overflow
class bugs.

there is no urgency, but why leave things sloppy? though i do wish this
had been responded to in a more timely manner 7 months for something
that should have almost been rubber stamped.

> Not updating has near zero effect on bug probability: Incorrectly returning signed int instead of unsigned int is extremely unlikely to cause problems.
> 
> Updating has near zero performance improvement: The unnecessary expansion of a parameter value from 32 to 64 bits is cheap.
> 
> The update's only tangible benefit is API consistency. :-)
> 
> -Morten
Thomas Monjalon Nov. 11, 2021, 11:54 a.m. UTC | #4 
11/11/2021 05:15, Tyler Retzlaff:
> On Tue, Oct 26, 2021 at 09:45:20AM +0200, Morten Brørup wrote:
> > > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Thomas Monjalon
> > > Sent: Monday, 25 October 2021 21.14
> > > 
> > > 15/03/2021 20:34, Tyler Retzlaff:
> > > > The proposal has resulted from request to review [1] the following
> > > > functions where there appeared to be inconsistency in return type
> > > > or parameter type selections for the following inline functions.
> > > >
> > > > rte_bsf32()
> > > > rte_bsf32_safe()
> > > > rte_bsf64()
> > > > rte_bsf64_safe()
> > > > rte_fls_u32()
> > > > rte_fls_u64()
> > > > rte_log2_u32()
> > > > rte_log2_u64()
> > > >
> > > > [1] http://mails.dpdk.org/archives/dev/2021-March/201590.html
> > > >
> > > > Signed-off-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
> > > > ---
> > > > --- a/doc/guides/rel_notes/deprecation.rst
> > > > +++ b/doc/guides/rel_notes/deprecation.rst
> > > > +* eal: Fix inline function return and parameter types for
> > > rte_{bsf,fls}
> > > > +  inline functions to be consistent.
> > > > +  Change ``rte_bsf32_safe`` parameter ``v`` from ``uint64_t`` to
> > > ``uint32_t``.
> > > > +  Change ``rte_bsf64`` return type to  ``uint32_t`` instead of
> > > ``int``.
> > > > +  Change ``rte_fls_u32`` return type to ``uint32_t`` instead of
> > > ``int``.
> > > > +  Change ``rte_fls_u64`` return type to ``uint32_t`` instead of
> > > ``int``.
> > > 
> > > It seems we completely forgot this.
> > > How critical is it?
> > 
> 
> our organization as a matter of internal security policy requires these
> sorts of things to be fixed. while i didn't see any bugs in the dpdk
> code there is an opportunity for users of these functions to
> accidentally write code that is prone to integer and buffer overflow
> class bugs.
> 
> there is no urgency, but why leave things sloppy? though i do wish this
> had been responded to in a more timely manner 7 months for something
> that should have almost been rubber stamped.

It's difficult to be on all topics.
The best way to avoid such miss is to ping when you see no progress.

So what's next?
They are only inline functions, right? so no ABI breakage.
Is it going to require any change on application-side? I guess no.
Is it acceptable in 21.11-rc3? maybe too late?
Is it acceptable in 22.02?
Morten Brørup Nov. 11, 2021, 12:41 p.m. UTC | #5 
> From: Thomas Monjalon [mailto:thomas@monjalon.net]
> Sent: Thursday, 11 November 2021 12.55
> 
> 11/11/2021 05:15, Tyler Retzlaff:
> > On Tue, Oct 26, 2021 at 09:45:20AM +0200, Morten Brørup wrote:
> > > > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Thomas
> Monjalon
> > > > Sent: Monday, 25 October 2021 21.14
> > > >
> > > > 15/03/2021 20:34, Tyler Retzlaff:
> > > > > The proposal has resulted from request to review [1] the
> following
> > > > > functions where there appeared to be inconsistency in return
> type
> > > > > or parameter type selections for the following inline
> functions.
> > > > >
> > > > > rte_bsf32()
> > > > > rte_bsf32_safe()
> > > > > rte_bsf64()
> > > > > rte_bsf64_safe()
> > > > > rte_fls_u32()
> > > > > rte_fls_u64()
> > > > > rte_log2_u32()
> > > > > rte_log2_u64()
> > > > >
> > > > > [1] http://mails.dpdk.org/archives/dev/2021-March/201590.html
> > > > >
> > > > > Signed-off-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
> > > > > ---
> > > > > --- a/doc/guides/rel_notes/deprecation.rst
> > > > > +++ b/doc/guides/rel_notes/deprecation.rst
> > > > > +* eal: Fix inline function return and parameter types for
> > > > rte_{bsf,fls}
> > > > > +  inline functions to be consistent.
> > > > > +  Change ``rte_bsf32_safe`` parameter ``v`` from ``uint64_t``
> to
> > > > ``uint32_t``.
> > > > > +  Change ``rte_bsf64`` return type to  ``uint32_t`` instead of
> > > > ``int``.
> > > > > +  Change ``rte_fls_u32`` return type to ``uint32_t`` instead
> of
> > > > ``int``.
> > > > > +  Change ``rte_fls_u64`` return type to ``uint32_t`` instead
> of
> > > > ``int``.
> > > >
> > > > It seems we completely forgot this.
> > > > How critical is it?
> > >
> >
> > our organization as a matter of internal security policy requires
> these
> > sorts of things to be fixed. while i didn't see any bugs in the dpdk
> > code there is an opportunity for users of these functions to
> > accidentally write code that is prone to integer and buffer overflow
> > class bugs.
> >
> > there is no urgency, but why leave things sloppy? though i do wish
> this
> > had been responded to in a more timely manner 7 months for something
> > that should have almost been rubber stamped.
> 
> It's difficult to be on all topics.
> The best way to avoid such miss is to ping when you see no progress.
> 
> So what's next?
> They are only inline functions, right? so no ABI breakage.
> Is it going to require any change on application-side? I guess no.
> Is it acceptable in 21.11-rc3? maybe too late?
> Is it acceptable in 22.02?

If Microsoft (represented by Tyler in this case) considers this a bug, I would prefer getting it into 21.11 - especially because it is an LTS release.

-Morten
Jerin Jacob July 11, 2022, 2:07 p.m. UTC | #6 
On Thu, Nov 11, 2021 at 6:11 PM Morten Brørup <mb@smartsharesystems.com> wrote:
>
> > From: Thomas Monjalon [mailto:thomas@monjalon.net]
> > Sent: Thursday, 11 November 2021 12.55
> >
> > 11/11/2021 05:15, Tyler Retzlaff:
> > > On Tue, Oct 26, 2021 at 09:45:20AM +0200, Morten Brørup wrote:
> > > > > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Thomas
> > Monjalon
> > > > > Sent: Monday, 25 October 2021 21.14
> > > > >
> > > > > 15/03/2021 20:34, Tyler Retzlaff:
> > > > > > The proposal has resulted from request to review [1] the
> > following
> > > > > > functions where there appeared to be inconsistency in return
> > type
> > > > > > or parameter type selections for the following inline
> > functions.
> > > > > >
> > > > > > rte_bsf32()
> > > > > > rte_bsf32_safe()
> > > > > > rte_bsf64()
> > > > > > rte_bsf64_safe()
> > > > > > rte_fls_u32()
> > > > > > rte_fls_u64()
> > > > > > rte_log2_u32()
> > > > > > rte_log2_u64()
> > > > > >
> > > > > > [1] http://mails.dpdk.org/archives/dev/2021-March/201590.html
> > > > > >
> > > > > > Signed-off-by: Tyler Retzlaff <roretzla@linux.microsoft.com>


Acked-by: Jerin Jacob <jerinj@marvell.com>



> > > > > > ---
> > > > > > --- a/doc/guides/rel_notes/deprecation.rst
> > > > > > +++ b/doc/guides/rel_notes/deprecation.rst
> > > > > > +* eal: Fix inline function return and parameter types for
> > > > > rte_{bsf,fls}
> > > > > > +  inline functions to be consistent.
> > > > > > +  Change ``rte_bsf32_safe`` parameter ``v`` from ``uint64_t``
> > to
> > > > > ``uint32_t``.
> > > > > > +  Change ``rte_bsf64`` return type to  ``uint32_t`` instead of
> > > > > ``int``.
> > > > > > +  Change ``rte_fls_u32`` return type to ``uint32_t`` instead
> > of
> > > > > ``int``.
> > > > > > +  Change ``rte_fls_u64`` return type to ``uint32_t`` instead
> > of
> > > > > ``int``.
> > > > >
> > > > > It seems we completely forgot this.
> > > > > How critical is it?
> > > >
> > >
> > > our organization as a matter of internal security policy requires
> > these
> > > sorts of things to be fixed. while i didn't see any bugs in the dpdk
> > > code there is an opportunity for users of these functions to
> > > accidentally write code that is prone to integer and buffer overflow
> > > class bugs.
> > >
> > > there is no urgency, but why leave things sloppy? though i do wish
> > this
> > > had been responded to in a more timely manner 7 months for something
> > > that should have almost been rubber stamped.
> >
> > It's difficult to be on all topics.
> > The best way to avoid such miss is to ping when you see no progress.
> >
> > So what's next?
> > They are only inline functions, right? so no ABI breakage.
> > Is it going to require any change on application-side? I guess no.
> > Is it acceptable in 21.11-rc3? maybe too late?
> > Is it acceptable in 22.02?
>
> If Microsoft (represented by Tyler in this case) considers this a bug, I would prefer getting it into 21.11 - especially because it is an LTS release.
>
> -Morten
>
Thomas Monjalon July 13, 2022, 10:13 a.m. UTC | #7 
Tyler, there was no progress on this topic during the past year.
Please could you send the patch to fix API inconsistencies,
so we will merge it in 22.11?
Tyler Retzlaff July 18, 2022, 9:28 p.m. UTC | #8 
Hi Thomas,

I will see if i can carve out some time budget for it, it's a pretty old
patch so it could take some time to review.

On Wed, Jul 13, 2022 at 12:13:20PM +0200, Thomas Monjalon wrote:
> Tyler, there was no progress on this topic during the past year.
> Please could you send the patch to fix API inconsistencies,
> so we will merge it in 22.11?
>
diff mbox series

Patch

diff --git a/doc/guides/rel_notes/deprecation.rst b/doc/guides/rel_notes/deprecation.rst
index 64629e064..4934f4da4 100644
--- a/doc/guides/rel_notes/deprecation.rst
+++ b/doc/guides/rel_notes/deprecation.rst
@@ -17,6 +17,13 @@  Deprecation Notices
 * eal: The function ``rte_eal_remote_launch`` will return new error codes
   after read or write error on the pipe, instead of calling ``rte_panic``.
 
+* eal: Fix inline function return and parameter types for rte_{bsf,fls}
+  inline functions to be consistent.
+  Change ``rte_bsf32_safe`` parameter ``v`` from ``uint64_t`` to ``uint32_t``.
+  Change ``rte_bsf64`` return type to  ``uint32_t`` instead of ``int``.
+  Change ``rte_fls_u32`` return type to ``uint32_t`` instead of ``int``.
+  Change ``rte_fls_u64`` return type to ``uint32_t`` instead of ``int``.
+
 * rte_atomicNN_xxx: These APIs do not take memory order parameter. This does
   not allow for writing optimized code for all the CPU architectures supported
   in DPDK. DPDK will adopt C11 atomic operations semantics and provide wrappers