On Thu, Oct 06, 2022, Vipin Sharma wrote: > +int atoi_paranoid(const char *num_str) > +{ > + int num; > + char *end_ptr; Reverse fir-tree when it's convention: char *end_ptr; > + > + errno = 0; > + num = (int)strtol(num_str, &end_ptr, 10); > + TEST_ASSERT(!errno, "strtol(\"%s\") failed", num_str); > + TEST_ASSERT(num_str != end_ptr, > + "strtol(\"%s\") didn't find any valid number.\n", num_str); s/number/integer ? And should that be "a valid intenger", not "any valid integer"? "any" implies that this helper will be happy if there's at least one integer, whereas I believe the intent is to find _exactly_ one integer. > + TEST_ASSERT( > + *end_ptr == '\0', Weird and unnecessary wrap+indentation. > + "strtol(\"%s\") failed to parse trailing characters \"%s\".\n", > + num_str, end_ptr); > + > + 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 9a6e4f3ad6b5..1595b73dc09a 100644 > --- a/tools/testing/selftests/kvm/max_guest_memory_test.c > +++ b/tools/testing/selftests/kvm/max_guest_memory_test.c > @@ -193,15 +193,15 @@ int main(int argc, char *argv[]) > while ((opt = getopt(argc, argv, "c:h:m:s:H")) != -1) { > switch (opt) { > case 'c': > - nr_vcpus = atoi(optarg); > + nr_vcpus = atoi_paranoid(optarg); > TEST_ASSERT(nr_vcpus > 0, "number of vcpus must be >0"); Many users require a positive and or non-negative value, maybe add wrappers in a follow-up? nr_vcpus = atoi_positive(optarg); and later down targs->tfirst = atoi_non_negative(optarg); We'll lose custom error messages, but I don't think that's a big deal. Definitely not required, just a thought. > diff --git a/tools/testing/selftests/kvm/set_memory_region_test.c b/tools/testing/selftests/kvm/set_memory_region_test.c > index 0d55f508d595..c366949c8362 100644 > --- a/tools/testing/selftests/kvm/set_memory_region_test.c > +++ b/tools/testing/selftests/kvm/set_memory_region_test.c > @@ -407,7 +407,7 @@ int main(int argc, char *argv[]) > > #ifdef __x86_64__ > if (argc > 1) > - loops = atoi(argv[1]); > + loops = atoi_paranoid(argv[1]); This is a good candidate for atoi_positive(). > else > loops = 10; > > diff --git a/tools/testing/selftests/kvm/x86_64/nx_huge_pages_test.c b/tools/testing/selftests/kvm/x86_64/nx_huge_pages_test.c > index 59ffe7fd354f..354b6902849c 100644 > --- a/tools/testing/selftests/kvm/x86_64/nx_huge_pages_test.c > +++ b/tools/testing/selftests/kvm/x86_64/nx_huge_pages_test.c > @@ -241,10 +241,10 @@ int main(int argc, char **argv) > while ((opt = getopt(argc, argv, "hp:t:r")) != -1) { > switch (opt) { > case 'p': > - reclaim_period_ms = atoi(optarg); > + reclaim_period_ms = atoi_paranoid(optarg); > break; > case 't': > - token = atoi(optarg); > + token = atoi_paranoid(optarg); > break; > case 'r': > reboot_permissions = true; > -- > 2.38.0.rc1.362.ged0d419d3c-goog >