Date:   Mon, 31 Oct 2022 19:48:16 +0000
From:   Sean Christopherson <seanjc@...gle.com>
To:     Vipin Sharma <vipinsh@...gle.com>
Cc:     pbonzini@...hat.com, dmatlack@...gle.com, andrew.jones@...ux.dev,
        wei.w.wang@...el.com, kvm@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v7 4/5] KVM: selftests: Add atoi_positive() and
 atoi_non_negative() for input validation

On Mon, Oct 31, 2022, Vipin Sharma wrote:
> diff --git a/tools/testing/selftests/kvm/lib/test_util.c b/tools/testing/selftests/kvm/lib/test_util.c
> index ec0f070a6f21..210e98a49a83 100644
> --- a/tools/testing/selftests/kvm/lib/test_util.c
> +++ b/tools/testing/selftests/kvm/lib/test_util.c
> @@ -353,3 +353,19 @@ int atoi_paranoid(const char *num_str)
>  
>  	return num;
>  }
> +
> +uint32_t atoi_positive(const char *num_str)

I think it makes sense to inline atoi_positive() and atoi_non_negative() in
test_util.h.  Depending on developer's setups, it might be one less layer to jump
through to look at the implementation.

> +{
> +	int num = atoi_paranoid(num_str);
> +
> +	TEST_ASSERT(num > 0, "%s is not a positive integer.\n", num_str);

Newlines aren't needed in asserts.  This applies to atoi_paranoid() in the previous
patch as well (I initially missed them).

> +	return num;
> +}
> +
> +uint32_t atoi_non_negative(const char *num_str)
> +{
> +	int num = atoi_paranoid(num_str);
> +
> +	TEST_ASSERT(num >= 0, "%s is not a non-negative integer.\n", num_str);
> +	return num;
> +}
> diff --git a/tools/testing/selftests/kvm/max_guest_memory_test.c b/tools/testing/selftests/kvm/max_guest_memory_test.c
> index 1595b73dc09a..20015de3b91c 100644
> --- a/tools/testing/selftests/kvm/max_guest_memory_test.c
> +++ b/tools/testing/selftests/kvm/max_guest_memory_test.c
> @@ -193,15 +193,14 @@ int main(int argc, char *argv[])
>  	while ((opt = getopt(argc, argv, "c:h:m:s:H")) != -1) {
>  		switch (opt) {
>  		case 'c':
> -			nr_vcpus = atoi_paranoid(optarg);
> -			TEST_ASSERT(nr_vcpus > 0, "number of vcpus must be >0");
> +			nr_vcpus = atoi_positive(optarg);

I know I originally made the claim that the assert would provide enough context
to offest lack of a specific message, but after actually playing around with this,
past me was wrong.  E.g. this

  Memory size must be greater than 0, got '-1'

is much more helpful than

  -1 is not a positive integer.

E.g. something like this?

  static inline uint32_t atoi_positive(const char *name, const char *num_str)
  {
	int num = atoi_paranoid(num_str);

	TEST_ASSERT(num > 0, "%s must be greater than 0, got '%s'", name, num_str);
	return num;
  }

  static inline uint32_t atoi_non_negative(const char *name, const char *num_str)
  {
	int num = atoi_paranoid(num_str);

	TEST_ASSERT(num >= 0, "%s must be non-negative, got '%s'", name, num_str);
	return num;
  }

IMO, that also makes the code slightly easier to follow as it's super obvious
what is being parsed.

  p.wr_fract = atoi_positive("Write fraction", optarg);

  p.iterations = atoi_positive("Number of iterations", optarg);

  nr_vcpus = atoi_positive("Number of vCPUs", optarg);

Last thought: my vote would be to ignore the 80 char soft limit when adding the
"name" to these calls, in every case except nr_memslot_modifications the overrun
is relatively minor and not worth wrapping.  See below for my thougts on that one.

>  			break;
>  		case 'm':
> -			max_mem = atoi_paranoid(optarg) * size_1gb;
> +			max_mem = atoi_positive(optarg) * size_1gb;
>  			TEST_ASSERT(max_mem > 0, "memory size must be >0");

This assert can be dropped, max_mem is a uint64_t so wrapping to '0' is impossible.

>  			break;
>  		case 's':
> -			slot_size = atoi_paranoid(optarg) * size_1gb;
> +			slot_size = atoi_positive(optarg) * size_1gb;

Same thing here.

>  			TEST_ASSERT(slot_size > 0, "slot size must be >0");
>  			break;
>  		case 'H':
> diff --git a/tools/testing/selftests/kvm/memslot_modification_stress_test.c b/tools/testing/selftests/kvm/memslot_modification_stress_test.c
> index 865276993ffb..7539ee7b6e95 100644
> --- a/tools/testing/selftests/kvm/memslot_modification_stress_test.c
> +++ b/tools/testing/selftests/kvm/memslot_modification_stress_test.c
> @@ -175,7 +175,7 @@ int main(int argc, char *argv[])
>  			p.partition_vcpu_memory_access = false;
>  			break;

memslot_modification_delay can be converted to atoi_non_negative(), it open codes
strtoul(), but the "long" part is unnecessary because memslot_modification_delay
is an "unsigned int", not an "unsigned long".

>  		case 'i':
> -			p.nr_memslot_modifications = atoi_paranoid(optarg);
> +			p.nr_memslot_modifications = atoi_positive(optarg);

To avoid a ridiculously long line, my vote is to rename the test args.  The names
are rather odd irrespective of line length.  E.g. in a prep patch do

  s/memslot_modification_delay/delay
  s/nr_memslot_modifications/nr_iterations

which yields parsing of:

	while ((opt = getopt(argc, argv, "hm:d:b:v:oi:")) != -1) {
		switch (opt) {
		case 'm':
			guest_modes_cmdline(optarg);
			break;
		case 'd':
			p.delay = atoi_non_negative("Delay", optarg);
			break;
		case 'b':
			guest_percpu_mem_size = parse_size(optarg);
			break;
		case 'v':
			nr_vcpus = atoi_positive("Number of vCPUs", optarg);
			TEST_ASSERT(nr_vcpus <= max_vcpus,
				    "Invalid number of vcpus, must be between 1 and %d",
				    max_vcpus);
			break;
		case 'o':
			p.partition_vcpu_memory_access = false;
			break;
		case 'i':
			p.nr_iterations = atoi_positive("Number of iterations", optarg);
			break;
		case 'h':
		default:
			help(argv[0]);
			break;
		}
	}

Powered by blists - more mailing lists