[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 18:07:28 UTC 2024
On Wed, Feb 28, 2024 at 05:07:52PM +0100, Janusz Krzysztofik wrote:
>On Wednesday, 28 February 2024 16:54:59 CET Lucas De Marchi wrote:
>> 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?
>
>Not with just "%s" (you better use igt_sysfs_set() for that), but possible
>with e.g.
>
> igt_sysfs_printf(fd, attr, "%c<more_fmt>", c, ...);
ohh, right... there are indeed cases where this could happen. Thanks for
spotting that.
...
>> > - 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)) {
shouldn't this be
ret = vsnprintf(buf, len + 1, fmt, ap);
if (igt_debug_on(ret != len)) {
?
what if ret is negative (unlikely, but it seems before this patch it could
happen due to an off-by-one in the sized passed)?
with your changes + these above, testing igt_sysfs_printf() I correctly
get the results for:
1)
ret = igt_sysfs_printf(fd, "path", "%*c", 130, '/');
ret == 130
$ hexdump -c /sys/module/firmware_class/parameters/path
0000000
*
0000080 / \n
0000083
2)
ret = igt_sysfs_printf(fd, "path", "%*c%s", 130, '/', "foobar");
ret == 136
$ hexdump -c /sys/module/firmware_class/parameters/path
0000000
*
0000080 / f o o b a r \n
0000089
3)
ret = igt_sysfs_printf(fd, "path", "%c%*c%s", '\0', 130, '/', "foobar");
ret == 1
$ hexdump -c /sys/module/firmware_class/parameters/path
0000000 \n
0000001
4) ret = igt_sysfs_printf(fd, "path", "%c%s", '\0', "foobar");
ret == 1
$ hexdump -c /sys/module/firmware_class/parameters/path
0000000 \n
0000001
5)
ret = igt_sysfs_printf(fd, "path", "%c", '\0');
ret == 1
hexdump -c /sys/module/firmware_class/parameters/path
0000000 \n
0000001
6)
ret = igt_sysfs_printf(fd, "path", "");
ret == 0
hexdump -c /sys/module/firmware_class/parameters/path
0000000 \n
0000001
7)
ret = igt_sysfs_set(fd, "path", "");
ret == true
hexdump -c /sys/module/firmware_class/parameters/path
0000000 \n
0000001
Lucas De Marchi
>> >+ 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