Re: [PATCH v7 4/5] KVM: selftests: Add atoi_positive() and atoi_non_negative() for input validation

[Vipin Sharma]

On Mon, Oct 31, 2022 at 12:48 PM Sean Christopherson <seanjc@xxxxxxxxxx> wrote:
>
> 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.
>

I am not sure if this makes life much easier for developers, as
"inline" can totally be ignored by the compiler. Also, not sure how
much qualitative improvement it will add in the developer's code
browsing journey. Anyways, I will add "inline" in the next version.

> > +{
> > +     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).
>

Okay, I will remove it from the previous patch also.

> > +     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);
>

I will make this change. It is indeed better.

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

I intentionally kept it, as it is also protecting against having
accidently making size_1gb to 0.

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

Okay, I will change this and any other places I find which can be shortened.

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