[PATCH i-g-t v3 3/3] lib/igt_sysfs: make sure to write empty strings
Lucas De Marchi
lucas.demarchi at intel.com
Wed Feb 28 15:54:59 UTC 2024
On Wed, Feb 28, 2024 at 12:13:22PM +0100, Janusz Krzysztofik wrote:
>Hi Lucas,
>
>On Wednesday, 28 February 2024 07:32:11 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
>>
>> 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.
>>
>> v2:
>> - Fix return code from igt_sysfs_vprintf() to differentiate between
>> writing 1 or 0 chars (Janusz)
>> - Document the behavior of igt_sysfs_set() as being a higher-level
>> than igt_sysfs_write().
>> v3:
>> - Partial "back to v1": include the \0 in the write only when writing
>> an empty string: there are some files in sysfs like pm_test that
>> don't like
>>
>> Signed-off-by: Lucas De Marchi <lucas.demarchi at intel.com>
>> ---
>> lib/igt_sysfs.c | 33 +++++++++++++++++++++++++++++----
>> 1 file changed, 29 insertions(+), 4 deletions(-)
>>
>> diff --git a/lib/igt_sysfs.c b/lib/igt_sysfs.c
>> index 2997925e5..2d574cc17 100644
>> --- a/lib/igt_sysfs.c
>> +++ b/lib/igt_sysfs.c
>> @@ -349,6 +349,9 @@ int igt_sysfs_get_num_gt(int device)
>> * @len: the length to write
>> *
>> * This writes @len bytes from @data to the sysfs file.
>> + * Contrary to igt_sysfs_set(), this does not automatically add a null char to
>
>Should now read something like "this does not automatically write a null char
>if len is 0", I believe.
>
>> + * terminate the data. It's caller responsibility to pass the right len
>> + * according to the data being written.
>> *
>> * Returns:
>> * The number of bytes written, or -errno on error.
>> @@ -407,6 +410,14 @@ 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);
>> +
>> + /*
>> + * Always write at least 1 char, the null byte, otherwise it
>> + * won't write anything on sysfs.
>> + */
>> + if (!len)
>> + len = 1;
>> +
>> return igt_sysfs_write(dir, attr, value, len) == len;
>> }
>>
>> @@ -506,7 +517,7 @@ int igt_sysfs_vprintf(int dir, const char *attr, const char *fmt, va_list ap)
>> {
>> char stack[128], *buf = stack;
>> va_list tmp;
>> - int ret, fd;
>> + int ret, fd, len;
>>
>> fd = openat(dir, attr, O_WRONLY);
>> if (igt_debug_on(fd < 0))
>> @@ -520,8 +531,18 @@ int igt_sysfs_vprintf(int dir, const char *attr, const char *fmt, va_list ap)
>> goto end;
>> }
>>
>> - if (ret > sizeof(stack)) {
>> - unsigned int len = ret + 1;
>> + if (!ret) {
>> + /*
>> + * 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. vsnprintf()
>> + * guarantees to return a \0 terminated string, so just add
>> + * that char. The return code is still the same as before, to
>> + * abstract that from caller.
>> + */
>> + len = 1;
>> + } else if (ret > sizeof(stack)) {
>> + len = ret + 1;
>>
>> buf = malloc(len);
>> if (igt_debug_on(!buf)) {
>> @@ -536,7 +557,11 @@ int igt_sysfs_vprintf(int dir, const char *attr, const char *fmt, va_list ap)
>> }
>> }
>>
>> - ret = igt_writen(fd, buf, ret);
>> + ret = igt_writen(fd, buf, len);
>> +
>> + /* Caller shouldn't know about special sysfs handling, just return 0 */
>> + if (ret == 1 && buf[0] == '\0')
>> + ret = 0;
>
>Hmm, now we no longer support writing data with null character embedded at
>position 0.
why not?
igt_sysfs_vprintf(fd, "foo", "") will cause vsnprintf() to return 0.
igt_sysfs_vprintf(fd, "foo", "\0") will cause vsnprintf() to return 0.
[1]
which is also true for both if tried like
foo_val = "\0";
igt_sysfs_vprintf(fd, "foo", "%s", foo_val);
Is this what you are trying to differentiate or did I misunderstand?
there's no difference from printf/snprintf point of view, so expecting a
return > 0 when appending \0 would be wrong.
( Could also be considered bad code to use *printf() family of function
to any constant string )
The patch seems to already handle that correctly and return 0 like snprintf()
would as far as I can see.
Lucas De Marchi
[ 1 ]
#include <stdio.h>
char buf[10];
int main(int argc, const char *argv[])
{
int ret;
char *foo_val = "";
char *foo_val2 = "\0";
char *foo_val3 = "\0test";
ret = snprintf(buf, sizeof(buf), "");
printf("ret=%d\n", ret);
ret = snprintf(buf, sizeof(buf), "\0");
printf("ret=%d\n", ret);
ret = snprintf(buf, sizeof(buf), "\0\0\0");
printf("ret=%d\n", ret);
ret = snprintf(buf, sizeof(buf), "%s", foo_val);
printf("ret=%d\n", ret);
ret = snprintf(buf, sizeof(buf), "%s", foo_val2);
printf("ret=%d\n", ret);
ret = snprintf(buf, sizeof(buf), "%s", foo_val3);
printf("ret=%d\n", ret);
return 0;
}
$ /tmp/a
ret=0
ret=0
ret=0
ret=0
ret=0
ret=0
btw, clang also warns by default about using printf like that:
$ clang -o /tmp/a /tmp/a.c && /tmp/a
/tmp/a.c:15:36: warning: format string contains '\0' within the string body [-Wformat]
ret = snprintf(buf, sizeof(buf), "\0");
~^~~
/tmp/a.c:18:36: warning: format string contains '\0' within the string body [-Wformat]
ret = snprintf(buf, sizeof(buf), "\0\0\0");
~^~~~~~~
2 warnings generated.
and gcc with -Wall goes a little bit further and also warns on the
empty string.
/tmp/a.c: In function ‘main’:
/tmp/a.c:12:42: warning: zero-length gnu_printf format string [-Wformat-zero-length]
12 | ret = snprintf(buf, sizeof(buf), "");
| ^~
/tmp/a.c:15:43: warning: embedded ‘\0’ in format [-Wformat-contains-nul]
15 | ret = snprintf(buf, sizeof(buf), "\0");
| ^~
/tmp/a.c:18:43: warning: embedded ‘\0’ in format [-Wformat-contains-nul]
18 | ret = snprintf(buf, sizeof(buf), "\0\0\0");
|
Lucas De Marchi
>
>How about keeping the original length of the resulting string in len?
>Seems easy to get with your code slightly modified:
>
>
> - if (ret > sizeof(stack)) {
> - unsigned int len = ret + 1;
>++ len = ret;
> + if (!ret)
> + /*
> + * 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. vsnprintf()
> + * guarantees to return a \0 terminated string, so just add
> + * that char. The return code is still the same as before, to
> + * abstract that from caller.
> + */
> + ret = 1;
> + } else if (ret > sizeof(stack)) {
>-+ len = ret + 1;
>-
>- buf = malloc(len);
>+-
>+- buf = malloc(len);
>++ buf = malloc(len + 1);
> if (igt_debug_on(!buf)) {
>+ ret = -ENOMEM;
>+ goto end;
>+ }
>+
>+ ret = vsnprintf(buf, ret, fmt, ap);
>+- if (igt_debug_on(ret > len)) {
>++ if (igt_debug_on(ret > len + 1)) {
>+ ret = -EINVAL;
>+ goto free_buf;
>+ }
>
>and then:
>
> }
>
>-- ret = igt_writen(fd, buf, ret);
>-+ ret = igt_writen(fd, buf, len);
>+ ret = igt_writen(fd, buf, ret);
> +
> + /* Caller shouldn't know about special sysfs handling, just return 0 */
>-+ if (ret == 1 && buf[0] == '\0')
>++ if (!len && ret == 1)
> + ret = 0;
>
>Thanks,
>Janusz
>
>
>>
>> free_buf:
>> if (buf != stack)
>>
>
>
>
>
More information about the igt-dev
mailing list