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