| Message ID | 20181217092559.21801-1-gaetan.rivet@6wind.com (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Delegated to: | Thomas Monjalon |
| Headers | show |
| Series | eal/option: fix option register duplicate detection | expand |
| Context | Check | Description |
|---|---|---|
| ci/checkpatch | success | coding style OK |
| ci/Intel-compilation | success | Compilation OK |
| ci/mellanox-Performance-Testing | success | Performance Testing PASS |
| ci/intel-Performance-Testing | success | Performance Testing PASS |
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>
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.
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
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);
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(-)