Re: [PATCH v6 1/6] selftests/mm: make file helpers return errors

> Change read_file(), write_file(), read_num() and write_num() in vm_util.c
> to report failures to callers instead of exiting from the helper.
> 
> Make read_file() return a negative errno on failure instead of 0, so
> callers can distinguish a successful read from an I/O error. Also make
> read_num() reject negative and malformed values.
> 
> Update callers to print diagnostics and fail wherever required. This
> patch prepares the helpers to be moved to tools/lib/mm without
> kselftest dependency.
> 
> Signed-off-by: Sarthak Sharma <[email protected]>
>
> diff --git a/tools/testing/selftests/mm/hugepage_settings.c b/tools/testing/selftests/mm/hugepage_settings.c
> index 2eab2110ac6a..db0db8a3df7c 100644
> --- a/tools/testing/selftests/mm/hugepage_settings.c
> +++ b/tools/testing/selftests/mm/hugepage_settings.c
> @@ -8,6 +8,7 @@
>  #include <stdlib.h>
>  #include <string.h>
>  #include <unistd.h>
> +#include <errno.h>
>  
>  #include "vm_util.h"
>  #include "hugepage_settings.h"
> @@ -61,8 +62,10 @@ int thp_read_string(const char *name, const char * const strings[])
>  		exit(EXIT_FAILURE);
>  	}
>  
> -	if (!read_file(path, buf, sizeof(buf))) {
> -		perror(path);
> +	ret = read_file(path, buf, sizeof(buf));
> +	if (ret < 0) {
> +		errno = -ret;
> +		ksft_perror(path);

I'm not a fan of changing errno, why can't we use

	ksft_print_msg("%s: %s\n", path, strerror(ret));

>  		exit(EXIT_FAILURE);
>  	}
>  
> @@ -700,91 +700,139 @@ int unpoison_memory(unsigned long pfn)
>  
>  int read_file(const char *path, char *buf, size_t buflen)
>  {
> -	int fd;
> +	int fd, err;
>  	ssize_t numread;
>  
>  	fd = open(path, O_RDONLY);
>  	if (fd == -1)
> -		return 0;
> +		return -errno;
>  
>  	numread = read(fd, buf, buflen - 1);
>  	if (numread < 1) {
> +		err = numread ? errno : ENODATA;
>  		close(fd);
> -		return 0;
> +		return -err;
>  	}
>  
>  	buf[numread] = '\0';
>  	close(fd);
>  
> -	return (unsigned int) numread;
> +	return (int)numread;

Do we really care about how many bytes we read?
Can't we return 0 for success and -error code for failure?

Will also make checks for read_file() return value neater.

>  }
>  
> -unsigned long read_num(const char *path)
> +int read_num(const char *path, unsigned long *num)
>  {
> +	unsigned long val;
> +	int ret;
>  	char buf[21];
> +	char *end;
>  
> -	if (read_file(path, buf, sizeof(buf)) < 0)
> -		ksft_exit_fail_perror("read_file()");
> +	if (!num)
> +		return -EINVAL;
>  
> -	return strtoul(buf, NULL, 10);
> +	ret = read_file(path, buf, sizeof(buf));
> +	if (ret < 0)
> +		return ret;
> +
> +	errno = 0;
> +	val = strtoul(buf, &end, 10);
> +	if (errno)
> +		return -errno;
> +
> +	if (end == buf || buf[0] == '-')
> +		return -EINVAL;

We can check the sign right after read_file() and skip errno dance
around strtoul().

-- 
Sincerely yours,
Mike.