diff mbox series

[bpf-next,v1,2/8] selftests/bpf: Fix arg parsing in veristat, test_progs

Message ID b488b997e1b966f6d35a2767da738fc9a44023a8.1721903630.git.tony.ambardar@gmail.com
State New
Headers show
Series selftests/bpf: Improve libc portability / musl support (part 2) | expand

Commit Message

Tony Ambardar July 25, 2024, 10:35 a.m. UTC 
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(-)

Comments

Tony Ambardar July 27, 2024, 3:34 a.m. UTC | #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 mbox series

Patch

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);