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

[Janusz Krzysztofik]

On Tuesday, 6 February 2024 18:13:13 CET Lucas De Marchi wrote:
> On Tue, Feb 06, 2024 at 04:25:16PM +0100, Janusz Krzysztofik wrote:
> >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
> 
> humn?? why would we leave the function below broken?

I wouldn't, and that's a misinterpretation of my words, since I didn't say 
that.  As I wrote in my former comments, I just didn't agree with your 
proposed solution.

> 
> 	const char *foo = get_foo_val();
> 	const char *bar = get_bar_val();
> 
> 	igt_sysfs_printf(dirfd, "test_sysfs_file", "%s%s", foo, bar);
> 
> why would we leave igt_sysfs_printf broken, forcing the user to check
> foo and bar and fallback to igt_sysfs_write() or igt_sysfs_set()? It
> seems even a higher level function than igt_sysfs_set() that should
> really abstract that.
> 
> Lucas De Marchi
> 
> >
> >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.

With your approach, igt_sysfs_printf() would return the same value of 1 in two 
different cases: when the successfully rendered and written string was either 
of one or zero bytes length.  I don't like it.

Since I don't agree with your approach, I can support only that part of 
your patch that fixes igt_sysfs_set(), in a way free from my doubts.  I'm 
surprised you have understood my lack of acceptance to your changes to 
igt_sysfs_printf() as my vote against fixing that function and leaving it 
broken.  I just left that topic open for further discussion.

Thanks,
Janusz


> >>
> >> >
> >> >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;
> >> >>
> >> >>
> >> >
> >> >
> >> >
> >> >
> >>
> >
> >
> >
> >
>