[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>
--
Eric Blake, Principal Software Engineer
Red Hat, Inc.
Virtualization: qemu.org | libguestfs.org