Re: [PATCH bpf-next v1] bpf/selftests: improve arg parsing in test_verifier

[Jiri Olsa <olsajiri@gmail.com>]

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