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

[Nick Child]

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@xxxxxxxxxxxxx> 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@xxxxxxxxxxxxx> 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?

> > Not trying to come off dismissive, I genuinely appreciate all the
> > insight, trying to learn more for next time.
> > 
> > > > I tried to avoid declaring new vars in this design because I thought it
> > > > would recive pushback due to possible name collision and variable
> > > > declaration inside for loop initializer.  
> > >
> > > The c std level got upped recently to allow declarations inside loops.
> > > Usually for a 'loop iterator' - but I think you needed that to be
> > > exposed outsize the loop.
> > > (Otherwise you don't need _offset and buf_offset.
> > >  
> > 
> > As in decrementing _len and increasing a _buf var rather than tracking
> > offset?
> > I don't really care for exposing the offset, during design I figured
> > some caller may make use of it but I think it is worth removing to reduce
> > the number of arguments.
> 
> Except the loop body needs it - so it needs to be a caller-defined name,
> even if they don't declare the variable.
> 
> 	David
> 
> > 
> > Thanks again,
> > Nick
>