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

[Janusz Krzysztofik]

On Wednesday, 28 February 2024 23:31:34 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().
> v3:
>   - Partial "back to v1": include the \0 in the write only when writing
>     an empty string: there are some files in sysfs like pm_test that
>     don't like
> v4:
>   - Fix return code in igt_sysfs_vprintf() when fmt string is something
>     like "%c<more_fmt>", c, ..." so it still returns > 0 (Janusz)
> 
> Signed-off-by: Lucas De Marchi <lucas.demarchi at intel.com>

Reviewed-by: Janusz Krzysztofik <janusz.krzysztofik at linux.intel.com>

> ---
>  lib/igt_sysfs.c | 41 +++++++++++++++++++++++++++++++++--------
>  1 file changed, 33 insertions(+), 8 deletions(-)
> 
> diff --git a/lib/igt_sysfs.c b/lib/igt_sysfs.c
> index a1ff5655d..0c5817eb1 100644
> --- a/lib/igt_sysfs.c
> +++ b/lib/igt_sysfs.c
> @@ -348,7 +348,10 @@ int igt_sysfs_get_num_gt(int device)
>   * @data: the block to write from
>   * @len: the length to write
>   *
> - * This writes @len bytes from @data to the sysfs file.
> + * This writes @len bytes from @data to the sysfs file.  Contrary to
> + * igt_sysfs_set(), this does not automatically write a null char if len is 0.
> + * 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.
> @@ -407,6 +410,14 @@ 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 at least 1 char, the null byte, otherwise it
> +	 * won't write anything on sysfs.
> +	 */
> +	if (!len)
> +		len = 1;
> +
>  	return igt_sysfs_write(dir, attr, value, len) == len;
>  }
>  
> @@ -506,7 +517,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))
> @@ -520,17 +531,27 @@ int igt_sysfs_vprintf(int dir, const char *attr, const char *fmt, va_list ap)
>  		goto end;
>  	}
>  
> -	if (ret > sizeof(stack)) {
> -		unsigned int len = ret + 1;
> -
> -		buf = malloc(len);
> +	len = ret;
> +
> +	if (!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.
> +		 */
> +		ret = 1;
> +	} else if (ret > sizeof(stack)) {
> +		buf = malloc(len + 1);
>  		if (igt_debug_on(!buf)) {
>  			ret = -ENOMEM;
>  			goto end;
>  		}
>  
> -		ret = vsnprintf(buf, len, fmt, ap);
> -		if (igt_debug_on(ret != len - 1)) {
> +		ret = vsnprintf(buf, len + 1, fmt, ap);
> +		if (igt_debug_on(ret != len)) {
>  			ret = -EINVAL;
>  			goto free_buf;
>  		}
> @@ -538,6 +559,10 @@ int igt_sysfs_vprintf(int dir, const char *attr, const char *fmt, va_list ap)
>  
>  	ret = igt_writen(fd, buf, ret);
>  
> +	/* Caller shouldn't know about special sysfs handling, just return 0 */
> +	if (!len && ret == 1)
> +		ret = 0;
> +
>  free_buf:
>  	if (buf != stack)
>  		free(buf);
>