| Message ID | 20230925233702.19466-1-Tony.Ambardar@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [bpf-next,v1] bpf/selftests: improve arg parsing in test_verifier | expand |
On Mon, Sep 25, 2023 at 04:37:02PM -0700, Tony Ambardar wrote: > Current test_verifier provides little feedback or argument validation, > instead silently falling back to running all tests in case of user error > or even expected use cases. Trying to do manual exploratory testing, > switching between kernel versions (e.g. with varying tests), or working > around problematic tests (e.g. kernel hangs/crashes) can be a frustrating > experience. > > Rework argument parsing to be more robust and strict, and provide basic > help on errors. Clamp test ranges to valid values and add an option to > list available built-in tests ("-l"). Default "test_verifier" behaviour > (run all tests) is unchanged and backwards-compatible. Updated examples: > > $ test_verifier die die die # previously ran all tests > Usage: test_verifier -l | [-v|-vv] [<tst_lo> [<tst_hi>]] > > $ test_verifier 700 9999 # runs test subset from 700 to end > > Signed-off-by: Tony Ambardar <Tony.Ambardar@gmail.com> > --- > tools/testing/selftests/bpf/test_verifier.c | 54 ++++++++++++--------- > 1 file changed, 30 insertions(+), 24 deletions(-) > > diff --git a/tools/testing/selftests/bpf/test_verifier.c b/tools/testing/selftests/bpf/test_verifier.c > index 98107e0452d3..3712b5363f60 100644 > --- a/tools/testing/selftests/bpf/test_verifier.c > +++ b/tools/testing/selftests/bpf/test_verifier.c > @@ -10,9 +10,11 @@ > #include <endian.h> > #include <asm/types.h> > #include <linux/types.h> > +#include <linux/minmax.h> this fails to compile BINARY test_verifier test_verifier.c:13:10: fatal error: linux/minmax.h: No such file or directory 13 | #include <linux/minmax.h> | ^~~~~~~~~~~~~~~~ looks like you could use perhaps <linux/kernel.h> instead? jirka > #include <stdint.h> > #include <stdio.h> > #include <stdlib.h> > +#include <ctype.h> > #include <unistd.h> > #include <errno.h> > #include <string.h> > @@ -1848,36 +1850,40 @@ int main(int argc, char **argv) > { > unsigned int from = 0, to = ARRAY_SIZE(tests); > bool unpriv = !is_admin(); > - int arg = 1; > - > - if (argc > 1 && strcmp(argv[1], "-v") == 0) { > + int i, arg = 1; > + > + while (argc > 1 && *argv[arg] == '-') { > + if (strcmp(argv[arg], "-l") == 0) { > + for (i = from; i < to; i++) > + printf("#%d %s\n", i, tests[i].descr); > + return EXIT_SUCCESS; > + } else if (strcmp(argv[arg], "-v") == 0) { > + verbose = true; > + verif_log_level = 1; > + } else if (strcmp(argv[arg], "-vv") == 0) { > + verbose = true; > + verif_log_level = 2; > + } else > + goto out_help; > arg++; > - verbose = true; > - verif_log_level = 1; > argc--; > } > - if (argc > 1 && strcmp(argv[1], "-vv") == 0) { > - arg++; > - verbose = true; > - verif_log_level = 2; > - argc--; > - } > - > - if (argc == 3) { > - unsigned int l = atoi(argv[arg]); > - unsigned int u = atoi(argv[arg + 1]); > > - if (l < to && u < to) { > - from = l; > - to = u + 1; > - } > - } else if (argc == 2) { > - unsigned int t = atoi(argv[arg]); > + for (i = 1; i <= 2 && argc > 1; i++, arg++, argc--) { > + unsigned int t = min(atoi(argv[arg]), ARRAY_SIZE(tests) - 1); > > - if (t < to) { > + if (!isdigit(*argv[arg])) > + goto out_help; > + if (i == 1) > from = t; > - to = t + 1; > - } > + to = t + 1; > + } > + > + if (argc > 1) { > +out_help: > + printf("Usage: %s -l | [-v|-vv] [<tst_lo> [<tst_hi>]]\n", > + argv[0]); > + return EXIT_FAILURE; > } > > unpriv_disabled = get_unpriv_disabled(); > -- > 2.34.1 > >
On Mon, Sep 25, 2023 at 04:37:02PM -0700, Tony Ambardar wrote: SNIP > @@ -1848,36 +1850,40 @@ int main(int argc, char **argv) > { > unsigned int from = 0, to = ARRAY_SIZE(tests); > bool unpriv = !is_admin(); > - int arg = 1; > - > - if (argc > 1 && strcmp(argv[1], "-v") == 0) { > + int i, arg = 1; > + > + while (argc > 1 && *argv[arg] == '-') { > + if (strcmp(argv[arg], "-l") == 0) { > + for (i = from; i < to; i++) > + printf("#%d %s\n", i, tests[i].descr); > + return EXIT_SUCCESS; > + } else if (strcmp(argv[arg], "-v") == 0) { > + verbose = true; > + verif_log_level = 1; > + } else if (strcmp(argv[arg], "-vv") == 0) { > + verbose = true; > + verif_log_level = 2; > + } else > + goto out_help; > arg++; > - verbose = true; > - verif_log_level = 1; > argc--; > } > - if (argc > 1 && strcmp(argv[1], "-vv") == 0) { > - arg++; > - verbose = true; > - verif_log_level = 2; > - argc--; > - } > - > - if (argc == 3) { > - unsigned int l = atoi(argv[arg]); > - unsigned int u = atoi(argv[arg + 1]); > > - if (l < to && u < to) { > - from = l; > - to = u + 1; > - } > - } else if (argc == 2) { > - unsigned int t = atoi(argv[arg]); > + for (i = 1; i <= 2 && argc > 1; i++, arg++, argc--) { > + unsigned int t = min(atoi(argv[arg]), ARRAY_SIZE(tests) - 1); this looks like unnecessary loop, the code before is easy to understand, could we just do the args check on isdigit and valid index value in there? jirka > > - if (t < to) { > + if (!isdigit(*argv[arg])) > + goto out_help; > + if (i == 1) > from = t; > - to = t + 1; > - } > + to = t + 1; > + } > + > + if (argc > 1) { > +out_help: > + printf("Usage: %s -l | [-v|-vv] [<tst_lo> [<tst_hi>]]\n", > + argv[0]); > + return EXIT_FAILURE; > } > > unpriv_disabled = get_unpriv_disabled(); > -- > 2.34.1 > >
diff --git a/tools/testing/selftests/bpf/test_verifier.c b/tools/testing/selftests/bpf/test_verifier.c index 98107e0452d3..3712b5363f60 100644 --- a/tools/testing/selftests/bpf/test_verifier.c +++ b/tools/testing/selftests/bpf/test_verifier.c @@ -10,9 +10,11 @@ #include <endian.h> #include <asm/types.h> #include <linux/types.h> +#include <linux/minmax.h> #include <stdint.h> #include <stdio.h> #include <stdlib.h> +#include <ctype.h> #include <unistd.h> #include <errno.h> #include <string.h> @@ -1848,36 +1850,40 @@ int main(int argc, char **argv) { unsigned int from = 0, to = ARRAY_SIZE(tests); bool unpriv = !is_admin(); - int arg = 1; - - if (argc > 1 && strcmp(argv[1], "-v") == 0) { + int i, arg = 1; + + while (argc > 1 && *argv[arg] == '-') { + if (strcmp(argv[arg], "-l") == 0) { + for (i = from; i < to; i++) + printf("#%d %s\n", i, tests[i].descr); + return EXIT_SUCCESS; + } else if (strcmp(argv[arg], "-v") == 0) { + verbose = true; + verif_log_level = 1; + } else if (strcmp(argv[arg], "-vv") == 0) { + verbose = true; + verif_log_level = 2; + } else + goto out_help; arg++; - verbose = true; - verif_log_level = 1; argc--; } - if (argc > 1 && strcmp(argv[1], "-vv") == 0) { - arg++; - verbose = true; - verif_log_level = 2; - argc--; - } - - if (argc == 3) { - unsigned int l = atoi(argv[arg]); - unsigned int u = atoi(argv[arg + 1]); - if (l < to && u < to) { - from = l; - to = u + 1; - } - } else if (argc == 2) { - unsigned int t = atoi(argv[arg]); + for (i = 1; i <= 2 && argc > 1; i++, arg++, argc--) { + unsigned int t = min(atoi(argv[arg]), ARRAY_SIZE(tests) - 1); - if (t < to) { + if (!isdigit(*argv[arg])) + goto out_help; + if (i == 1) from = t; - to = t + 1; - } + to = t + 1; + } + + if (argc > 1) { +out_help: + printf("Usage: %s -l | [-v|-vv] [<tst_lo> [<tst_hi>]]\n", + argv[0]); + return EXIT_FAILURE; } unpriv_disabled = get_unpriv_disabled();
Current test_verifier provides little feedback or argument validation, instead silently falling back to running all tests in case of user error or even expected use cases. Trying to do manual exploratory testing, switching between kernel versions (e.g. with varying tests), or working around problematic tests (e.g. kernel hangs/crashes) can be a frustrating experience. Rework argument parsing to be more robust and strict, and provide basic help on errors. Clamp test ranges to valid values and add an option to list available built-in tests ("-l"). Default "test_verifier" behaviour (run all tests) is unchanged and backwards-compatible. Updated examples: $ test_verifier die die die # previously ran all tests Usage: test_verifier -l | [-v|-vv] [<tst_lo> [<tst_hi>]] $ test_verifier 700 9999 # runs test subset from 700 to end Signed-off-by: Tony Ambardar <Tony.Ambardar@gmail.com> --- tools/testing/selftests/bpf/test_verifier.c | 54 ++++++++++++--------- 1 file changed, 30 insertions(+), 24 deletions(-)