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