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

On Fri, Nov 03, 2023 at 06:53:34PM +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 | 98 ++++++++++++++++++--------------------- > 2 files changed, 46 insertions(+), 56 deletions(-) > > +++ b/tests/test-pattern.sh > +requires_run > +requires_nbdsh_uri > + > +nbdkit pattern 2M --run 'nbdsh -u "$uri" -c -' <<EOF Should we use 'pattern 5G' so that we can test reads that cross the boundary from 0x00000000ffffffff to 0x0000000100000000, to ensure we don't have any accidental sign problems? > + > +# Read from an aligned offset at the beginning of the disk. > +expected = bytearray() > +for i in range(0,64,8): > + expected = expected + i.to_bytes(8, "big") > +print("expected = %r" % expected) > +actual = h.pread(64, 0) > +print("actual = %r" % actual) > +assert actual == expected > + > +# Read from an unaligned offset. > +actual = bytearray(4) + h.pread(60, 4) > +print("actual = %r" % actual) > +assert actual == expected Couldn't we do: actual = h.pread(60, 4) print("actual = %r" % actual) assert actual == expected[4:] instead of having to append to a temporary bytearray(4)? > + > +# Read to an unaligned offset. > +actual = h.pread(60, 0) > +print("actual = %r" % actual) > +assert actual == expected[:60] > + > +# Read from an aligned offset further in the disk. > +expected = bytearray() > +for i in range(1000000,1000064,8): > + expected = expected + i.to_bytes(8, "big") This offset is still less than 4G, but definitely gets into a more interesting part of the pattern output. > +print("expected = %r" % expected) > +actual = h.pread(64, 1000000) > +print("actual = %r" % actual) > +assert actual == expected > + > +# Read from an unaligned offset. > +actual = bytearray(3) + h.pread(61, 1000003) > +print("actual = %r" % actual) > +assert actual == expected > + > +# Read to an unaligned offset. > +actual = h.pread(60, 1000000) > +print("actual = %r" % actual) > +assert actual == expected[:60] Whether or not you add even more corner case coverage (such as by reading around the 4G boundarh), this is definitely improved.

Reviewed-by: Eric Blake <eblake(a)redhat.com&gt; -- Eric Blake, Principal Software Engineer Red Hat, Inc. Virtualization: qemu.org | libguestfs.org