| Message ID | b488b997e1b966f6d35a2767da738fc9a44023a8.1721903630.git.tony.ambardar@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | selftests/bpf: Improve libc portability / musl support (part 2) | expand |
On Thu, Jul 25, 2024 at 3:39 AM Tony Ambardar <tony.ambardar@gmail.com> wrote: > > From: Tony Ambardar <tony.ambardar@gmail.com> > > Current code parses arguments with strtok_r() using a construct like > > char *state = NULL; > while ((next = strtok_r(state ? NULL : input, ",", &state))) { > ... > } > > where logic assumes the 'state' var can distinguish between first and > subsequent strtok_r() calls, and adjusts parameters accordingly. However, > 'state' is strictly internal context for strtok_r() and no such assumptions > are supported in the man page. Moreover, the exact behaviour of 'state' > depends on the libc implementation, making the above code fragile. > > Indeed, invoking "./test_progs -t <test_name>" on mips64el/musl will hang, > with the above code in an infinite loop. > > Similarly, we see strange behaviour running 'veristat' on mips64el/musl: > > $ ./veristat -e file,prog,verdict,insns -C two-ok add-failure > Can't specify more than 9 stats > > Rewrite code using a 'for' loop without logic dependent on var 'state', the > same approach already used in cgroup_helpers.c. > > Fixes: 61ddff373ffa ("selftests/bpf: Improve by-name subtest selection logic in prog_tests") > Fixes: 394169b079b5 ("selftests/bpf: add comparison mode to veristat") > Fixes: c8bc5e050976 ("selftests/bpf: Add veristat tool for mass-verifying BPF object files") > Signed-off-by: Tony Ambardar <tony.ambardar@gmail.com> > --- > tools/testing/selftests/bpf/testing_helpers.c | 2 +- > tools/testing/selftests/bpf/veristat.c | 4 ++-- > 2 files changed, 3 insertions(+), 3 deletions(-) > > diff --git a/tools/testing/selftests/bpf/testing_helpers.c b/tools/testing/selftests/bpf/testing_helpers.c > index ac7c66f4fc7b..2a73b72feb18 100644 > --- a/tools/testing/selftests/bpf/testing_helpers.c > +++ b/tools/testing/selftests/bpf/testing_helpers.c > @@ -227,7 +227,7 @@ int parse_test_list(const char *s, > if (!input) > return -ENOMEM; > > - while ((test_spec = strtok_r(state ? NULL : input, ",", &state))) { > + for (test_spec = strtok_r(input, ",", &state); test_spec; test_spec = strtok_r(NULL, ",", &state)) { oh, this is so long and verbose, let's just add a counter and use that to determine whether to pass NULL or input, ok? > err = insert_test(set, test_spec, is_glob_pattern); > if (err) > break; > diff --git a/tools/testing/selftests/bpf/veristat.c b/tools/testing/selftests/bpf/veristat.c > index 11ec1190d582..6808679827ac 100644 > --- a/tools/testing/selftests/bpf/veristat.c > +++ b/tools/testing/selftests/bpf/veristat.c > @@ -791,7 +791,7 @@ static int parse_stats(const char *stats_str, struct stat_specs *specs) > if (!input) > return -ENOMEM; > > - while ((next = strtok_r(state ? NULL : input, ",", &state))) { > + for (next = strtok_r(input, ",", &state); next; next = strtok_r(NULL, ",", &state)) { ditto, let's not duplicate strtok_r() calls > err = parse_stat(next, specs); > if (err) { > free(input); > @@ -1513,7 +1513,7 @@ static int parse_stats_csv(const char *filename, struct stat_specs *specs, > *stat_cntp += 1; > } > > - while ((next = strtok_r(state ? NULL : input, ",\n", &state))) { > + for (next = strtok_r(input, ",\n", &state); next; next = strtok_r(NULL, ",\n", &state)) { > if (header) { > /* for the first line, set up spec stats */ > err = parse_stat(next, specs); > -- > 2.34.1 >
On Thu, Jul 25, 2024 at 01:09:24PM -0700, Andrii Nakryiko wrote: > On Thu, Jul 25, 2024 at 3:39 AM Tony Ambardar <tony.ambardar@gmail.com> wrote: > > ... > > --- a/tools/testing/selftests/bpf/testing_helpers.c > > +++ b/tools/testing/selftests/bpf/testing_helpers.c > > @@ -227,7 +227,7 @@ int parse_test_list(const char *s, > > if (!input) > > return -ENOMEM; > > > > - while ((test_spec = strtok_r(state ? NULL : input, ",", &state))) { > > + for (test_spec = strtok_r(input, ",", &state); test_spec; test_spec = strtok_r(NULL, ",", &state)) { > > oh, this is so long and verbose, let's just add a counter and use that > to determine whether to pass NULL or input, ok? > ... > > - while ((next = strtok_r(state ? NULL : input, ",", &state))) { > > + for (next = strtok_r(input, ",", &state); next; next = strtok_r(NULL, ",", &state)) { > > ditto, let's not duplicate strtok_r() calls > Sounds good. I'll update for v2 and thanks for the suggestion. ...
diff --git a/tools/testing/selftests/bpf/testing_helpers.c b/tools/testing/selftests/bpf/testing_helpers.c index ac7c66f4fc7b..2a73b72feb18 100644 --- a/tools/testing/selftests/bpf/testing_helpers.c +++ b/tools/testing/selftests/bpf/testing_helpers.c @@ -227,7 +227,7 @@ int parse_test_list(const char *s, if (!input) return -ENOMEM; - while ((test_spec = strtok_r(state ? NULL : input, ",", &state))) { + for (test_spec = strtok_r(input, ",", &state); test_spec; test_spec = strtok_r(NULL, ",", &state)) { err = insert_test(set, test_spec, is_glob_pattern); if (err) break; diff --git a/tools/testing/selftests/bpf/veristat.c b/tools/testing/selftests/bpf/veristat.c index 11ec1190d582..6808679827ac 100644 --- a/tools/testing/selftests/bpf/veristat.c +++ b/tools/testing/selftests/bpf/veristat.c @@ -791,7 +791,7 @@ static int parse_stats(const char *stats_str, struct stat_specs *specs) if (!input) return -ENOMEM; - while ((next = strtok_r(state ? NULL : input, ",", &state))) { + for (next = strtok_r(input, ",", &state); next; next = strtok_r(NULL, ",", &state)) { err = parse_stat(next, specs); if (err) { free(input); @@ -1513,7 +1513,7 @@ static int parse_stats_csv(const char *filename, struct stat_specs *specs, *stat_cntp += 1; } - while ((next = strtok_r(state ? NULL : input, ",\n", &state))) { + for (next = strtok_r(input, ",\n", &state); next; next = strtok_r(NULL, ",\n", &state)) { if (header) { /* for the first line, set up spec stats */ err = parse_stat(next, specs);