Re: [PATCH v2 1/1] lib: Optimise hex_dump_to_buffer()

[David Laight <david.laight.linux@gmail.com>]

On Wed, 12 Mar 2025 21:31:58 +0200
Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:

> On Wed, Mar 12, 2025 at 07:18:16PM +0000, David Laight wrote:
> > On Mon, 10 Mar 2025 11:05:13 +0200
> > Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:  
> > > On Sat, Mar 08, 2025 at 09:34:21AM +0000, David Laight wrote:  
> 
> ...
> 
> > > > -extern int hex_dump_to_buffer(const void *buf, size_t len, int rowsize,
> > > > -			      int groupsize, char *linebuf, size_t linebuflen,
> > > > -			      bool ascii);
> > > > +extern size_t hex_dump_to_buffer(const void *buf, size_t len, size_t rowsize,  
> > > > +				 size_t groupsize, char *linebuf,
> > > > +				 size_t linebuflen, bool ascii);    
> > > 
> > > int - > size_t in the returned value is incorrect change.
> > > This is explained in the comments to the test cases patch series.  
> > 
> > I don't see you mentioning why.
> > The return value is 'the number of bytes that would be output if the buffer
> > were large enough' - it is never negative.  
> 
> True...
> 
> > Although given 'a large enough buffer' length is trivially calculable
> > it would have been safer to return the actual number of bytes added
> > (excluding the '\0').  
> 
> ...but the functions keep the snprintf() semantics, which returns an int.
> This makes it more-or-less 1:1 snprintf() substitute in cases where it can
> be done in general.

And scnprintf() has been added because the return value of snprintf()
isn't the one most code wanted.

I've looked through all the code that uses the result of hex_dump_to_buffer().
The only code that needs the 'overflow' result is the test code.
Everything else will work just the same if it returns the number of characters
added to the buffer.
The code in drivers/platform/chrome/wilco_ec/debugfs.c uses the return
value without checking - hard to say whether the buffer is big enough (or whether
the code has the required locking to allow for multiple readers.

> 
> > There were no tests for 'len == 0 && linebuflen == 0', with !ascii the
> > existing hex_dump_to_buffer() even manages to return -1.
> > (and the function than generates the 'test compare data' is also broken.)  
> 
> Then you can start with fixes of those?

No one calls it like that.
I could split it into multiple patches, but they don't overlap and it just
makes more work for everyone.
> 
> > Note that libc snprintf() has the same return type as fprintf() which can
> > be -1, but any code the looks at is probably broken!
> > 
> > So an unsigned return type it better.  
> 
> Maybe, but this will deviate from the prototype and use cases.

The use cases all want a 'length' never an 'error'.
Having an unsigned return type makes it absolutely clear that -1 (or -errno)
won't be returned.
It isn't the sort of function where you want to have to 'go through hoops'
to write valid code.

	David