[Libguestfs] Re: [PATCH nbdkit v2] pattern: Accurately update b & offset in loop

On Fri, Nov 03, 2023 at 08:32:41PM +0000, Richard W.M. Jones wrote: > It's better to update the 'b' pointer and the 'offset' using the > calculated increment 'n', rather than using '8-o' which might be > larger than the remainder of the buffer. Previously we could create a > dangling pointer beyond the end of the output buffer, and this should > not matter, but it's best not to do this if we can easily avoid it. > > Also improve the test by replacing qemu-io with nbdsh. It is able to > make non-aligned requests so we can now test those. > --- > plugins/pattern/pattern.c | 4 +- > tests/test-pattern.sh | 129 ++++++++++++++++++++++---------------- > 2 files changed, 77 insertions(+), 56 deletions(-) > > + > +# Run nbdkit-pattern-plugin. Use a disk > 4G so we can test 2G and 4G > +# boundaries. > +nbdkit pattern 5G --run 'nbdsh -u "$uri" -c -' <<EOF > + > +# Generate the expected pattern in the given range. > +# This only works for 8-byte aligned ranges. > +def generated_expected(start,end): I think Python prefers a space after that comma. > + assert start % 8 == 0 > + assert end % 8 == 0 > + expected = bytearray() > + for i in range(start,end,8): Here as well. (Since this script is embedded inside a larger file, our python style checkers can't easily see what we've typed...). Fixing it is cosmetic; no need to spin a v3 if that's all the more I point out. > + expected = expected + i.to_bytes(8, 'big') > + return expected Yes, that's a nicer helper function. ... > +# Same as above, but around offset 4G. > +offset = 4*1024*1024*1024 - 32 > +expected = generated_expected(offset, offset+64) > +actual = h.pread(64, offset) > +check_same(actual, expected) > +actual = h.pread(59, offset+5) > +check_same(actual, expected[5:]) > +actual = h.pread(63, offset) > +check_same(actual, expected[:63]) > + > +# Finish at the end of the disk. > +offset = 5*1024*1024*1024 - 64 > +expected = generated_expected(offset, offset+64) > +actual = h.pread(64, offset) > +check_same(actual, expected) Reviewed-by: Eric Blake <eblake(a)redhat.com&gt;