Re: [PATCH v4 3/4] KVM: selftests: Add atoi_paranoid() to catch errors missed by atoi()

[Vipin Sharma]

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