[PATCH][next] mips: sgi-ip22: Replace "s[n]?printf" with sysfs_emit in sysfs callbacks

[PATCH][next] mips: sgi-ip22: Replace "s[n]?printf" with sysfs_emit in sysfs callbacks

[Paulo Miguel Almeida]

snprintf() has the documented, but still rather strange trait of
returning the length of the data that *would have been* written to the
array if space were available, rather than the arguably more useful
length of data *actually* written, it is usually considered wise to use
scnprintf()/vscnprintf() instead. In the case of sysfs call-backs,
new wrappers exist that do just that while also being PAGE_SIZE aware

This patch updates the sysfs .show() callbacks to use the sysfs_emit()
helper instead of snprintf() and sprintf().

Link: https://lwn.net/Articles/69419/
Link: https://github.com/KSPP/linux/issues/105
Signed-off-by: Paulo Miguel Almeida <paulo.miguel.almeida.rodenas@gmail.com>
---
 arch/mips/sgi-ip22/ip22-gio.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/arch/mips/sgi-ip22/ip22-gio.c b/arch/mips/sgi-ip22/ip22-gio.c
index d20eec742bfa..5893ea4e382c 100644
--- a/arch/mips/sgi-ip22/ip22-gio.c
+++ b/arch/mips/sgi-ip22/ip22-gio.c
@@ -165,9 +165,8 @@ static ssize_t modalias_show(struct device *dev, struct device_attribute *a,
 			     char *buf)
 {
 	struct gio_device *gio_dev = to_gio_device(dev);
-	int len = snprintf(buf, PAGE_SIZE, "gio:%x\n", gio_dev->id.id);
 
-	return (len >= PAGE_SIZE) ? (PAGE_SIZE - 1) : len;
+	return sysfs_emit(buf, "gio:%x\n", gio_dev->id.id);
 }
 static DEVICE_ATTR_RO(modalias);
 
@@ -177,7 +176,7 @@ static ssize_t name_show(struct device *dev,
 	struct gio_device *giodev;
 
 	giodev = to_gio_device(dev);
-	return sprintf(buf, "%s", giodev->name);
+	return sysfs_emit(buf, "%s\n", giodev->name);
 }
 static DEVICE_ATTR_RO(name);
 
@@ -187,7 +186,7 @@ static ssize_t id_show(struct device *dev,
 	struct gio_device *giodev;
 
 	giodev = to_gio_device(dev);
-	return sprintf(buf, "%x", giodev->id.id);
+	return sysfs_emit(buf, "%x\n", giodev->id.id);
 }
 static DEVICE_ATTR_RO(id);
 
-- 
2.47.0

Re: [PATCH][next] mips: sgi-ip22: Replace "s[n]?printf" with sysfs_emit in sysfs callbacks

[Maciej W. Rozycki]

On Tue, 15 Oct 2024, Paulo Miguel Almeida wrote:

> snprintf() has the documented, but still rather strange trait of
> returning the length of the data that *would have been* written to the
> array if space were available, rather than the arguably more useful
> length of data *actually* written, [...]

 Why do you think that just returning `n - 1' in the case of a length 
overflow would be more useful than returning the unmet buffer length 
requirement?  I think the opposite is the case: the value returned lets 
you reallocate the buffer for more space and retry, and there's no other 
way to figure out how much this would be.  And if you need to know how 
many characters were actually written, then `min(n - 1, snprintf(...))' 
will do (and code you propose to replace does exactly that, open-coded).

 The change itself makes sense to me, but not your proposed description 
I'm afraid.  Just replacing open-coded pieces with calls to `sysfs_emit' 
is enough justification.

  Maciej

Re: [PATCH][next] mips: sgi-ip22: Replace "s[n]?printf" with sysfs_emit in sysfs callbacks

[Paulo Miguel Almeida]

On Tue, Oct 15, 2024 at 03:01:13AM +0100, Maciej W. Rozycki wrote:
> On Tue, 15 Oct 2024, Paulo Miguel Almeida wrote:
> 
> > snprintf() has the documented, but still rather strange trait of
> > returning the length of the data that *would have been* written to the
> > array if space were available, rather than the arguably more useful
> > length of data *actually* written, [...]
> 
>  Why do you think that just returning `n - 1' in the case of a length 
> overflow would be more useful than returning the unmet buffer length 
> requirement?  I think the opposite is the case: the value returned lets 
> you reallocate the buffer for more space and retry, and there's no other 
> way to figure out how much this would be.  And if you need to know how 
> many characters were actually written, then `min(n - 1, snprintf(...))' 
> will do (and code you propose to replace does exactly that, open-coded).
> 
>  The change itself makes sense to me, but not your proposed description 
> I'm afraid.  Just replacing open-coded pieces with calls to `sysfs_emit' 
> is enough justification.
> 
>   Maciej

Thanks for taking the time to review this patch.

Will submit a v2 with the description you pointed out.

- Paulo A.