diff mbox series

rcu: fix build failure with debug dp log level

Message ID 20220829165151.472-1-anoobj@marvell.com (mailing list archive)
State Superseded, archived
Delegated to: Thomas Monjalon
Headers
Series rcu: fix build failure with debug dp log level |

Checks

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

Commit Message

Anoob Joseph Aug. 29, 2022, 4:51 p.m. UTC 
  Build fails if RTE_LOG_DP_LEVEL is set to RTE_LOG_DEBUG. Fix the
same by including the required header when RTE_LOG_DP_LEVEL is
set to RTE_LOG_DEBUG.

../lib/rcu/rte_rcu_qsbr.h:678:40: error: expected ‘)’ before ‘PRIu64’
  678 |    "%s: status: least acked token = %" PRIu64,
      |                                        ^~~~~~

Fixes: 30a1de105a5f ("lib: remove unneeded header includes")
Cc: sean.morrissey@intel.com

Signed-off-by: Anoob Joseph <anoobj@marvell.com>
---
 lib/rcu/rte_rcu_qsbr.h | 4 ++++
 1 file changed, 4 insertions(+)

--
2.25.1

Comments

Honnappa Nagarahalli Aug. 29, 2022, 4:55 p.m. UTC | #1 
> -----Original Message-----
> From: Anoob Joseph <anoobj@marvell.com>
> Sent: Monday, August 29, 2022 11:52 AM
> To: Honnappa Nagarahalli <Honnappa.Nagarahalli@arm.com>
> Cc: jerinj@marvell.com; dev@dpdk.org; sean.morrissey@intel.com
> Subject: [PATCH] rcu: fix build failure with debug dp log level
> 
> Build fails if RTE_LOG_DP_LEVEL is set to RTE_LOG_DEBUG. Fix the same by
> including the required header when RTE_LOG_DP_LEVEL is set to
> RTE_LOG_DEBUG.
> 
> ../lib/rcu/rte_rcu_qsbr.h:678:40: error: expected ‘)’ before ‘PRIu64’
>   678 |    "%s: status: least acked token = %" PRIu64,
>       |                                        ^~~~~~
> 
> Fixes: 30a1de105a5f ("lib: remove unneeded header includes")
> Cc: sean.morrissey@intel.com
Agree on the fix.
@sean.morrissey@intel.com Does the process that removed this header file inclusion needs fixing?
If yes, should that fix be included in this patch?

> 
> Signed-off-by: Anoob Joseph <anoobj@marvell.com>
> ---
>  lib/rcu/rte_rcu_qsbr.h | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/lib/rcu/rte_rcu_qsbr.h b/lib/rcu/rte_rcu_qsbr.h index
> d81bf5e8db..b0f1720ca1 100644
> --- a/lib/rcu/rte_rcu_qsbr.h
> +++ b/lib/rcu/rte_rcu_qsbr.h
> @@ -37,6 +37,10 @@ extern "C" {
>  #include <rte_atomic.h>
>  #include <rte_ring.h>
> 
> +#if RTE_LOG_DP_LEVEL >= RTE_LOG_DEBUG
> +#include <inttypes.h>
> +#endif
> +
>  extern int rte_rcu_log_type;
> 
>  #if RTE_LOG_DP_LEVEL >= RTE_LOG_DEBUG
> --
> 2.25.1
Sean Morrissey Aug. 30, 2022, 8:38 a.m. UTC | #2 
On 29/08/2022 17:55, Honnappa Nagarahalli wrote:
>> -----Original Message-----
>> From: Anoob Joseph <anoobj@marvell.com>
>> Sent: Monday, August 29, 2022 11:52 AM
>> To: Honnappa Nagarahalli <Honnappa.Nagarahalli@arm.com>
>> Cc: jerinj@marvell.com; dev@dpdk.org; sean.morrissey@intel.com
>> Subject: [PATCH] rcu: fix build failure with debug dp log level
>>
>> Build fails if RTE_LOG_DP_LEVEL is set to RTE_LOG_DEBUG. Fix the same by
>> including the required header when RTE_LOG_DP_LEVEL is set to
>> RTE_LOG_DEBUG.
>>
>> ../lib/rcu/rte_rcu_qsbr.h:678:40: error: expected ‘)’ before ‘PRIu64’
>>    678 |    "%s: status: least acked token = %" PRIu64,
>>        |                                        ^~~~~~
>>
>> Fixes: 30a1de105a5f ("lib: remove unneeded header includes")
>> Cc: sean.morrissey@intel.com
> Agree on the fix.
> @sean.morrissey@intel.com Does the process that removed this header file inclusion needs fixing?
> If yes, should that fix be included in this patch?

