All of lore.kernel.org
 help / color / mirror / Atom feed
From: Lucas De Marchi <lucas.demarchi@intel.com>
To: igt-dev@lists.freedesktop.org
Cc: Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com>,
	Lucas De Marchi <lucas.demarchi@intel.com>
Subject: [PATCH i-g-t v4 4/4] lib/igt_sysfs: make sure to write empty strings
Date: Wed, 28 Feb 2024 14:31:34 -0800	[thread overview]
Message-ID: <20240228223134.3908035-4-lucas.demarchi@intel.com> (raw)
In-Reply-To: <20240228223134.3908035-1-lucas.demarchi@intel.com>

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
v4:
  - Fix return code in igt_sysfs_vprintf() when fmt string is something
    like "%c<more_fmt>", c, ..." so it still returns > 0 (Janusz)

Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
---
 lib/igt_sysfs.c | 41 +++++++++++++++++++++++++++++++++--------
 1 file changed, 33 insertions(+), 8 deletions(-)

diff --git a/lib/igt_sysfs.c b/lib/igt_sysfs.c
index a1ff5655d..0c5817eb1 100644
--- a/lib/igt_sysfs.c
+++ b/lib/igt_sysfs.c
@@ -348,7 +348,10 @@ int igt_sysfs_get_num_gt(int device)
  * @data: the block to write from
  * @len: the length to write
  *
- * This writes @len bytes from @data to the sysfs file.
+ * This writes @len bytes from @data to the sysfs file.  Contrary to
+ * igt_sysfs_set(), this does not automatically write a null char if len is 0.
+ * 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,17 +531,27 @@ 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;
-
-		buf = malloc(len);
+	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)) {
+		buf = malloc(len + 1);
 		if (igt_debug_on(!buf)) {
 			ret = -ENOMEM;
 			goto end;
 		}
 
-		ret = vsnprintf(buf, len, fmt, ap);
-		if (igt_debug_on(ret != len - 1)) {
+		ret = vsnprintf(buf, len + 1, fmt, ap);
+		if (igt_debug_on(ret != len)) {
 			ret = -EINVAL;
 			goto free_buf;
 		}
@@ -538,6 +559,10 @@ int igt_sysfs_vprintf(int dir, const char *attr, const char *fmt, va_list ap)
 
 	ret = igt_writen(fd, buf, ret);
 
+	/* Caller shouldn't know about special sysfs handling, just return 0 */
+	if (!len && ret == 1)
+		ret = 0;
+
 free_buf:
 	if (buf != stack)
 		free(buf);
-- 
2.43.0


  parent reply	other threads:[~2024-02-28 22:33 UTC|newest]

Thread overview: 23+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2024-02-28 22:31 [PATCH i-g-t v4 1/4] lib/igt_sysfs: Use same var for sizeof() Lucas De Marchi
2024-02-28 22:31 ` [PATCH i-g-t v4 2/4] lib/igt_sysfs: stop leaking fd on write failures Lucas De Marchi
2024-02-29 11:06   ` Janusz Krzysztofik
2024-02-29 17:33     ` Lucas De Marchi
2024-03-01 15:15       ` Janusz Krzysztofik
2024-03-01 15:53         ` Lucas De Marchi
2024-02-28 22:31 ` [PATCH i-g-t v4 3/4] lib/igt_sysfs: Fix off-by-one in buffer size Lucas De Marchi
2024-02-29 11:07   ` Janusz Krzysztofik
2024-02-29 17:01     ` Lucas De Marchi
2024-03-01 15:16       ` Janusz Krzysztofik
2024-03-01 15:55         ` Lucas De Marchi
2024-02-28 22:31 ` Lucas De Marchi [this message]
2024-03-01 15:21   ` [PATCH i-g-t v4 4/4] lib/igt_sysfs: make sure to write empty strings Janusz Krzysztofik
2024-02-28 23:14 ` ✓ CI.xeBAT: success for series starting with [i-g-t,v4,1/4] lib/igt_sysfs: Use same var for sizeof() Patchwork
2024-02-28 23:19 ` ✓ Fi.CI.BAT: " Patchwork
2024-02-29 18:06 ` ✗ Fi.CI.IGT: failure " Patchwork
2024-03-05 16:58 ` ✓ CI.xeBAT: success for series starting with [i-g-t,v4,1/4] lib/igt_sysfs: Use same var for sizeof() (rev2) Patchwork
2024-03-05 17:07 ` ✗ Fi.CI.BAT: failure " Patchwork
2024-03-06  4:54 ` ✗ CI.xeBAT: failure for series starting with [i-g-t,v4,1/4] lib/igt_sysfs: Use same var for sizeof() (rev3) Patchwork
2024-03-06  5:12 ` ✗ Fi.CI.BAT: " Patchwork
2024-03-11 22:17 ` ✓ Fi.CI.BAT: success for series starting with [i-g-t,v4,1/4] lib/igt_sysfs: Use same var for sizeof() (rev4) Patchwork
2024-03-11 22:19 ` ✓ CI.xeBAT: " Patchwork
2024-03-12  5:38 ` ✗ Fi.CI.IGT: failure " Patchwork

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20240228223134.3908035-4-lucas.demarchi@intel.com \
    --to=lucas.demarchi@intel.com \
    --cc=igt-dev@lists.freedesktop.org \
    --cc=janusz.krzysztofik@linux.intel.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.