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

On Fri, Feb 21, 2025 at 10:18:15PM +0000, David Laight wrote:

On Fri, 21 Feb 2025 12:50:59 -0600
Nick Child [off-list ref] wrote:

quoted

On Fri, Feb 21, 2025 at 06:04:35PM +0000, David Laight wrote:

quoted

On Fri, 21 Feb 2025 11:37:46 -0600
Nick Child [off-list ref] wrote:  

quoted

On Thu, Feb 20, 2025 at 10:00:50PM +0000, David Laight wrote:  

quoted

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.
  

quoted

quoted

		_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?

quoted

Not trying to come off dismissive, I genuinely appreciate all the
insight, trying to learn more for next time.

quoted

quoted

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

quoted

Thanks again,
Nick