diff mbox series

examples/rxtx_callbacks: fix port ID format specifier

Message ID 20210502025656.29910-1-dmitry.kozliuk@gmail.com (mailing list archive)
State Superseded, archived
Delegated to: Thomas Monjalon
Headers
Series examples/rxtx_callbacks: fix port ID format specifier |

Checks

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

Commit Message

Dmitry Kozlyuk May 2, 2021, 2:56 a.m. UTC 
  Use "%u" and a cast as in other places when port ID is formatted.
This fixes -Wformat warning with clang 10.0.0 on Windows.

Fixes: f8244c6399d9 ("ethdev: increase port id range")
Cc: stable@dpdk.org

Signed-off-by: Dmitry Kozlyuk <dmitry.kozliuk@gmail.com>
---
 examples/rxtx_callbacks/main.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Tyler Retzlaff May 4, 2021, 12:11 a.m. UTC | #1 
On Sun, May 02, 2021 at 05:56:56AM +0300, Dmitry Kozlyuk wrote:
> Use "%u" and a cast as in other places when port ID is formatted.
> This fixes -Wformat warning with clang 10.0.0 on Windows.
> 
> Fixes: f8244c6399d9 ("ethdev: increase port id range")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Dmitry Kozlyuk <dmitry.kozliuk@gmail.com>
> ---
>  examples/rxtx_callbacks/main.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/examples/rxtx_callbacks/main.c b/examples/rxtx_callbacks/main.c
> index b57b2fc6bc..9574b6ea0d 100644
> --- a/examples/rxtx_callbacks/main.c
> +++ b/examples/rxtx_callbacks/main.c
> @@ -329,8 +329,8 @@ main(int argc, char *argv[])
>  	/* initialize all ports */
>  	RTE_ETH_FOREACH_DEV(portid)
>  		if (port_init(portid, mbuf_pool) != 0)
> -			rte_exit(EXIT_FAILURE, "Cannot init port %"PRIu8"\n",
> -					portid);

how come not just `% " PRIu16 "\n"' ?

what was the -Wformat clang on windows complaint?
Dmitry Kozlyuk May 4, 2021, 6:48 a.m. UTC | #2 
2021-05-03 17:11 (UTC-0700), Tyler Retzlaff:
> On Sun, May 02, 2021 at 05:56:56AM +0300, Dmitry Kozlyuk wrote:
> > Use "%u" and a cast as in other places when port ID is formatted.
> > This fixes -Wformat warning with clang 10.0.0 on Windows.
> > 
> > Fixes: f8244c6399d9 ("ethdev: increase port id range")
> > Cc: stable@dpdk.org
> > 
> > Signed-off-by: Dmitry Kozlyuk <dmitry.kozliuk@gmail.com>
> > ---
> >  examples/rxtx_callbacks/main.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/examples/rxtx_callbacks/main.c b/examples/rxtx_callbacks/main.c
> > index b57b2fc6bc..9574b6ea0d 100644
> > --- a/examples/rxtx_callbacks/main.c
> > +++ b/examples/rxtx_callbacks/main.c
> > @@ -329,8 +329,8 @@ main(int argc, char *argv[])
> >  	/* initialize all ports */
> >  	RTE_ETH_FOREACH_DEV(portid)
> >  		if (port_init(portid, mbuf_pool) != 0)
> > -			rte_exit(EXIT_FAILURE, "Cannot init port %"PRIu8"\n",
> > -					portid);  
> 
> how come not just `% " PRIu16 "\n"' ?
> 
> what was the -Wformat clang on windows complaint?

