selftests/mm: fix read_file() return value check

[PATCH] selftests/mm: fix read_file() return value check

Posted by Hongfu Li 1 month, 3 weeks ago

From: Hongfu Li <lihongfu@kylinos.cn>

read_file() returns 0 on open/read failures and never returns negative
values. Existing < 0 error checks never trigger, so read failures are
silently ignored. Check for zero return to detect read_file() failures.

Also fix misleading error message in get_finfo(). The error string
incorrectly references read_num when reading uevent files.

Signed-off-by: Hongfu Li <lihongfu@kylinos.cn>
---
 tools/testing/selftests/mm/khugepaged.c | 4 ++--
 tools/testing/selftests/mm/vm_util.c    | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/tools/testing/selftests/mm/khugepaged.c b/tools/testing/selftests/mm/khugepaged.c
index 10e8dedcb087..506310d4b4d5 100644
--- a/tools/testing/selftests/mm/khugepaged.c
+++ b/tools/testing/selftests/mm/khugepaged.c
@@ -138,8 +138,8 @@ static void get_finfo(const char *dir)
 		     major(path_stat.st_dev), minor(path_stat.st_dev))
 	    >= sizeof(path))
 		ksft_exit_fail_msg("%s: Pathname is too long\n", __func__);
-	if (read_file(path, buf, sizeof(buf)) < 0)
-		ksft_exit_fail_perror("read_file(read_num)");
+	if (!read_file(path, buf, sizeof(buf)))
+		ksft_exit_fail_perror("read_file(uevent)");
 	if (strstr(buf, "DEVTYPE=disk")) {
 		/* Found it */
 		if (snprintf(finfo.dev_queue_read_ahead_path,
diff --git a/tools/testing/selftests/mm/vm_util.c b/tools/testing/selftests/mm/vm_util.c
index ef1ea11981a7..2697d50d07f4 100644
--- a/tools/testing/selftests/mm/vm_util.c
+++ b/tools/testing/selftests/mm/vm_util.c
@@ -755,7 +755,7 @@ unsigned long read_num(const char *path)
 {
 	char buf[21];
 
-	if (read_file(path, buf, sizeof(buf)) < 0)
+	if (!read_file(path, buf, sizeof(buf)))
 		ksft_exit_fail_perror("read_file()");
 
 	return strtoul(buf, NULL, 10);
-- 
2.54.0

Re: [PATCH] selftests/mm: fix read_file() return value check

Posted by David Hildenbrand (Arm) 1 month, 3 weeks ago

On 8/7/26 03:35, Hongfu Li wrote:
> From: Hongfu Li <lihongfu@kylinos.cn>
> 
> read_file() returns 0 on open/read failures and never returns negative
> values. Existing < 0 error checks never trigger, so read failures are
> silently ignored. Check for zero return to detect read_file() failures.
> 
> Also fix misleading error message in get_finfo(). The error string
> incorrectly references read_num when reading uevent files.
> 
> Signed-off-by: Hongfu Li <lihongfu@kylinos.cn>
> ---
>  tools/testing/selftests/mm/khugepaged.c | 4 ++--
>  tools/testing/selftests/mm/vm_util.c    | 2 +-
>  2 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/tools/testing/selftests/mm/khugepaged.c b/tools/testing/selftests/mm/khugepaged.c
> index 10e8dedcb087..506310d4b4d5 100644
> --- a/tools/testing/selftests/mm/khugepaged.c
> +++ b/tools/testing/selftests/mm/khugepaged.c
> @@ -138,8 +138,8 @@ static void get_finfo(const char *dir)
>  		     major(path_stat.st_dev), minor(path_stat.st_dev))
>  	    >= sizeof(path))
>  		ksft_exit_fail_msg("%s: Pathname is too long\n", __func__);
> -	if (read_file(path, buf, sizeof(buf)) < 0)
> -		ksft_exit_fail_perror("read_file(read_num)");
> +	if (!read_file(path, buf, sizeof(buf)))
> +		ksft_exit_fail_perror("read_file(uevent)");
>  	if (strstr(buf, "DEVTYPE=disk")) {
>  		/* Found it */
>  		if (snprintf(finfo.dev_queue_read_ahead_path,
> diff --git a/tools/testing/selftests/mm/vm_util.c b/tools/testing/selftests/mm/vm_util.c
> index ef1ea11981a7..2697d50d07f4 100644
> --- a/tools/testing/selftests/mm/vm_util.c
> +++ b/tools/testing/selftests/mm/vm_util.c
> @@ -755,7 +755,7 @@ unsigned long read_num(const char *path)
>  {
>  	char buf[21];
>  
> -	if (read_file(path, buf, sizeof(buf)) < 0)
> +	if (!read_file(path, buf, sizeof(buf)))
>  		ksft_exit_fail_perror("read_file()");
>  
>  	return strtoul(buf, NULL, 10);


Yeah, that looks correct. Do we have Fixes: tags?

I think it was already wrong in read_num() in


commit e0c13f9761df8f97cf5e81495d12ecbc4075684a
Author: Kiryl Shutsemau <kas@kernel.org>
Date:   Wed Jun 3 16:00:06 2020 -0700

    khugepaged: add self test


where we had

+       ret = read_file(path, buf, sizeof(buf));
+       if (ret < 0) {
+               perror("read_file(read_num)");
+               exit(EXIT_FAILURE);
+       }


So likely

Fixes: e0c13f9761df ("khugepaged: add self test")

Acked-by: David Hildenbrand (Arm) <david@kernel.org>


-- 
Cheers,

David

Re: [PATCH] selftests/mm: fix read_file() return value check

Posted by Hongfu Li 1 month, 2 weeks ago

> > diff --git a/tools/testing/selftests/mm/vm_util.c b/tools/testing/selftests/mm/vm_util.c
> > index ef1ea11981a7..2697d50d07f4 100644
> > --- a/tools/testing/selftests/mm/vm_util.c
> > +++ b/tools/testing/selftests/mm/vm_util.c
> > @@ -755,7 +755,7 @@ unsigned long read_num(const char *path)
> >  {
> >  	char buf[21];
> >  
> > -	if (read_file(path, buf, sizeof(buf)) < 0)
> > +	if (!read_file(path, buf, sizeof(buf)))
> >  		ksft_exit_fail_perror("read_file()");
> >  
> >  	return strtoul(buf, NULL, 10);
> 
> 
> Yeah, that looks correct. Do we have Fixes: tags?
> 
> I think it was already wrong in read_num() in
> 
> 
> commit e0c13f9761df8f97cf5e81495d12ecbc4075684a
> Author: Kiryl Shutsemau <kas@kernel.org>
> Date:   Wed Jun 3 16:00:06 2020 -0700
> 
>     khugepaged: add self test
> 
> 
> where we had
> 
> +       ret = read_file(path, buf, sizeof(buf));
> +       if (ret < 0) {
> +               perror("read_file(read_num)");
> +               exit(EXIT_FAILURE);
> +       }
> 
> 
> So likely
> 
> Fixes: e0c13f9761df ("khugepaged: add self test")
> 
> Acked-by: David Hildenbrand (Arm) <david@kernel.org>

Thank you for the review and tracking down the original commit.
I will include the Fixes tag you mentioned in the next patch revision.

Best regards,
Hongfu