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