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

[Lucas De Marchi]

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