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

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.