[PATCH] perf config: Refine error message to eliminate confusion

[PATCH] perf config: Refine error message to eliminate confusion

[Like Xu]

From: Like Xu <likexu@tencent.com>

If there is no configuration file at first, the user can
write any pair of "key.subkey=value" to the newly created
configuration file, while value validation against a valid
configurable key is *deferred* until the next execution or
the implied execution of "perf config ... ".

For example:

 $ rm ~/.perfconfig
 $ perf config call-graph.dump-size=65529
 $ cat ~/.perfconfig
 # this file is auto-generated.
 [call-graph]
 	dump-size = 65529
 $ perf config call-graph.dump-size=2048
 callchain: Incorrect stack dump size (max 65528): 65529
 Error: wrong config key-value pair call-graph.dump-size=65529

The user might expect that the second value 2048 is valid
and can be updated to the configuration file, but the error
message is very confusing because the first value 65529 is
not reported as an error during the last configuration.

It is recommended not to change the current behavior of
delayed validation (as more effort is needed), but to refine
the original error message to *clearly indicate* that the
cause of the error is the configuration file.

Signed-off-by: Like Xu <likexu@tencent.com>
---
 tools/perf/util/config.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/perf/util/config.c b/tools/perf/util/config.c
index 4fb5e90d7a57..60ce5908c664 100644
--- a/tools/perf/util/config.c
+++ b/tools/perf/util/config.c
@@ -801,7 +801,7 @@ int perf_config_set(struct perf_config_set *set,
 				  section->name, item->name);
 			ret = fn(key, value, data);
 			if (ret < 0) {
-				pr_err("Error: wrong config key-value pair %s=%s\n",
+				pr_err("Error in the given config file: wrong config key-value pair %s=%s\n",
 				       key, value);
 				/*
 				 * Can't be just a 'break', as perf_config_set__for_each_entry()
-- 
2.32.0

Re: [PATCH] perf config: Refine error message to eliminate confusion

[Ian Rogers]

On Fri, Sep 24, 2021 at 4:58 AM Like Xu <like.xu.linux@gmail.com> wrote:
>
> From: Like Xu <likexu@tencent.com>
>
> If there is no configuration file at first, the user can
> write any pair of "key.subkey=value" to the newly created
> configuration file, while value validation against a valid
> configurable key is *deferred* until the next execution or
> the implied execution of "perf config ... ".
>
> For example:
>
>  $ rm ~/.perfconfig
>  $ perf config call-graph.dump-size=65529
>  $ cat ~/.perfconfig
>  # this file is auto-generated.
>  [call-graph]
>         dump-size = 65529
>  $ perf config call-graph.dump-size=2048
>  callchain: Incorrect stack dump size (max 65528): 65529
>  Error: wrong config key-value pair call-graph.dump-size=65529
>
> The user might expect that the second value 2048 is valid
> and can be updated to the configuration file, but the error
> message is very confusing because the first value 65529 is
> not reported as an error during the last configuration.
>
> It is recommended not to change the current behavior of
> delayed validation (as more effort is needed), but to refine
> the original error message to *clearly indicate* that the
> cause of the error is the configuration file.
>
> Signed-off-by: Like Xu <likexu@tencent.com>

Acked-by: Ian Rogers <irogers@google.com>

Thanks,
Ian

> ---
>  tools/perf/util/config.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/tools/perf/util/config.c b/tools/perf/util/config.c
> index 4fb5e90d7a57..60ce5908c664 100644
> --- a/tools/perf/util/config.c
> +++ b/tools/perf/util/config.c
> @@ -801,7 +801,7 @@ int perf_config_set(struct perf_config_set *set,
>                                   section->name, item->name);
>                         ret = fn(key, value, data);
>                         if (ret < 0) {
> -                               pr_err("Error: wrong config key-value pair %s=%s\n",
> +                               pr_err("Error in the given config file: wrong config key-value pair %s=%s\n",
>                                        key, value);
>                                 /*
>                                  * Can't be just a 'break', as perf_config_set__for_each_entry()
> --
> 2.32.0
>

Re: [PATCH] perf config: Refine error message to eliminate confusion

[Arnaldo Carvalho de Melo]

Em Fri, Sep 24, 2021 at 08:08:07AM -0700, Ian Rogers escreveu:
> On Fri, Sep 24, 2021 at 4:58 AM Like Xu <like.xu.linux@gmail.com> wrote:
> >
> > From: Like Xu <likexu@tencent.com>
> >
> > If there is no configuration file at first, the user can
> > write any pair of "key.subkey=value" to the newly created
> > configuration file, while value validation against a valid
> > configurable key is *deferred* until the next execution or
> > the implied execution of "perf config ... ".
> >
> > For example:
> >
> >  $ rm ~/.perfconfig
> >  $ perf config call-graph.dump-size=65529
> >  $ cat ~/.perfconfig
> >  # this file is auto-generated.
> >  [call-graph]
> >         dump-size = 65529
> >  $ perf config call-graph.dump-size=2048
> >  callchain: Incorrect stack dump size (max 65528): 65529
> >  Error: wrong config key-value pair call-graph.dump-size=65529
> >
> > The user might expect that the second value 2048 is valid
> > and can be updated to the configuration file, but the error
> > message is very confusing because the first value 65529 is
> > not reported as an error during the last configuration.
> >
> > It is recommended not to change the current behavior of
> > delayed validation (as more effort is needed), but to refine
> > the original error message to *clearly indicate* that the
> > cause of the error is the configuration file.
> >
> > Signed-off-by: Like Xu <likexu@tencent.com>
> 
> Acked-by: Ian Rogers <irogers@google.com>

Thanks, applied.

- Arnaldo

 
> Thanks,
> Ian
> 
> > ---
> >  tools/perf/util/config.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/tools/perf/util/config.c b/tools/perf/util/config.c
> > index 4fb5e90d7a57..60ce5908c664 100644
> > --- a/tools/perf/util/config.c
> > +++ b/tools/perf/util/config.c
> > @@ -801,7 +801,7 @@ int perf_config_set(struct perf_config_set *set,
> >                                   section->name, item->name);
> >                         ret = fn(key, value, data);
> >                         if (ret < 0) {
> > -                               pr_err("Error: wrong config key-value pair %s=%s\n",
> > +                               pr_err("Error in the given config file: wrong config key-value pair %s=%s\n",
> >                                        key, value);
> >                                 /*
> >                                  * Can't be just a 'break', as perf_config_set__for_each_entry()
> > --
> > 2.32.0
> >

-- 

- Arnaldo