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

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