[PATCH i-g-t 2/3] lib/igt_sysfs: make sure to write empty strings

[Janusz Krzysztofik]

On Thursday, 1 February 2024 01:50:29 CET Lucas De Marchi wrote:
> 	echo -n "" > /sys/module/<modulename>/parameters/<param>
> 
> doesn't really work as it will just create a open() + close() expecting
> the file to be truncated. The same issue happens with igt as it will
> stop writing when there are 0 chars to write. Special case the empty
> string so it always write a '\0' and make sure igt_sysfs_set() accounts
> for the extra null char.
> 
> Shell example:
> 	# echo -n "/foo" > /sys/module/firmware_class/parameters/path
> 	# cat /sys/module/firmware_class/parameters/path
> 	/foo
> 	# echo -n "" > /sys/module/firmware_class/parameters/path
> 	/foo
> 	# # make sure to actually write a \0:
> 	echo -ne "\0" > /sys/module/firmware_class/parameters/path
> 	# cat /sys/module/firmware_class/parameters/path
> 	/foo

Still not overwritten?  Isn't that a mistake?

> 
> Same thing happens when testing igt_sysfs_set():
>        int dirfd = open("/sys/module/firmware_class/parameters", O_RDONLY);
>        igt_sysfs_set(dirfd, "path", "");
> 
> Previously it was not really setting the param.
> 
> Signed-off-by: Lucas De Marchi <lucas.demarchi at intel.com>
> ---
>  lib/igt_sysfs.c | 12 ++++++++++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/lib/igt_sysfs.c b/lib/igt_sysfs.c
> index 567b4f6d5..814220ddb 100644
> --- a/lib/igt_sysfs.c
> +++ b/lib/igt_sysfs.c
> @@ -406,7 +406,7 @@ int igt_sysfs_read(int dir, const char *attr, void 
*data, int len)
>   */
>  bool igt_sysfs_set(int dir, const char *attr, const char *value)
>  {
> -	int len = strlen(value);
> +	int len = strlen(value) + 1;
>  	return igt_sysfs_write(dir, attr, value, len) == len;
>  }
>  
> @@ -513,8 +513,16 @@ int igt_sysfs_vprintf(int dir, const char *attr, const 
char *fmt, va_list ap)
>  		return -errno;
>  
>  	va_copy(tmp, ap);
> -	ret = vsnprintf(buf, sizeof(stack), fmt, tmp);
> +	ret = vsnprintf(stack, sizeof(stack), fmt, tmp);

Looks not related.

>  	va_end(tmp);
> +
> +	/*
> +	 * make sure to always issue a write() syscall, even if writing an 
empty string,
> +	 * otherwise values in sysfs like module parameters don't really get 
overwritten
> +	 */
> +	if (igt_debug_on(ret = 0))
> +		return igt_writen(fd, "", 1);

I think users may know that an empty string is going to be written, then may 
expect return value of 0, not 1.  Also, what value should we return to the 
user if (unlikely) we got 0 from igt_writen()?  write(2) says: "If count is 
zero and fd refers to a file other than a regular file, the results are not 
specified."

While resolving two instances of one and the same issue, you use two different 
approaches:
1) always increase the number of characters to be written by 1 when writing 
   strings, so at least the terminating null byte is always written,
2) divert to a separate call to igt_writen() that writes just the terminating 
   null byte when the string to be written is empty.
While both methods can work correctly, I think it would be more clear if you 
selected one of them and stuck to it, or provide a justification if not.

Thanks,
Janusz


> +
>  	if (igt_debug_on(ret < 0))
>  		return -EINVAL;
>  
>