@Honnappa.Nagarahalli@arm.com Yes, I believe the tool will need an 
update, however, I believe it should be a separate patch for that.

>> Signed-off-by: Anoob Joseph <anoobj@marvell.com>
>> ---
>>   lib/rcu/rte_rcu_qsbr.h | 4 ++++
>>   1 file changed, 4 insertions(+)
>>
>> diff --git a/lib/rcu/rte_rcu_qsbr.h b/lib/rcu/rte_rcu_qsbr.h index
>> d81bf5e8db..b0f1720ca1 100644
>> --- a/lib/rcu/rte_rcu_qsbr.h
>> +++ b/lib/rcu/rte_rcu_qsbr.h
>> @@ -37,6 +37,10 @@ extern "C" {
>>   #include <rte_atomic.h>
>>   #include <rte_ring.h>
>>
>> +#if RTE_LOG_DP_LEVEL >= RTE_LOG_DEBUG
>> +#include <inttypes.h>
>> +#endif
>> +
>>   extern int rte_rcu_log_type;
>>
>>   #if RTE_LOG_DP_LEVEL >= RTE_LOG_DEBUG
>> --
>> 2.25.1
Honnappa Nagarahalli Aug. 30, 2022, 1:17 p.m. UTC | #3 
<snip>

> >>
> >> Build fails if RTE_LOG_DP_LEVEL is set to RTE_LOG_DEBUG. Fix the same
> >> by including the required header when RTE_LOG_DP_LEVEL is set to
> >> RTE_LOG_DEBUG.
> >>
> >> ../lib/rcu/rte_rcu_qsbr.h:678:40: error: expected ‘)’ before ‘PRIu64’
> >>    678 |    "%s: status: least acked token = %" PRIu64,
> >>        |                                        ^~~~~~
> >>
> >> Fixes: 30a1de105a5f ("lib: remove unneeded header includes")
> >> Cc: sean.morrissey@intel.com
> > Agree on the fix.
> > @sean.morrissey@intel.com Does the process that removed this header file
> inclusion needs fixing?
> > If yes, should that fix be included in this patch?
> 
> @Honnappa.Nagarahalli@arm.com Yes, I believe the tool will need an
> update, however, I believe it should be a separate patch for that.
Ok, as long as it is addressed, it should be fine.
 
> 
> >> Signed-off-by: Anoob Joseph <anoobj@marvell.com>
> >> ---
> >>   lib/rcu/rte_rcu_qsbr.h | 4 ++++
> >>   1 file changed, 4 insertions(+)
> >>
> >> diff --git a/lib/rcu/rte_rcu_qsbr.h b/lib/rcu/rte_rcu_qsbr.h index
> >> d81bf5e8db..b0f1720ca1 100644
> >> --- a/lib/rcu/rte_rcu_qsbr.h
> >> +++ b/lib/rcu/rte_rcu_qsbr.h
> >> @@ -37,6 +37,10 @@ extern "C" {
> >>   #include <rte_atomic.h>
> >>   #include <rte_ring.h>
> >>
> >> +#if RTE_LOG_DP_LEVEL >= RTE_LOG_DEBUG #include <inttypes.h> #endif
> >> +
> >>   extern int rte_rcu_log_type;
> >>
> >>   #if RTE_LOG_DP_LEVEL >= RTE_LOG_DEBUG
> >> --
> >> 2.25.1
Honnappa Nagarahalli Sept. 28, 2022, 10:04 p.m. UTC | #4 
<snip>
> 
> > >>
> > >> Build fails if RTE_LOG_DP_LEVEL is set to RTE_LOG_DEBUG. Fix the
> > >> same by including the required header when RTE_LOG_DP_LEVEL is set
> > >> to RTE_LOG_DEBUG.
> > >>
> > >> ../lib/rcu/rte_rcu_qsbr.h:678:40: error: expected ‘)’ before ‘PRIu64’
> > >>    678 |    "%s: status: least acked token = %" PRIu64,
> > >>        |                                        ^~~~~~
> > >>
> > >> Fixes: 30a1de105a5f ("lib: remove unneeded header includes")
> > >> Cc: sean.morrissey@intel.com
> > > Agree on the fix.
> > > @sean.morrissey@intel.com Does the process that removed this header
> > > file
> > inclusion needs fixing?
> > > If yes, should that fix be included in this patch?
> >
> > @Honnappa.Nagarahalli@arm.com Yes, I believe the tool will need an
> > update, however, I believe it should be a separate patch for that.
> Ok, as long as it is addressed, it should be fine.
> 
> >
> > >> Signed-off-by: Anoob Joseph <anoobj@marvell.com>
Reviewed-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>

