| Message ID | 20230627181030.95608-14-irogers@google.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | parse-events clean up | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Not a local patch |
On Tue, Jun 27, 2023 at 11:11 AM Ian Rogers <irogers@google.com> wrote: > > Prefer informative messages rather than none with ABORT_ON. Document > one failure mode and add an error message for another. > > Signed-off-by: Ian Rogers <irogers@google.com> > --- > tools/perf/util/parse-events.y | 22 ++++++++++++++-------- > 1 file changed, 14 insertions(+), 8 deletions(-) > > diff --git a/tools/perf/util/parse-events.y b/tools/perf/util/parse-events.y > index 844646752462..454577f7aff6 100644 > --- a/tools/perf/util/parse-events.y > +++ b/tools/perf/util/parse-events.y > @@ -22,12 +22,6 @@ > > void parse_events_error(YYLTYPE *loc, void *parse_state, void *scanner, char const *msg); > > -#define ABORT_ON(val) \ > -do { \ > - if (val) \ > - YYABORT; \ > -} while (0) > - > #define PE_ABORT(val) \ > do { \ > if (val == -ENOMEM) \ > @@ -618,7 +612,9 @@ PE_RAW opt_event_config > YYNOMEM; > errno = 0; > num = strtoull($1 + 1, NULL, 16); > - ABORT_ON(errno); > + /* Given the lexer will only give [a-fA-F0-9]+ a failure here should be impossible. */ > + if (errno) > + YYABORT; > free($1); > err = parse_events_add_numeric(_parse_state, list, PERF_TYPE_RAW, num, $2, > /*wildcard=*/false); > @@ -978,7 +974,17 @@ PE_VALUE PE_ARRAY_RANGE PE_VALUE > { > struct parse_events_array array; > > - ABORT_ON($3 < $1); > + if ($3 < $1) { > + struct parse_events_state *parse_state = _parse_state; > + struct parse_events_error *error = parse_state->error; > + char *err_str; > + > + if (asprintf(&err_str, "Expected '%ld' to be less-than '%ld'", $3, $1) < 0) Isn't it to be "greater-than or equal" ? Thanks, Namhyung > + err_str = NULL; > + > + parse_events_error__handle(error, @1.first_column, err_str, NULL); > + YYABORT; > + } > array.nr_ranges = 1; > array.ranges = malloc(sizeof(array.ranges[0])); > if (!array.ranges) > -- > 2.41.0.162.gfafddb0af9-goog >
On Thu, Jun 29, 2023 at 2:49 PM Namhyung Kim <namhyung@kernel.org> wrote: > > On Tue, Jun 27, 2023 at 11:11 AM Ian Rogers <irogers@google.com> wrote: > > > > Prefer informative messages rather than none with ABORT_ON. Document > > one failure mode and add an error message for another. > > > > Signed-off-by: Ian Rogers <irogers@google.com> > > --- > > tools/perf/util/parse-events.y | 22 ++++++++++++++-------- > > 1 file changed, 14 insertions(+), 8 deletions(-) > > > > diff --git a/tools/perf/util/parse-events.y b/tools/perf/util/parse-events.y > > index 844646752462..454577f7aff6 100644 > > --- a/tools/perf/util/parse-events.y > > +++ b/tools/perf/util/parse-events.y > > @@ -22,12 +22,6 @@ > > > > void parse_events_error(YYLTYPE *loc, void *parse_state, void *scanner, char const *msg); > > > > -#define ABORT_ON(val) \ > > -do { \ > > - if (val) \ > > - YYABORT; \ > > -} while (0) > > - > > #define PE_ABORT(val) \ > > do { \ > > if (val == -ENOMEM) \ > > @@ -618,7 +612,9 @@ PE_RAW opt_event_config > > YYNOMEM; > > errno = 0; > > num = strtoull($1 + 1, NULL, 16); > > - ABORT_ON(errno); > > + /* Given the lexer will only give [a-fA-F0-9]+ a failure here should be impossible. */ > > + if (errno) > > + YYABORT; > > free($1); > > err = parse_events_add_numeric(_parse_state, list, PERF_TYPE_RAW, num, $2, > > /*wildcard=*/false); > > @@ -978,7 +974,17 @@ PE_VALUE PE_ARRAY_RANGE PE_VALUE > > { > > struct parse_events_array array; > > > > - ABORT_ON($3 < $1); > > + if ($3 < $1) { > > + struct parse_events_state *parse_state = _parse_state; > > + struct parse_events_error *error = parse_state->error; > > + char *err_str; > > + > > + if (asprintf(&err_str, "Expected '%ld' to be less-than '%ld'", $3, $1) < 0) > > Isn't it to be "greater-than or equal" ? I think the order is right. From the man page: When successful, these functions return the number of bytes printed, just like sprintf(3). If memory allocation wasn't possible, or some other error occurs, these functions will return -1, and the contents of strp are undefined. So here we need to catch -1 and ensure strp (&err_str) is NULL before passing it to parse_events_error__handle. Thanks, Ian > Thanks, > Namhyung > > > > + err_str = NULL; > > + > > + parse_events_error__handle(error, @1.first_column, err_str, NULL); > > + YYABORT; > > + } > > array.nr_ranges = 1; > > array.ranges = malloc(sizeof(array.ranges[0])); > > if (!array.ranges) > > -- > > 2.41.0.162.gfafddb0af9-goog > >
> Prefer informative messages rather than none with ABORT_ON. Document > one failure mode and add an error message for another. Does such a wording really fit to the known requirement “Solve only one problem per patch.”? See also: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.4#n81 Regards, Markus
On Fri, Jun 30, 2023 at 9:56 AM Markus Elfring <Markus.Elfring@web.de> wrote: > > > Prefer informative messages rather than none with ABORT_ON. Document > > one failure mode and add an error message for another. > > Does such a wording really fit to the known requirement “Solve only one problem per patch.”? > > See also: > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.4#n81 Sorry your explanation isn't clear. Please can you elaborate. Thanks, Ian > > Regards, > Markus
>>> Prefer informative messages rather than none with ABORT_ON. Document >>> one failure mode and add an error message for another. >> >> Does such a wording really fit to the known requirement “Solve only one problem per patch.”? >> >> See also: >> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.4#n81 > > Sorry your explanation isn't clear. Do you really find the application of the linked development documentation unclear in this case? > Sorry your explanation isn't clear. Please can you elaborate. Will it become helpful to split the proposed patch into smaller update steps? Regards, Markus
On Fri, Jun 30, 2023 at 10:40 AM Markus Elfring <Markus.Elfring@web.de> wrote: > > >>> Prefer informative messages rather than none with ABORT_ON. Document > >>> one failure mode and add an error message for another. > >> > >> Does such a wording really fit to the known requirement “Solve only one problem per patch.”? > >> > >> See also: > >> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.4#n81 > > > > Sorry your explanation isn't clear. > > Do you really find the application of the linked development documentation unclear > in this case? > > > > Sorry your explanation isn't clear. Please can you elaborate. > > Will it become helpful to split the proposed patch into smaller update steps? This is kind of why the series is 13 patches long, I'm not seeing why you think the following stats qualify as "long": 14 insertions(+), 8 deletions(-) Thanks, Ian > Regards, > Markus
>> Will it become helpful to split the proposed patch into smaller update steps? > > This is kind of why the series is 13 patches long, I'm not seeing why > you think the following stats qualify as "long": It seems that we came along different expectations for a desirable change granularity. Intentions influence how known “code problems” can be adjusted (also for this update step). How should following change ideas be handled then? 1. Deletion of the macro “ABORT_ON” 2. Addition of a comment for a special check 3. Introduction of another error message for one failure mode Would you like to adjust the change description another bit? Regards, Markus
On Sat, Jul 01, 2023 at 11:00:15AM +0200, Markus Elfring wrote: > >> Will it become helpful to split the proposed patch into smaller update steps? > > > > This is kind of why the series is 13 patches long, I'm not seeing why > > you think the following stats qualify as "long": > > It seems that we came along different expectations for a desirable change granularity. > Intentions influence how known “code problems” can be adjusted (also for this update step). > > How should following change ideas be handled then? > > 1. Deletion of the macro “ABORT_ON” > > 2. Addition of a comment for a special check > > 3. Introduction of another error message for one failure mode > > > Would you like to adjust the change description another bit? Please no, it's fine.
On Fri, Jun 30, 2023 at 8:14 AM Ian Rogers <irogers@google.com> wrote: > > On Thu, Jun 29, 2023 at 2:49 PM Namhyung Kim <namhyung@kernel.org> wrote: > > > > On Tue, Jun 27, 2023 at 11:11 AM Ian Rogers <irogers@google.com> wrote: > > > > > > Prefer informative messages rather than none with ABORT_ON. Document > > > one failure mode and add an error message for another. > > > > > > Signed-off-by: Ian Rogers <irogers@google.com> > > > --- > > > tools/perf/util/parse-events.y | 22 ++++++++++++++-------- > > > 1 file changed, 14 insertions(+), 8 deletions(-) > > > > > > diff --git a/tools/perf/util/parse-events.y b/tools/perf/util/parse-events.y > > > index 844646752462..454577f7aff6 100644 > > > --- a/tools/perf/util/parse-events.y > > > +++ b/tools/perf/util/parse-events.y > > > @@ -22,12 +22,6 @@ > > > > > > void parse_events_error(YYLTYPE *loc, void *parse_state, void *scanner, char const *msg); > > > > > > -#define ABORT_ON(val) \ > > > -do { \ > > > - if (val) \ > > > - YYABORT; \ > > > -} while (0) > > > - > > > #define PE_ABORT(val) \ > > > do { \ > > > if (val == -ENOMEM) \ > > > @@ -618,7 +612,9 @@ PE_RAW opt_event_config > > > YYNOMEM; > > > errno = 0; > > > num = strtoull($1 + 1, NULL, 16); > > > - ABORT_ON(errno); > > > + /* Given the lexer will only give [a-fA-F0-9]+ a failure here should be impossible. */ > > > + if (errno) > > > + YYABORT; > > > free($1); > > > err = parse_events_add_numeric(_parse_state, list, PERF_TYPE_RAW, num, $2, > > > /*wildcard=*/false); > > > @@ -978,7 +974,17 @@ PE_VALUE PE_ARRAY_RANGE PE_VALUE > > > { > > > struct parse_events_array array; > > > > > > - ABORT_ON($3 < $1); > > > + if ($3 < $1) { > > > + struct parse_events_state *parse_state = _parse_state; > > > + struct parse_events_error *error = parse_state->error; > > > + char *err_str; > > > + > > > + if (asprintf(&err_str, "Expected '%ld' to be less-than '%ld'", $3, $1) < 0) > > > > Isn't it to be "greater-than or equal" ? > > I think the order is right. From the man page: > > When successful, these functions return the number of bytes printed, > just like sprintf(3). If memory allocation wasn't possible, or some > other error occurs, these functions will return -1, and the contents of > strp are undefined. > > So here we need to catch -1 and ensure strp (&err_str) is NULL before > passing it to parse_events_error__handle. Oh, I meant the message not the condition in the if statement. It seems it aborts if $3 < $1, then it expects $3 >= $1 in the normal condition, right? Thanks, Namhyung
On Sat, Jul 1, 2023 at 11:43 AM Namhyung Kim <namhyung@kernel.org> wrote: > > On Fri, Jun 30, 2023 at 8:14 AM Ian Rogers <irogers@google.com> wrote: > > > > On Thu, Jun 29, 2023 at 2:49 PM Namhyung Kim <namhyung@kernel.org> wrote: > > > > > > On Tue, Jun 27, 2023 at 11:11 AM Ian Rogers <irogers@google.com> wrote: > > > > > > > > Prefer informative messages rather than none with ABORT_ON. Document > > > > one failure mode and add an error message for another. > > > > > > > > Signed-off-by: Ian Rogers <irogers@google.com> > > > > --- > > > > tools/perf/util/parse-events.y | 22 ++++++++++++++-------- > > > > 1 file changed, 14 insertions(+), 8 deletions(-) > > > > > > > > diff --git a/tools/perf/util/parse-events.y b/tools/perf/util/parse-events.y > > > > index 844646752462..454577f7aff6 100644 > > > > --- a/tools/perf/util/parse-events.y > > > > +++ b/tools/perf/util/parse-events.y > > > > @@ -22,12 +22,6 @@ > > > > > > > > void parse_events_error(YYLTYPE *loc, void *parse_state, void *scanner, char const *msg); > > > > > > > > -#define ABORT_ON(val) \ > > > > -do { \ > > > > - if (val) \ > > > > - YYABORT; \ > > > > -} while (0) > > > > - > > > > #define PE_ABORT(val) \ > > > > do { \ > > > > if (val == -ENOMEM) \ > > > > @@ -618,7 +612,9 @@ PE_RAW opt_event_config > > > > YYNOMEM; > > > > errno = 0; > > > > num = strtoull($1 + 1, NULL, 16); > > > > - ABORT_ON(errno); > > > > + /* Given the lexer will only give [a-fA-F0-9]+ a failure here should be impossible. */ > > > > + if (errno) > > > > + YYABORT; > > > > free($1); > > > > err = parse_events_add_numeric(_parse_state, list, PERF_TYPE_RAW, num, $2, > > > > /*wildcard=*/false); > > > > @@ -978,7 +974,17 @@ PE_VALUE PE_ARRAY_RANGE PE_VALUE > > > > { > > > > struct parse_events_array array; > > > > > > > > - ABORT_ON($3 < $1); > > > > + if ($3 < $1) { > > > > + struct parse_events_state *parse_state = _parse_state; > > > > + struct parse_events_error *error = parse_state->error; > > > > + char *err_str; > > > > + > > > > + if (asprintf(&err_str, "Expected '%ld' to be less-than '%ld'", $3, $1) < 0) > > > > > > Isn't it to be "greater-than or equal" ? > > > > I think the order is right. From the man page: > > > > When successful, these functions return the number of bytes printed, > > just like sprintf(3). If memory allocation wasn't possible, or some > > other error occurs, these functions will return -1, and the contents of > > strp are undefined. > > > > So here we need to catch -1 and ensure strp (&err_str) is NULL before > > passing it to parse_events_error__handle. > > Oh, I meant the message not the condition in the if statement. > It seems it aborts if $3 < $1, then it expects $3 >= $1 in the > normal condition, right? In the old code with the macro expanded it did: if ($3 < $1) YYABORT in the new code it fills in parse_state->error if the same error condition applies. The change is to get rid of the macro and add an error message. The asprintf is just added to make the error message more informative. Thanks, Ian > Thanks, > Namhyung
diff --git a/tools/perf/util/parse-events.y b/tools/perf/util/parse-events.y index 844646752462..454577f7aff6 100644 --- a/tools/perf/util/parse-events.y +++ b/tools/perf/util/parse-events.y @@ -22,12 +22,6 @@ void parse_events_error(YYLTYPE *loc, void *parse_state, void *scanner, char const *msg); -#define ABORT_ON(val) \ -do { \ - if (val) \ - YYABORT; \ -} while (0) - #define PE_ABORT(val) \ do { \ if (val == -ENOMEM) \ @@ -618,7 +612,9 @@ PE_RAW opt_event_config YYNOMEM; errno = 0; num = strtoull($1 + 1, NULL, 16); - ABORT_ON(errno); + /* Given the lexer will only give [a-fA-F0-9]+ a failure here should be impossible. */ + if (errno) + YYABORT; free($1); err = parse_events_add_numeric(_parse_state, list, PERF_TYPE_RAW, num, $2, /*wildcard=*/false); @@ -978,7 +974,17 @@ PE_VALUE PE_ARRAY_RANGE PE_VALUE { struct parse_events_array array; - ABORT_ON($3 < $1); + if ($3 < $1) { + struct parse_events_state *parse_state = _parse_state; + struct parse_events_error *error = parse_state->error; + char *err_str; + + if (asprintf(&err_str, "Expected '%ld' to be less-than '%ld'", $3, $1) < 0) + err_str = NULL; + + parse_events_error__handle(error, @1.first_column, err_str, NULL); + YYABORT; + } array.nr_ranges = 1; array.ranges = malloc(sizeof(array.ranges[0])); if (!array.ranges)
Prefer informative messages rather than none with ABORT_ON. Document one failure mode and add an error message for another. Signed-off-by: Ian Rogers <irogers@google.com> --- tools/perf/util/parse-events.y | 22 ++++++++++++++-------- 1 file changed, 14 insertions(+), 8 deletions(-)