PRIx16 would work, but I noticed that in other places where port ID is
printed, the pattern above is used. IMO uniform approach is better.
Tyler Retzlaff May 5, 2021, 4 p.m. UTC | #3 
On Tue, May 04, 2021 at 09:48:22AM +0300, Dmitry Kozlyuk wrote:
> 2021-05-03 17:11 (UTC-0700), Tyler Retzlaff:
> > On Sun, May 02, 2021 at 05:56:56AM +0300, Dmitry Kozlyuk wrote:
> > > Use "%u" and a cast as in other places when port ID is formatted.
> > > This fixes -Wformat warning with clang 10.0.0 on Windows.
> > > 
> > > Fixes: f8244c6399d9 ("ethdev: increase port id range")
> > > Cc: stable@dpdk.org
> > > 
> > > Signed-off-by: Dmitry Kozlyuk <dmitry.kozliuk@gmail.com>
> > > ---
> > >  examples/rxtx_callbacks/main.c | 4 ++--
> > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/examples/rxtx_callbacks/main.c b/examples/rxtx_callbacks/main.c
> > > index b57b2fc6bc..9574b6ea0d 100644
> > > --- a/examples/rxtx_callbacks/main.c
> > > +++ b/examples/rxtx_callbacks/main.c
> > > @@ -329,8 +329,8 @@ main(int argc, char *argv[])
> > >  	/* initialize all ports */
> > >  	RTE_ETH_FOREACH_DEV(portid)
> > >  		if (port_init(portid, mbuf_pool) != 0)
> > > -			rte_exit(EXIT_FAILURE, "Cannot init port %"PRIu8"\n",
> > > -					portid);  
> > 
> > how come not just `% " PRIu16 "\n"' ?
> > 
> > what was the -Wformat clang on windows complaint?
> 
> PRIx16 would work, but I noticed that in other places where port ID is
> printed, the pattern above is used. IMO uniform approach is better.

ah, consistency. yes i'll have some of that. maybe one day in the future
we can change them all in one shot, but not today.

Acked-By: Tyler Retzlaff <roretzla@linux.microsoft.com>
Thomas Monjalon May 5, 2021, 9:39 p.m. UTC | #4 
05/05/2021 18:00, Tyler Retzlaff:
> On Tue, May 04, 2021 at 09:48:22AM +0300, Dmitry Kozlyuk wrote:
> > 2021-05-03 17:11 (UTC-0700), Tyler Retzlaff:
> > > On Sun, May 02, 2021 at 05:56:56AM +0300, Dmitry Kozlyuk wrote:
> > > > Use "%u" and a cast as in other places when port ID is formatted.
> > > > This fixes -Wformat warning with clang 10.0.0 on Windows.
> > > > 
> > > > Fixes: f8244c6399d9 ("ethdev: increase port id range")
> > > > Cc: stable@dpdk.org
> > > > 
> > > > Signed-off-by: Dmitry Kozlyuk <dmitry.kozliuk@gmail.com>
[...]
> > > > -			rte_exit(EXIT_FAILURE, "Cannot init port %"PRIu8"\n",
> > > > -					portid);  
> > > 
> > > how come not just `% " PRIu16 "\n"' ?
> > > 
> > > what was the -Wformat clang on windows complaint?
> > 
> > PRIx16 would work, but I noticed that in other places where port ID is
> > printed, the pattern above is used. IMO uniform approach is better.
> 
> ah, consistency. yes i'll have some of that. maybe one day in the future
> we can change them all in one shot, but not today.
> 
> Acked-By: Tyler Retzlaff <roretzla@linux.microsoft.com>

I think PRIu16 is more appropriate.
Consistency doesn't matter if an approach is better.
Tyler Retzlaff May 5, 2021, 10:45 p.m. UTC | #5 
On Wed, May 05, 2021 at 11:39:23PM +0200, Thomas Monjalon wrote:
> 05/05/2021 18:00, Tyler Retzlaff:
> > On Tue, May 04, 2021 at 09:48:22AM +0300, Dmitry Kozlyuk wrote:
> > > > 
> > > > what was the -Wformat clang on windows complaint?
> > > 
> > > PRIx16 would work, but I noticed that in other places where port ID is
> > > printed, the pattern above is used. IMO uniform approach is better.
> > 
> > ah, consistency. yes i'll have some of that. maybe one day in the future
> > we can change them all in one shot, but not today.
> > 
> > Acked-By: Tyler Retzlaff <roretzla@linux.microsoft.com>
> 
> I think PRIu16 is more appropriate.

works for me.

> Consistency doesn't matter if an approach is better.

well, there is some value in having consistency even if it is something
sub-optimal if we intend to do a mechanical change of all instances in
the future.

as a side question, what is the projects stance on getting more warnings
clean? there are a few not enabled that i'd really like to see e.g.
format, conversion, truncation etc..

