On Thu, Oct 6, 2022 at 12:58 PM Sean Christopherson <seanjc@xxxxxxxxxx> wrote:
>
> 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;
>
Okay, I will do:
char *end_ptr;
int num;
I was not aware of reverse christmas tree convention in KVM subsystem.
> > +
> > + 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.
>
I will change it to "a valid integer".
> > + TEST_ASSERT(
> > + *end_ptr == '\0',
>
> Weird and unnecessary wrap+indentation.
Clang-format script did it. I think it is because the script is
considering 80 characters limit and the next line "strtol..."
overshoots that 80 character limit.
I will manually change it to:
TEST_ASSERT(*end_ptr == '\0',
and align "strtol..." to * above.
>
> > + "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.
>
I think atoi_positive() and atoi_non_negative() will be useful
additions. I will add one more patch in v5, which replaces
atoi_paranoid() and update/remove TEST_ASSERT() condition from all
test cases.
> > 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
> >
