[dpdk-dev] [PATCH] eal/option: fix option register duplicate detection

[dpdk-dev] [PATCH] eal/option: fix option register duplicate detection

[Gaetan Rivet]

Missing brackets around the if means that the loop will end at its first
iteration.

Cc: stable@dpdk.org

Fixes: 2395332798d0 ("eal: add option register infrastructure")
Signed-off-by: Gaetan Rivet <gaetan.rivet@6wind.com>
---
 lib/librte_eal/common/rte_option.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/lib/librte_eal/common/rte_option.c b/lib/librte_eal/common/rte_option.c
index 02d59a869..eefb8b923 100644
--- a/lib/librte_eal/common/rte_option.c
+++ b/lib/librte_eal/common/rte_option.c
@@ -35,10 +35,11 @@ void __rte_experimental
 rte_option_register(struct rte_option *opt)
 {
 	TAILQ_FOREACH(option, &rte_option_list, next) {
-		if (strcmp(opt->opt_str, option->opt_str) == 0)
+		if (strcmp(opt->opt_str, option->opt_str) == 0) {
 			RTE_LOG(INFO, EAL, "Option %s has already been registered.",
 					opt->opt_str);
 			return;
+		}
 	}
 
 	TAILQ_INSERT_HEAD(&rte_option_list, opt, next);
-- 
2.19.1

Re: [dpdk-dev] [PATCH] eal/option: fix option register duplicate detection

[Burakov, Anatoly]

On 17-Dec-18 9:25 AM, Gaetan Rivet wrote:
> Missing brackets around the if means that the loop will end at its first
> iteration.
> 
> Cc: stable@dpdk.org
> 
> Fixes: 2395332798d0 ("eal: add option register infrastructure")
> Signed-off-by: Gaetan Rivet <gaetan.rivet@6wind.com>
> ---
>   lib/librte_eal/common/rte_option.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/lib/librte_eal/common/rte_option.c b/lib/librte_eal/common/rte_option.c
> index 02d59a869..eefb8b923 100644
> --- a/lib/librte_eal/common/rte_option.c
> +++ b/lib/librte_eal/common/rte_option.c
> @@ -35,10 +35,11 @@ void __rte_experimental
>   rte_option_register(struct rte_option *opt)
>   {
>   	TAILQ_FOREACH(option, &rte_option_list, next) {
> -		if (strcmp(opt->opt_str, option->opt_str) == 0)
> +		if (strcmp(opt->opt_str, option->opt_str) == 0) {
>   			RTE_LOG(INFO, EAL, "Option %s has already been registered.",
>   					opt->opt_str);
>   			return;
> +		}
>   	}
>   
>   	TAILQ_INSERT_HEAD(&rte_option_list, opt, next);
> 

This is why i don't like the "no brackets on single-statement if clause" 
rule in our coding style guide - it makes mistakes like these easier to 
do. This wouldn't have happened if we mandated to have brackets always, 
for everything.

Reviewed-by: Anatoly Burakov <anatoly.burakov@intel.com>

-- 
Thanks,
Anatoly

Re: [dpdk-dev] [PATCH] eal/option: fix option register duplicate detection

[David Marchand]

On Mon, Dec 17, 2018 at 10:26 AM Gaetan Rivet <gaetan.rivet@6wind.com>
wrote:

> Missing brackets around the if means that the loop will end at its first
> iteration.
>
> Cc: stable@dpdk.org
>
> Fixes: 2395332798d0 ("eal: add option register infrastructure")
> Signed-off-by: Gaetan Rivet <gaetan.rivet@6wind.com>
> ---
>  lib/librte_eal/common/rte_option.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/lib/librte_eal/common/rte_option.c
> b/lib/librte_eal/common/rte_option.c
> index 02d59a869..eefb8b923 100644
> --- a/lib/librte_eal/common/rte_option.c
> +++ b/lib/librte_eal/common/rte_option.c
> @@ -35,10 +35,11 @@ void __rte_experimental
>  rte_option_register(struct rte_option *opt)
>  {
>         TAILQ_FOREACH(option, &rte_option_list, next) {
> -               if (strcmp(opt->opt_str, option->opt_str) == 0)
> +               if (strcmp(opt->opt_str, option->opt_str) == 0) {
>                         RTE_LOG(INFO, EAL, "Option %s has already been
> registered.",
>                                         opt->opt_str);
>                         return;
> +               }
>         }
>
>         TAILQ_INSERT_HEAD(&rte_option_list, opt, next);
>

Reviewed-by: David Marchand <david.marchand@redhat.com>

Different topic but having a return code would be better than a simple log.

-- 
David Marchand

[dpdk-dev] [PATCH] mk: use misleading indentation warning when available

[Gaetan Rivet]

-Wmisleading-indentation was introduced in GCC 6.0.

Use it at least when available. This should catch most common
error of the types (due to the codebase being properly tabbed),
but will still miss patterns such as

    if (!condition)
    	// commented_fn_call();
    do_stuff();

Signed-off-by: Gaetan Rivet <gaetan.rivet@6wind.com>
---

I completely agree that we should change the coding style and force
all if()s to have brackets.

In the meantime, this patch might help alleviate the issue.

 mk/toolchain/gcc/rte.vars.mk | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/mk/toolchain/gcc/rte.vars.mk b/mk/toolchain/gcc/rte.vars.mk
index d8b99faf6..2c9bde1d5 100644
--- a/mk/toolchain/gcc/rte.vars.mk
+++ b/mk/toolchain/gcc/rte.vars.mk
@@ -87,5 +87,9 @@ WERROR_FLAGS += -Wimplicit-fallthrough=2
 WERROR_FLAGS += -Wno-format-truncation
 endif
 
+ifeq ($(shell test $(GCC_VERSION) -gt 60 && echo 1), 1)
+WERROR_FLAGS += -Wmisleading-indentation
+endif
+
 export CC AS AR LD OBJCOPY OBJDUMP STRIP READELF
 export TOOLCHAIN_CFLAGS TOOLCHAIN_LDFLAGS TOOLCHAIN_ASFLAGS
-- 
2.19.1

Re: [dpdk-dev] [PATCH] mk: use misleading indentation warning when available

[Ferruh Yigit]

On 12/18/2018 10:26 AM, Gaetan Rivet wrote:
> -Wmisleading-indentation was introduced in GCC 6.0.

It seems '-Wmisleading-indentation' is part of -Wall, which we already set by
default. If so no need to explicitly add it.

The link I found:
https://www.gnu.org/software/gcc/gcc-6/porting_to.html

copy-paste:
"
A new warning -Wmisleading-indentation was added to -Wall, warning about places
where the indentation of the code might mislead a human reader about the control
flow:
"

Is there a way to confirm it is part of -Wall?

> 
> Use it at least when available. This should catch most common
> error of the types (due to the codebase being properly tabbed),
> but will still miss patterns such as
> 
>     if (!condition)
>     	// commented_fn_call();
>     do_stuff();
> 
> Signed-off-by: Gaetan Rivet <gaetan.rivet@6wind.com>
> ---
> 
> I completely agree that we should change the coding style and force
> all if()s to have brackets.
> 
> In the meantime, this patch might help alleviate the issue.
> 
>  mk/toolchain/gcc/rte.vars.mk | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/mk/toolchain/gcc/rte.vars.mk b/mk/toolchain/gcc/rte.vars.mk
> index d8b99faf6..2c9bde1d5 100644
> --- a/mk/toolchain/gcc/rte.vars.mk
> +++ b/mk/toolchain/gcc/rte.vars.mk
> @@ -87,5 +87,9 @@ WERROR_FLAGS += -Wimplicit-fallthrough=2
>  WERROR_FLAGS += -Wno-format-truncation
>  endif
>  
> +ifeq ($(shell test $(GCC_VERSION) -gt 60 && echo 1), 1)
> +WERROR_FLAGS += -Wmisleading-indentation
> +endif
> +
>  export CC AS AR LD OBJCOPY OBJDUMP STRIP READELF
>  export TOOLCHAIN_CFLAGS TOOLCHAIN_LDFLAGS TOOLCHAIN_ASFLAGS
> 

Re: [dpdk-dev] [PATCH] mk: use misleading indentation warning when available

[Gaëtan Rivet]

On Tue, Dec 18, 2018 at 02:50:30PM +0000, Ferruh Yigit wrote:
> On 12/18/2018 10:26 AM, Gaetan Rivet wrote:
> > -Wmisleading-indentation was introduced in GCC 6.0.
> 
> It seems '-Wmisleading-indentation' is part of -Wall, which we already set by
> default. If so no need to explicitly add it.
> 
> The link I found:
> https://www.gnu.org/software/gcc/gcc-6/porting_to.html
> 
> copy-paste:
> "
> A new warning -Wmisleading-indentation was added to -Wall, warning about places
> where the indentation of the code might mislead a human reader about the control
> flow:
> "
> 
> Is there a way to confirm it is part of -Wall?
> 

I think you are right, actually the check was already used.
This is worrying, given that the original bug was not seen.

This patch can be left out then, but the problem remains. Maybe an
update to coding style is needed, or an evolution to checkpatch //
preferably something else.

> > 
> > Use it at least when available. This should catch most common
> > error of the types (due to the codebase being properly tabbed),
> > but will still miss patterns such as
> > 
> >     if (!condition)
> >     	// commented_fn_call();
> >     do_stuff();
> > 
> > Signed-off-by: Gaetan Rivet <gaetan.rivet@6wind.com>
> > ---
> > 
> > I completely agree that we should change the coding style and force
> > all if()s to have brackets.
> > 
> > In the meantime, this patch might help alleviate the issue.
> > 
> >  mk/toolchain/gcc/rte.vars.mk | 4 ++++
> >  1 file changed, 4 insertions(+)
> > 
> > diff --git a/mk/toolchain/gcc/rte.vars.mk b/mk/toolchain/gcc/rte.vars.mk
> > index d8b99faf6..2c9bde1d5 100644
> > --- a/mk/toolchain/gcc/rte.vars.mk
> > +++ b/mk/toolchain/gcc/rte.vars.mk
> > @@ -87,5 +87,9 @@ WERROR_FLAGS += -Wimplicit-fallthrough=2
> >  WERROR_FLAGS += -Wno-format-truncation
> >  endif
> >  
> > +ifeq ($(shell test $(GCC_VERSION) -gt 60 && echo 1), 1)
> > +WERROR_FLAGS += -Wmisleading-indentation
> > +endif
> > +
> >  export CC AS AR LD OBJCOPY OBJDUMP STRIP READELF
> >  export TOOLCHAIN_CFLAGS TOOLCHAIN_LDFLAGS TOOLCHAIN_ASFLAGS
> > 
> 

-- 
Gaëtan Rivet
6WIND

Re: [dpdk-dev] [PATCH] mk: use misleading indentation warning when available

[Burakov, Anatoly]

On 19-Dec-18 9:24 AM, Gaëtan Rivet wrote:
> On Tue, Dec 18, 2018 at 02:50:30PM +0000, Ferruh Yigit wrote:
>> On 12/18/2018 10:26 AM, Gaetan Rivet wrote:
>>> -Wmisleading-indentation was introduced in GCC 6.0.
>>
>> It seems '-Wmisleading-indentation' is part of -Wall, which we already set by
>> default. If so no need to explicitly add it.
>>
>> The link I found:
>> https://www.gnu.org/software/gcc/gcc-6/porting_to.html
>>
>> copy-paste:
>> "
>> A new warning -Wmisleading-indentation was added to -Wall, warning about places
>> where the indentation of the code might mislead a human reader about the control
>> flow:
>> "
>>
>> Is there a way to confirm it is part of -Wall?
>>
> 
> I think you are right, actually the check was already used.
> This is worrying, given that the original bug was not seen.
> 
> This patch can be left out then, but the problem remains. Maybe an
> update to coding style is needed, or an evolution to checkpatch //
> preferably something else.

clang-format? It's not exactly a style *checker*, but if we had a 
.clang-format config file included with DPDK, it would be easy to 1) 
format the code properly before sending out the patches, and 2) convert 
the entire DPDK codebase to the new style, should such need arise.

(also, uncrustify and other code beautifiers...)

-- 
Thanks,
Anatoly

Re: [dpdk-dev] [PATCH] eal/option: fix option register duplicate detection

[Thomas Monjalon]

17/12/2018 11:19, David Marchand:
> On Mon, Dec 17, 2018 at 10:26 AM Gaetan Rivet <gaetan.rivet@6wind.com>
> wrote:
> 
> > Missing brackets around the if means that the loop will end at its first
> > iteration.
> >
> > Cc: stable@dpdk.org
> >
> > Fixes: 2395332798d0 ("eal: add option register infrastructure")
> > Signed-off-by: Gaetan Rivet <gaetan.rivet@6wind.com>
> > ---
> >  rte_option_register(struct rte_option *opt)
> >  {
> >         TAILQ_FOREACH(option, &rte_option_list, next) {
> > -               if (strcmp(opt->opt_str, option->opt_str) == 0)
> > +               if (strcmp(opt->opt_str, option->opt_str) == 0) {
> >                         RTE_LOG(INFO, EAL, "Option %s has already been
> > registered.",
> >                                         opt->opt_str);
> >                         return;
> > +               }
> >         }
> 
> Reviewed-by: David Marchand <david.marchand@redhat.com>

Applied, thanks

> Different topic but having a return code would be better than a simple log.

+1