[PATCH] USB: usbsevseg: cleanup sysfs show handlers

[PATCH] USB: usbsevseg: cleanup sysfs show handlers

[Dmitry Antipov]

Prefer the convenient sysfs_emit() and sysfs_emit_at() over
sprintf() and strcat()-based quirks in sysfs show handlers.

Signed-off-by: Dmitry Antipov <dmantipov@yandex.ru>
---
 drivers/usb/misc/usbsevseg.c | 26 ++++++++++----------------
 1 file changed, 10 insertions(+), 16 deletions(-)

diff --git a/drivers/usb/misc/usbsevseg.c b/drivers/usb/misc/usbsevseg.c
index 89d25fcef642..f456a7161a26 100644
--- a/drivers/usb/misc/usbsevseg.c
+++ b/drivers/usb/misc/usbsevseg.c
@@ -145,7 +145,7 @@ static ssize_t name##_show(struct device *dev,			\
 	struct usb_interface *intf = to_usb_interface(dev);	\
 	struct usb_sevsegdev *mydev = usb_get_intfdata(intf);	\
 								\
-	return sprintf(buf, "%u\n", mydev->name);		\
+	return sysfs_emit(buf, "%u\n", mydev->name);		\
 }								\
 								\
 static ssize_t name##_store(struct device *dev,			\