i looked at lib/eal previously and there are... hundreds? of instances so
it's a non-trivial task. the problem i see is somehow getting to a
warnings clean state where we can enable -Werror in the CI pipeline but
at the same time figuring out how to prevent new instances from
appearing until we do.
Thomas Monjalon May 5, 2021, 11:13 p.m. UTC | #6 
06/05/2021 00:45, Tyler Retzlaff:
> as a side question, what is the projects stance on getting more warnings
> clean? there are a few not enabled that i'd really like to see e.g.
> format, conversion, truncation etc..
> 
> i looked at lib/eal previously and there are... hundreds? of instances so
> it's a non-trivial task. the problem i see is somehow getting to a
> warnings clean state where we can enable -Werror in the CI pipeline but
> at the same time figuring out how to prevent new instances from
> appearing until we do.

I don't understand the question.
We are already supposed to be warning-free,
and -werror should be enabled in all CI labs.
What are the gaps?
Tyler Retzlaff May 11, 2021, 10:34 p.m. UTC | #7 
On Thu, May 06, 2021 at 01:13:07AM +0200, Thomas Monjalon wrote:
> 06/05/2021 00:45, Tyler Retzlaff:
> > as a side question, what is the projects stance on getting more warnings
> > clean? there are a few not enabled that i'd really like to see e.g.
> > format, conversion, truncation etc..
> > 
> > i looked at lib/eal previously and there are... hundreds? of instances so
> > it's a non-trivial task. the problem i see is somehow getting to a
> > warnings clean state where we can enable -Werror in the CI pipeline but
> > at the same time figuring out how to prevent new instances from
> > appearing until we do.
> 
> I don't understand the question.
> We are already supposed to be warning-free,
> and -werror should be enabled in all CI labs.
> What are the gaps?

with the warnings we have enabled -Werror passes, but we don't have all
the warnings we could have that are of value.  it would be great to see
-Wconversion -Wsign-compare -Wsign-conversion passing clean.

i admit it is a lot of work though.
Thomas Monjalon May 11, 2021, 11:35 p.m. UTC | #8 
12/05/2021 00:34, Tyler Retzlaff:
> On Thu, May 06, 2021 at 01:13:07AM +0200, Thomas Monjalon wrote:
> > 06/05/2021 00:45, Tyler Retzlaff:
> > > as a side question, what is the projects stance on getting more warnings
> > > clean? there are a few not enabled that i'd really like to see e.g.
> > > format, conversion, truncation etc..
> > > 
> > > i looked at lib/eal previously and there are... hundreds? of instances so
> > > it's a non-trivial task. the problem i see is somehow getting to a
> > > warnings clean state where we can enable -Werror in the CI pipeline but
> > > at the same time figuring out how to prevent new instances from
> > > appearing until we do.
> > 
> > I don't understand the question.
> > We are already supposed to be warning-free,
> > and -werror should be enabled in all CI labs.
> > What are the gaps?
> 
> with the warnings we have enabled -Werror passes, but we don't have all
> the warnings we could have that are of value.  it would be great to see
> -Wconversion -Wsign-compare -Wsign-conversion passing clean.
> 
> i admit it is a lot of work though.

-Wsign-compare is enabled for sure, look at config/meson.build.
For the conversion warnings, I don't know.
Do not hesitate to propose new warnings enablement
with a list of warnings it is generating in its current state,
so we can decide whether to enable (and fix) or not.
diff mbox series

Patch

diff --git a/examples/rxtx_callbacks/main.c b/examples/rxtx_callbacks/main.c
index b57b2fc6bc..9574b6ea0d 100644
--- a/examples/rxtx_callbacks/main.c
+++ b/examples/rxtx_callbacks/main.c
@@ -329,8 +329,8 @@  main(int argc, char *argv[])
 	/* initialize all ports */
 	RTE_ETH_FOREACH_DEV(portid)
 		if (port_init(portid, mbuf_pool) != 0)
-			rte_exit(EXIT_FAILURE, "Cannot init port %"PRIu8"\n",
-					portid);
+			rte_exit(EXIT_FAILURE, "Cannot init port %u\n",
+					(unsigned int)portid);
 
 	if (rte_lcore_count() > 1)
 		printf("\nWARNING: Too much enabled lcores - "