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

[David Laight]

On Mon, 10 Mar 2025 11:05:13 +0200
Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx> wrote:

> On Sat, Mar 08, 2025 at 09:34:21AM +0000, David Laight wrote:
> > Fastpath the normal case of single byte output that fits in the buffer.
> > Output byte groups (byteswapped on little-endian) without calling snprintf().
> > Remove the restriction that rowsize must be 16 or 32.
> > (All callers currently pass 16 or 32.)
> > Remove the restriction that groupsize must be 8 or less.
> > If groupsize isn't a power of 2 or doesn't divide into both len and
> >   rowsize it is set to 1 (otherwise byteswapping is hard).
> > Change the types of the rowsize and groupsize parameters to be unsigned types.
> > 
> > Fix the return value (should be zero) when both len and linebuflen are zero.
> > 
> > All the updated tests in lib/test_hexdump.c pass.
> > Code size (x86-64) approximately halved.  
> 
> ...
> 
> > -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,  
> 
> Why is extern still here?

Because I didn't spot it ...

> 
> > +				 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.
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').

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.)

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.

	David