Re: [PATCH 1/3] selftests/mm/write_to_hugetlbfs: parse -s with strtoull and use size_t

[Li Wang <liwang@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 3503 bytes --]

Andrew Morton <akpm@linux-foundation.org> wrote:

>  #./charge_reserved_hugetlb.sh -cgroup-v2
> >  # -----------------------------------------
> >  ...
> >  # nr hugepages = 10
> >  # writing cgroup limit: 5368709120
> >  # writing reseravation limit: 5368709120
>
> Can we fix that typo while we're in there?  "reservation".
>

Good catch, let's fix it in patch 2/3.


> >       while ((c = getopt(argc, argv, "s:p:m:owlrn")) != -1) {
> >               switch (c) {
> >               case 's':
> > -                     size = atoi(optarg);
> > +                     errno = 0;
> > +                     char *end = NULL;
> > +                     unsigned long long tmp = strtoull(optarg, &end,
> 10);
>
> Coding-style nits: we do accept c99-style definitions nowadays but I do
> think our eyes prefer the less surprising "definitions come before
> code" style.  So the above could be
>
>                         char *end = NULL;
>                         unsigned long long tmp = strtoull(optarg, &end,
> 10);
>
>                         errno = 0;
>
> Also, `errno' belongs to libc.  It seems wrong to be altering it from
> within our client code.
>

Indeed, since I saw 'errono' modified in many places in this test,
so to avoid other effects, I set it to zero. However, we can abandon
this method and use sscanf() instead, thanks!


>
> > +                     if (errno || end == optarg || *end != '\0') {
> > +                             errno = EINVAL;
> > +                             perror("Invalid -s size");
> > +                             exit_usage();
> > +                     }
> > +                     if (tmp == 0) {
> > +                             errno = EINVAL;
> > +                             perror("size not found");
> > +                             exit_usage();
> > +                     }
> > +                     size = (size_t)tmp;
> >                       break;
>
> I'm not really clear on what problems we're trying to solve here, but
> this all seems like a lot of fuss.  Can we just do
>
>         if (sscanf(optarg, "%zu", &size) != 1)
>

Yes, I admit I was overthinking the int overflow handling.
This is nice, so the V2 should be simply like:

--- a/tools/testing/selftests/mm/write_to_hugetlbfs.c
+++ b/tools/testing/selftests/mm/write_to_hugetlbfs.c
@@ -68,7 +68,7 @@ int main(int argc, char **argv)
        int key = 0;
        int *ptr = NULL;
        int c = 0;
-       int size = 0;
+       size_t size = 0;
        char path[256] = "";
        enum method method = MAX_METHOD;
        int want_sleep = 0, private = 0;
@@ -86,7 +86,10 @@ int main(int argc, char **argv)
        while ((c = getopt(argc, argv, "s:p:m:owlrn")) != -1) {
                switch (c) {
                case 's':
-                       size = atoi(optarg);
+                       if (sscanf(optarg, "%zu", &size) != 1) {
+                               perror("Invalid -s.");
+                               exit_usage();
+                       }
                        break;
                case 'p':
                        strncpy(path, optarg, sizeof(path) - 1);
@@ -131,7 +134,7 @@ int main(int argc, char **argv)
        }

        if (size != 0) {
-               printf("Writing this size: %d\n", size);
+               printf("Writing this size: %zu\n", size);
        } else {
                errno = EINVAL;
                perror("size not found");


And, if no more comments on the rest of patchset, I will send v2 later
today.


-- 
Regards,
Li Wang

[-- Attachment #2: Type: text/html, Size: 5928 bytes --]