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

[Lucas De Marchi]

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?

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