Re: [PATCH RFC 2/2] sysfs: add helper macro for showing simple integer values

[Alex Dewar]

On Sun, Aug 30, 2020 at 11:13:53AM +0200, Greg Kroah-Hartman wrote:
> On Sun, Aug 30, 2020 at 12:37:17AM +0100, Alex Dewar wrote:
> > sysfs attributes are supposed to be only single values, which are
> > printed into a buffer of PAGE_SIZE. Accordingly, for many simple
> > attributes, sprintf() can be used like so:
> > 	static ssize_t my_show(..., char *buf)
> > 	{
> > 		...
> > 		return sprintf("%d\n", my_integer);
> > 	}
> > 
> > The problem is that whilst this use of sprintf() is memory safe, other
> > cases where e.g. a possibly unterminated string is passed as input, are
> > not and so use of sprintf() here might make it more difficult to
> > identify these problematic cases.
> > 
> > Define a macro, sysfs_sprinti(), which outputs the value of a single
> > integer to a buffer (with terminating "\n\0") and returns the size written.
> > This way, we can convert over the some of the trivially correct users of
> > sprintf() and decrease its usage in the kernel source tree.
> > 
> > Another advantage of this approach is that we can now statically check
> > the type of the integer so that e.g. an unsigned long long will be
> > formatted as %llu. This will fix cases where the wrong format string has
> > been passed to sprintf().
> > 
> > Signed-off-by: Alex Dewar <alex.dewar90@xxxxxxxxx>
> > ---
> >  include/linux/sysfs.h | 31 +++++++++++++++++++++++++++++++
> >  1 file changed, 31 insertions(+)
> 
> Did you try this out?  Don't you need to return the number of bytes
> written?

I tried it out, but maybe not thoroughly enough ;-)

> 
> I like Joe's patches better, this feels like more work...

Fair enough. As Joe's pointed out, even for single numbers the
formatting is sometimes more complicated, so his approach does seem
best. Thanks for looking though :-)

> 
> thanks,
> 
> greg k-h