@@ -246,25 +246,19 @@ static ssize_t textmode_show(struct device *dev,
 {
 	struct usb_interface *intf = to_usb_interface(dev);
 	struct usb_sevsegdev *mydev = usb_get_intfdata(intf);
-	int i;
-
-	buf[0] = 0;
+	int i, ret = 0;
 
 	for (i = 0; i < ARRAY_SIZE(display_textmodes); i++) {
-		if (mydev->textmode == i) {
-			strcat(buf, " [");
-			strcat(buf, display_textmodes[i]);
-			strcat(buf, "] ");
-		} else {
-			strcat(buf, " ");
-			strcat(buf, display_textmodes[i]);
-			strcat(buf, " ");
-		}
+		if (mydev->textmode == i)
+			ret += sysfs_emit_at(buf, ret, " [%s] ",
+					     display_textmodes[i]);
+		else
+			ret += sysfs_emit_at(buf, ret, " %s ",
+					     display_textmodes[i]);
 	}
-	strcat(buf, "\n");
-
 
-	return strlen(buf);
+	ret += sysfs_emit_at(buf, ret - 1, "\n");
+	return ret;
 }
 
 static ssize_t textmode_store(struct device *dev,
-- 
2.56.0

Re: [PATCH] USB: usbsevseg: cleanup sysfs show handlers

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] usb: usbsevseg: off-by-one in textmode_show() emits trailing NUL byte
--

commit a74c1b2c71cf0a70caf0b902cff95d63b0e182d0
Author: Dmitry Antipov <dmantipov@yandex.ru>

USB: usbsevseg: cleanup sysfs show handlers

This commit replaces sprintf() and strcat() calls in usbsevseg sysfs show
handlers with sysfs_emit() and sysfs_emit_at().

> diff --git a/drivers/usb/misc/usbsevseg.c b/drivers/usb/misc/usbsevseg.c
> index 89d25fcef6424..f456a7161a260 100644
> --- a/drivers/usb/misc/usbsevseg.c
> +++ b/drivers/usb/misc/usbsevseg.c

[ ... ]

> @@ -246,25 +246,19 @@ static ssize_t textmode_show(struct device *dev,
>  {
>  	struct usb_interface *intf = to_usb_interface(dev);
>  	struct usb_sevsegdev *mydev = usb_get_intfdata(intf);
> -	int i;
> -
> -	buf[0] = 0;
> +	int i, ret = 0;
>  
>  	for (i = 0; i < ARRAY_SIZE(display_textmodes); i++) {
> -		if (mydev->textmode == i) {
> -			strcat(buf, " [");
> -			strcat(buf, display_textmodes[i]);
> -			strcat(buf, "] ");
> -		} else {
> -			strcat(buf, " ");
> -			strcat(buf, display_textmodes[i]);
> -			strcat(buf, " ");
> -		}
> +		if (mydev->textmode == i)
> +			ret += sysfs_emit_at(buf, ret, " [%s] ",
> +					     display_textmodes[i]);
> +		else
> +			ret += sysfs_emit_at(buf, ret, " %s ",
> +					     display_textmodes[i]);
>  	}
> -	strcat(buf, "\n");
> -
>  
> -	return strlen(buf);
> +	ret += sysfs_emit_at(buf, ret - 1, "\n");
> +	return ret;

[Severity: Medium]
Does passing ret - 1 to sysfs_emit_at() while still adding its return value
to ret cause textmode_show() in drivers/usb/misc/usbsevseg.c to include a
trailing NUL byte in the output returned to userspace when reading the
textmode sysfs attribute?

At the end of the loop, buf[0 .. ret - 1] holds the formatted string with a
trailing space at buf[ret - 1] and a NUL terminator at buf[ret]. Calling
sysfs_emit_at(buf, ret - 1, "\n") overwrites the space at buf[ret - 1] with
a newline, writes the NUL terminator at buf[ret], and returns 1.

Adding that return value to ret increments ret to strlen(buf) + 1. When
sysfs_kf_seq_show() in fs/sysfs/file.c calls seq_commit(sf, count) with that
returned count, the NUL byte at buf[ret - 1] is committed into the seq_file
buffer and exposed to userspace.

Should this either pass ret instead of ret - 1 to preserve the original
trailing space before the newline, or avoid incrementing ret if overwriting
the trailing space was intentional?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261029124716.611540-1-dmantipov@yandex.ru?part=1

[PATCH v2] USB: usbsevseg: cleanup sysfs show handlers

[Dmitry Antipov]

Prefer the convenient sysfs_emit() and sysfs_emit_at() over
sprintf() and strcat()-based quirks in sysfs show handlers.

Signed-off-by: Dmitry Antipov <dmantipov@yandex.ru>
---
v2: fix off-by-one error in textmode_show() (Sashiko)
---
 drivers/usb/misc/usbsevseg.c | 26 ++++++++++----------------
 1 file changed, 10 insertions(+), 16 deletions(-)

diff --git a/drivers/usb/misc/usbsevseg.c b/drivers/usb/misc/usbsevseg.c
index 89d25fcef642..1140ca4fbd8d 100644
--- a/drivers/usb/misc/usbsevseg.c
+++ b/drivers/usb/misc/usbsevseg.c
@@ -145,7 +145,7 @@ static ssize_t name##_show(struct device *dev,			\
 	struct usb_interface *intf = to_usb_interface(dev);	\
 	struct usb_sevsegdev *mydev = usb_get_intfdata(intf);	\
 								\
-	return sprintf(buf, "%u\n", mydev->name);		\
+	return sysfs_emit(buf, "%u\n", mydev->name);		\
 }								\
 								\
 static ssize_t name##_store(struct device *dev,			\
@@ -246,25 +246,19 @@ static ssize_t textmode_show(struct device *dev,
 {
 	struct usb_interface *intf = to_usb_interface(dev);
 	struct usb_sevsegdev *mydev = usb_get_intfdata(intf);
-	int i;
-
-	buf[0] = 0;
+	int i, ret = 0;
 
 	for (i = 0; i < ARRAY_SIZE(display_textmodes); i++) {
-		if (mydev->textmode == i) {
-			strcat(buf, " [");
-			strcat(buf, display_textmodes[i]);
-			strcat(buf, "] ");
-		} else {
-			strcat(buf, " ");
-			strcat(buf, display_textmodes[i]);
-			strcat(buf, " ");
-		}
+		if (mydev->textmode == i)
+			ret += sysfs_emit_at(buf, ret, " [%s] ",
+					     display_textmodes[i]);
+		else
+			ret += sysfs_emit_at(buf, ret, " %s ",
+					     display_textmodes[i]);
 	}
-	strcat(buf, "\n");
-
 
-	return strlen(buf);
+	sysfs_emit_at(buf, ret - 1, "\n");
+	return ret;
 }
 
 static ssize_t textmode_store(struct device *dev,
-- 
2.56.0

Re: [PATCH v2] USB: usbsevseg: cleanup sysfs show handlers

[sashiko-bot]

> Prefer the convenient sysfs_emit() and sysfs_emit_at() over
> sprintf() and strcat()-based quirks in sysfs show handlers.
> 
> Signed-off-by: Dmitry Antipov <dmantipov@yandex.ru>

Sashiko has reviewed this patch and found no issues.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261029131401.676734-1-dmantipov@yandex.ru?part=1