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

[Janusz Krzysztofik]

On Thursday, 1 February 2024 14:51:26 CET Lucas De Marchi wrote:
> On Thu, Feb 01, 2024 at 02:15:53PM +0100, Janusz Krzysztofik wrote:
> >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?
> 
> in the commit message. Yes. It was supposed to be show the real effect
> of that, i.e. we do write  "".
> 
> 
> >
> >>
> >> 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;
> >>  }
> >>

If you agree on limiting this patch to the above chunk and dropping below 
changes to igt_sysfs_vprintf() which look problematic to me then, with the 
above discussed mistake corrected, you have my

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

Thanks,
Janusz

> >> @@ -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.
> 
> I will add a line in the commit message for this sneaking in.
> 
> >
> >>  	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."
> 
> we are passing 1 so we don't fall in the case of passing 0 that write(2)
> refers to. If the return value is 0, then we return whatever write()
> returned and the user deals with it like it had done before.
> 
> >
> >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.
> 
> if you look with a different angle, both are actually passing len + 1.
> One goes directly to igt_written() vs igt_sysfs_write() because of the
> arguments it has at hand and is consistent with the layer the specific
> function is operating on.
> 
> 	if (igt_debug_on(ret == 0))
> 		return igt_writen(fd, stack, ret + 1);
> 
> to be similar would equally work, but slightly harder to understand
> what's going on.
> 
> Lucas De Marchi
> 
> >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;
> >>
> >>
> >
> >
> >
> >
>