Re: [PATCH net-next v3 1/3] hexdump: Implement macro for converting large buffers

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

On Sat, 22 Feb 2025 12:58:48 -0600
Nick Child <nnac123@linux.ibm.com> wrote:

> On Fri, Feb 21, 2025 at 10:18:15PM +0000, David Laight wrote:
> > On Fri, 21 Feb 2025 12:50:59 -0600
> > Nick Child <nnac123@linux.ibm.com> wrote:
> >   
> > > On Fri, Feb 21, 2025 at 06:04:35PM +0000, David Laight wrote:  
> > > > On Fri, 21 Feb 2025 11:37:46 -0600
> > > > Nick Child <nnac123@linux.ibm.com> wrote:    
> > > > > On Thu, Feb 20, 2025 at 10:00:50PM +0000, David Laight wrote:    
> > > > > > You could do:
> > > > > > #define for_each_line_in_hex_dump(buf_offset, rowsize, linebuf, linebuflen, groupsize, buf, len, ascii) \
> > > > > > for (unsigned int _offset = 0, _rowsize = (rowsize), _len = (len); \
> > > > > > 	((offset) = _offset) < _len && (hex_dump_to_buffer((const char *)(buf) + _offset, _len - _offset, \    
> > > >           ^ needs to be buf_offset.
> > > >     
> > > > > > 		_rowsize, (groupsize), (linebuf), (linebuflen), (ascii)), 1); \
> > > > > > 	_offset += _rowsize )
> > > > > > 
> > > > > > (Assuming I've not mistyped it.)
> > > > > >       
> > > > >
> > > > > Trying to understand the reasoning for declaring new tmp variables;
> > > > > Is this to prevent the values from changing in the body of the loop?    
> > > >
> > > > No, it is to prevent side-effects happening more than once.
> > > > Think about what would happen if someone passed 'foo -= 4' for len.
> > > >    
> > > 
> > > If we are protecting against those cases then linebuf, linebuflen,
> > > groupsize and ascii should also be stored into tmp variables since they
> > > are referenced in the loop conditional every iteration.
> > > At which point the loop becomes too messy IMO.
> > > Are any other for_each implementations taking these precautions?  
> > 
> > No, it only matters if they appear in the text expansion of the #define
> > more than once.  
> 
> But the operation is still executed more than once when the variable
> appears in the loop conditional. This still sounds like the same type
> of unexpected behaviour. For example, when I set groupsize = 1 then
> invoke for_each_line_in_hex_dump with groupsize *= 2 I get:
> [    4.688870][  T145] HD: 0100 0302 0504 0706 0908 0b0a 0d0c 0f0e
> [    4.688949][  T145] HD: 13121110 17161514 1b1a1918 1f1e1d1c
> [    4.688969][  T145] HD: 2726252423222120 2f2e2d2c2b2a2928
> [    4.688983][  T145] HD: 30 31 32 33 34 35 36 37 38 39 3a 3b 3c 3d 3e 3f
> Similarly if I run with buf: buf += 8:
> [    5.019031][  T149] HD: 08 09 0a 0b 0c 0d 0e 0f 10 11 12 13 14 15 16 17
> [    5.019057][  T149] HD: 20 21 22 23 24 25 26 27 28 29 2a 2b 2c 2d 2e 2f
> [    5.019069][  T149] HD: 38 39 3a 3b 3c 3d 3e 3f 98 1a 6a 95 de e6 9a 71
> [    5.019081][  T149] HD: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> 
> The operations are getting executed more than once. Should this be
> classified as expected behaviour just because those vars are technically
> only expanded once in the macro?

Brain fade on my side :-(

While you can copy all the integers, all the variables defined in a for(;;)
statement have to have the same type - so you can't take a copy of the
pointers.

So maybe this is actually unwritable without having odd side effects and
probably doesn't really save much text in any place it is used.

I did a scan of the kernel sources earlier.
Everything sets rowsize to 16 or 32, so it doesn't matter if hexdump_to_buffer()
just uses the supplied value.
I didn't look at the history.

	David