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

[Janusz Krzysztofik]

Hi Lucas,

On Thursday, 22 February 2024 20:33:26 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
> 
> 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.
> 
> v2:
>   - Fix return code from igt_sysfs_vprintf() to differentiate between
>     writing 1 or 0 chars (Janusz)
>   - Document the behavior of igt_sysfs_set() as being a higher-level
>     than igt_sysfs_write().
> 
> Signed-off-by: Lucas De Marchi <lucas.demarchi at intel.com>
> ---
>  lib/igt_sysfs.c | 28 +++++++++++++++++++++++-----
>  1 file changed, 23 insertions(+), 5 deletions(-)
> 
> diff --git a/lib/igt_sysfs.c b/lib/igt_sysfs.c
> index 2b0225138..d5d5b249f 100644
> --- a/lib/igt_sysfs.c
> +++ b/lib/igt_sysfs.c
> @@ -349,6 +349,9 @@ int igt_sysfs_get_num_gt(int device)
>   * @len: the length to write
>   *
>   * This writes @len bytes from @data to the sysfs file.
> + * Contrary to igt_sysfs_set(), this does not automatically add a
> + * null char to terminate the data. It's caller responsibility to pass
> + * the right len according to the data being written.
>   *
>   * Returns:
>   * The number of bytes written, or -errno on error.
> @@ -406,7 +409,12 @@ 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);
> +	/*
> +	 * Always write the null char at the end, to guarantee the write goes
> +	 * through, even for an empty string.
> +	 */
> +	int len = strlen(value) + 1;

Since pre-merge results indicate some sysfs entries don't like that extra null 
char, I'm assuming you are going to submit a fixed version.

> +
>  	return igt_sysfs_write(dir, attr, value, len) == len;
>  }
>  
> @@ -506,7 +514,7 @@ int igt_sysfs_vprintf(int dir, const char *attr, const char *fmt, va_list ap)
>  {
>  	char stack[128], *buf = stack;
>  	va_list tmp;
> -	int ret, fd;
> +	int ret, fd, len;
>  
>  	fd = openat(dir, attr, O_WRONLY);
>  	if (igt_debug_on(fd < 0))
> @@ -515,11 +523,12 @@ int igt_sysfs_vprintf(int dir, const char *attr, const char *fmt, va_list ap)
>  	va_copy(tmp, ap);
>  	ret = vsnprintf(stack, sizeof(stack), fmt, tmp);
>  	va_end(tmp);
> +

NIT: Not related.

>  	if (igt_debug_on(ret < 0))
>  		return -EINVAL;
>  
>  	if (ret > sizeof(stack)) {
> -		unsigned int len = ret + 1;
> +		len = ret + 1;
>  
>  		buf = malloc(len);
>  		if (igt_debug_on(!buf))
> @@ -532,13 +541,22 @@ int igt_sysfs_vprintf(int dir, const char *attr, const char *fmt, va_list ap)
>  		}
>  	}
>  
> -	ret = igt_writen(fd, buf, ret);
> +	/*
> +	 * 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.  vsnprintf() guarantees to return a \0
> +	 * terminated string, so just add that char. The return code is still
> +	 * the same as before, to abstract that from caller.
> +	 */
> +	len = ret + 1;

Potentially the same issue with some sysfs entries, I believe.

Thanks,
Janusz


> +
> +	ret = igt_writen(fd, buf, len);
>  
>  	close(fd);
>  	if (buf != stack)
>  		free(buf);
>  
> -	return ret;
> +	return ret == len ? len - 1 : ret;
>  }
>  
>  /**
>