Hi Mike!
On 8/3/26 2:30 PM, Mike Rapoport wrote:
>> 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));
Ack
>
>> 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.
Yeah, no caller actually uses the number of bytes read. Will make this
change.
>
>> }
>>
>> -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().
Yes, we can check the sign after read_file(), but still we would have to
check errno after strtoul() to see if an unsigned long overflow happened.
