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

On Sat, 22 Feb 2025 12:58:48 -0600
Nick Child [off-list ref] wrote:

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

quoted

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?

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