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

[Lucas De Marchi]

On Thu, Feb 01, 2024 at 02:12:04PM +0100, Kamil Konieczny wrote:
>Hi Lucas,
>On 2024-01-31 at 19:11:56 -0800, 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
>>
>> 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..64c2aaabb 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;
>
>Why not adding write "\n" at end?

because it's not what we want. On the kernel side it will have different
effect depending what sysfs file you are writing to. "foo\n" != "foo"
and sysfs won't strip '\n' (while it always guarantee a \0 at the end).

Try this and see the extra newline at the end as part of the value:

	# echo -ne "/foo\n" > /sys/module/firmware_class/parameters/path
	# hexdump /sys/module/firmware_class/parameters/path
	0000000 662f 6f6f 0a0a                         
	0000006
	# echo -ne "/foo" > /sys/module/firmware_class/parameters/path
	# hexdump /sys/module/firmware_class/parameters/path
	0000000 662f 6f6f 000a                         
	0000005
	# echo -ne "\n" > /sys/module/firmware_class/parameters/path
	# hexdump /sys/module/firmware_class/parameters/path
	0000000 0a0a                                   
	0000002
	# echo -ne "\0" > /sys/module/firmware_class/parameters/path
	# hexdump /sys/module/firmware_class/parameters/path
	0000000 000a                                   
	0000001



>
>>  }
>>
>> @@ -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);
>------------------- ^^^^^
>Nice catch and looks like a separate bug fix, please send
>as separate patch.

it's not really a bug as at this point in the code they point to the
same thing. However it is a bug waiting to appear.

Lucas De Marchi

>
>Regards,
>Kamil
>
>>  	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);
>> +
>>  	if (igt_debug_on(ret < 0))
>>  		return -EINVAL;
>>
>> --
>> 2.43.0
>>