> > >> ---
> > >>   lib/rcu/rte_rcu_qsbr.h | 4 ++++
> > >>   1 file changed, 4 insertions(+)
> > >>
> > >> diff --git a/lib/rcu/rte_rcu_qsbr.h b/lib/rcu/rte_rcu_qsbr.h index
> > >> d81bf5e8db..b0f1720ca1 100644
> > >> --- a/lib/rcu/rte_rcu_qsbr.h
> > >> +++ b/lib/rcu/rte_rcu_qsbr.h
> > >> @@ -37,6 +37,10 @@ extern "C" {
> > >>   #include <rte_atomic.h>
> > >>   #include <rte_ring.h>
> > >>
> > >> +#if RTE_LOG_DP_LEVEL >= RTE_LOG_DEBUG #include <inttypes.h>
> #endif
> > >> +
> > >>   extern int rte_rcu_log_type;
> > >>
> > >>   #if RTE_LOG_DP_LEVEL >= RTE_LOG_DEBUG
> > >> --
> > >> 2.25.1
Stephen Hemminger Sept. 28, 2022, 10:21 p.m. UTC | #5 
On Mon, 29 Aug 2022 22:21:51 +0530
Anoob Joseph <anoobj@marvell.com> wrote:

> Build fails if RTE_LOG_DP_LEVEL is set to RTE_LOG_DEBUG. Fix the
> same by including the required header when RTE_LOG_DP_LEVEL is
> set to RTE_LOG_DEBUG.
> 
> ../lib/rcu/rte_rcu_qsbr.h:678:40: error: expected ‘)’ before ‘PRIu64’
>   678 |    "%s: status: least acked token = %" PRIu64,
>       |                                        ^~~~~~
> 
> Fixes: 30a1de105a5f ("lib: remove unneeded header includes")
> Cc: sean.morrissey@intel.com
> 
> Signed-off-by: Anoob Joseph <anoobj@marvell.com>
> ---
>  lib/rcu/rte_rcu_qsbr.h | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/lib/rcu/rte_rcu_qsbr.h b/lib/rcu/rte_rcu_qsbr.h
> index d81bf5e8db..b0f1720ca1 100644
> --- a/lib/rcu/rte_rcu_qsbr.h
> +++ b/lib/rcu/rte_rcu_qsbr.h
> @@ -37,6 +37,10 @@ extern "C" {
>  #include <rte_atomic.h>
>  #include <rte_ring.h>
> 
> +#if RTE_LOG_DP_LEVEL >= RTE_LOG_DEBUG
> +#include <inttypes.h>
> +#endif
> +
>  extern int rt

This is not the best way to fix this.
Just always include the header file.
Having it conditional can lead to more problems
Anoob Joseph Sept. 29, 2022, 3:25 a.m. UTC | #6 
Hi Stephen,

Please see inline.

Thanks,
Anoob

> -----Original Message-----
> From: Stephen Hemminger <stephen@networkplumber.org>
> Sent: Thursday, September 29, 2022 3:51 AM
> To: Anoob Joseph <anoobj@marvell.com>
> Cc: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>; Jerin Jacob
> Kollanukkaran <jerinj@marvell.com>; dev@dpdk.org;
> sean.morrissey@intel.com
> Subject: [EXT] Re: [PATCH] rcu: fix build failure with debug dp log level
> 
> External Email
> 
> ----------------------------------------------------------------------
> On Mon, 29 Aug 2022 22:21:51 +0530
> Anoob Joseph <anoobj@marvell.com> wrote:
> 
> > Build fails if RTE_LOG_DP_LEVEL is set to RTE_LOG_DEBUG. Fix the same
> > by including the required header when RTE_LOG_DP_LEVEL is set to
> > RTE_LOG_DEBUG.
> >
> > ../lib/rcu/rte_rcu_qsbr.h:678:40: error: expected ‘)’ before ‘PRIu64’
> >   678 |    "%s: status: least acked token = %" PRIu64,
> >       |                                        ^~~~~~
> >
> > Fixes: 30a1de105a5f ("lib: remove unneeded header includes")
> > Cc: sean.morrissey@intel.com
> >
> > Signed-off-by: Anoob Joseph <anoobj@marvell.com>
> > ---
> >  lib/rcu/rte_rcu_qsbr.h | 4 ++++
> >  1 file changed, 4 insertions(+)
> >
> > diff --git a/lib/rcu/rte_rcu_qsbr.h b/lib/rcu/rte_rcu_qsbr.h index
> > d81bf5e8db..b0f1720ca1 100644
> > --- a/lib/rcu/rte_rcu_qsbr.h
> > +++ b/lib/rcu/rte_rcu_qsbr.h
> > @@ -37,6 +37,10 @@ extern "C" {
> >  #include <rte_atomic.h>
> >  #include <rte_ring.h>
> >
> > +#if RTE_LOG_DP_LEVEL >= RTE_LOG_DEBUG #include <inttypes.h>
> #endif
> > +
> >  extern int rt
> 
> This is not the best way to fix this.
> Just always include the header file.
> Having it conditional can lead to more problems

[Anoob] The header include is only required when RTE_LOG_DP_LEVEL is lower than RTE_LOG_DEBUG. I'm not sure how the tool runs, but it may still flag this include as an unnecessary include. But I can make the change if the tool can ignore this case.

@Sean, do you agree with the suggestion?
Bruce Richardson Sept. 29, 2022, 8:27 a.m. UTC | #7 
On Thu, Sep 29, 2022 at 03:25:44AM +0000, Anoob Joseph wrote:
> Hi Stephen,
> 
> Please see inline.
> 
> Thanks,
> Anoob
> 
> > -----Original Message-----
> > From: Stephen Hemminger <stephen@networkplumber.org>
> > Sent: Thursday, September 29, 2022 3:51 AM
> > To: Anoob Joseph <anoobj@marvell.com>
> > Cc: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>; Jerin Jacob
> > Kollanukkaran <jerinj@marvell.com>; dev@dpdk.org;
> > sean.morrissey@intel.com
> > Subject: [EXT] Re: [PATCH] rcu: fix build failure with debug dp log level
> > 
> > External Email
> > 
> > ----------------------------------------------------------------------
> > On Mon, 29 Aug 2022 22:21:51 +0530
> > Anoob Joseph <anoobj@marvell.com> wrote:
> > 
> > > Build fails if RTE_LOG_DP_LEVEL is set to RTE_LOG_DEBUG. Fix the same
> > > by including the required header when RTE_LOG_DP_LEVEL is set to
> > > RTE_LOG_DEBUG.
> > >
> > > ../lib/rcu/rte_rcu_qsbr.h:678:40: error: expected ‘)’ before ‘PRIu64’
> > >   678 |    "%s: status: least acked token = %" PRIu64,
> > >       |                                        ^~~~~~
> > >
> > > Fixes: 30a1de105a5f ("lib: remove unneeded header includes")
> > > Cc: sean.morrissey@intel.com
> > >
> > > Signed-off-by: Anoob Joseph <anoobj@marvell.com>
> > > ---
> > >  lib/rcu/rte_rcu_qsbr.h | 4 ++++
> > >  1 file changed, 4 insertions(+)
> > >
> > > diff --git a/lib/rcu/rte_rcu_qsbr.h b/lib/rcu/rte_rcu_qsbr.h index
> > > d81bf5e8db..b0f1720ca1 100644
> > > --- a/lib/rcu/rte_rcu_qsbr.h
> > > +++ b/lib/rcu/rte_rcu_qsbr.h
> > > @@ -37,6 +37,10 @@ extern "C" {
> > >  #include <rte_atomic.h>
> > >  #include <rte_ring.h>
> > >
> > > +#if RTE_LOG_DP_LEVEL >= RTE_LOG_DEBUG #include <inttypes.h>
> > #endif
> > > +
> > >  extern int rt
> > 
> > This is not the best way to fix this.
> > Just always include the header file.
> > Having it conditional can lead to more problems
> 
> [Anoob] The header include is only required when RTE_LOG_DP_LEVEL is lower than RTE_LOG_DEBUG. I'm not sure how the tool runs, but it may still flag this include as an unnecessary include. But I can make the change if the tool can ignore this case.
>

I think the number of build configurations needing testing has made the
automatic removal of header includes too problematic to use further.
Therefore, I think either way - with or without the #if - is probably fine.

/Bruce
diff mbox series

Patch

diff --git a/lib/rcu/rte_rcu_qsbr.h b/lib/rcu/rte_rcu_qsbr.h
index d81bf5e8db..b0f1720ca1 100644
--- a/lib/rcu/rte_rcu_qsbr.h
+++ b/lib/rcu/rte_rcu_qsbr.h
@@ -37,6 +37,10 @@  extern "C" {
 #include <rte_atomic.h>
 #include <rte_ring.h>

+#if RTE_LOG_DP_LEVEL >= RTE_LOG_DEBUG
+#include <inttypes.h>
+#endif
+
 extern int rte_rcu_log_type;

 #if RTE_LOG_DP_LEVEL >= RTE_LOG_DEBUG