3:32 p.m.
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(-)
diff --git a/plugins/pattern/pattern.c b/plugins/pattern/pattern.c
index 3d2bb6040..3db1a21eb 100644
--- a/plugins/pattern/pattern.c
+++ b/plugins/pattern/pattern.c
@@ -119,8 +119,8 @@ pattern_pread (void *handle, void *buf, uint32_t count, uint64_t
offset,
o = offset & 7;
n = MIN (count, 8-o);
memcpy (b, (char *)&d + o, n);
- b += 8-o;
- offset += 8-o;
+ b += n;
+ offset += n;
count -= n;
}
diff --git a/tests/test-pattern.sh b/tests/test-pattern.sh
index 1b1bd56fb..79c42bc29 100755
--- a/tests/test-pattern.sh
+++ b/tests/test-pattern.sh
@@ -31,61 +31,82 @@
# SUCH DAMAGE.
# Test the pattern plugin.
-#
-# Note we don't have any client which can issue misaligned NBD
-# requests. qemu-io will issue 512-byte aligned requests no matter
-# what read parameters we give it. Hence these tests are rather
-# limited. (XXX)
source ./functions.sh
set -e
-requires qemu-io --version
-
-sock=$(mktemp -u /tmp/nbdkit-test-sock.XXXXXX)
-files="pattern.out pattern.pid $sock"
-rm -f $files
-cleanup_fn rm -f $files
-
-# Run nbdkit with pattern plugin.
-start_nbdkit -P pattern.pid -U $sock pattern 1G
-
-qemu-io -r -f raw "nbd+unix://?socket=$sock" \
- -c 'r -v 0 512' | grep -E '^[[:xdigit:]]+:' > pattern.out
-if [ "$(cat pattern.out)" != "00000000: 00 00 00 00 00 00 00 00 00 00 00
00 00 00 00 08 ................
-00000010: 00 00 00 00 00 00 00 10 00 00 00 00 00 00 00 18 ................
-00000020: 00 00 00 00 00 00 00 20 00 00 00 00 00 00 00 28 ................
-00000030: 00 00 00 00 00 00 00 30 00 00 00 00 00 00 00 38 .......0.......8
-00000040: 00 00 00 00 00 00 00 40 00 00 00 00 00 00 00 48 ...............H
-00000050: 00 00 00 00 00 00 00 50 00 00 00 00 00 00 00 58 .......P.......X
-00000060: 00 00 00 00 00 00 00 60 00 00 00 00 00 00 00 68 ...............h
-00000070: 00 00 00 00 00 00 00 70 00 00 00 00 00 00 00 78 .......p.......x
-00000080: 00 00 00 00 00 00 00 80 00 00 00 00 00 00 00 88 ................
-00000090: 00 00 00 00 00 00 00 90 00 00 00 00 00 00 00 98 ................
-000000a0: 00 00 00 00 00 00 00 a0 00 00 00 00 00 00 00 a8 ................
-000000b0: 00 00 00 00 00 00 00 b0 00 00 00 00 00 00 00 b8 ................
-000000c0: 00 00 00 00 00 00 00 c0 00 00 00 00 00 00 00 c8 ................
-000000d0: 00 00 00 00 00 00 00 d0 00 00 00 00 00 00 00 d8 ................
-000000e0: 00 00 00 00 00 00 00 e0 00 00 00 00 00 00 00 e8 ................
-000000f0: 00 00 00 00 00 00 00 f0 00 00 00 00 00 00 00 f8 ................
-00000100: 00 00 00 00 00 00 01 00 00 00 00 00 00 00 01 08 ................
-00000110: 00 00 00 00 00 00 01 10 00 00 00 00 00 00 01 18 ................
-00000120: 00 00 00 00 00 00 01 20 00 00 00 00 00 00 01 28 ................
-00000130: 00 00 00 00 00 00 01 30 00 00 00 00 00 00 01 38 .......0.......8
-00000140: 00 00 00 00 00 00 01 40 00 00 00 00 00 00 01 48 ...............H
-00000150: 00 00 00 00 00 00 01 50 00 00 00 00 00 00 01 58 .......P.......X
-00000160: 00 00 00 00 00 00 01 60 00 00 00 00 00 00 01 68 ...............h
-00000170: 00 00 00 00 00 00 01 70 00 00 00 00 00 00 01 78 .......p.......x
-00000180: 00 00 00 00 00 00 01 80 00 00 00 00 00 00 01 88 ................
-00000190: 00 00 00 00 00 00 01 90 00 00 00 00 00 00 01 98 ................
-000001a0: 00 00 00 00 00 00 01 a0 00 00 00 00 00 00 01 a8 ................
-000001b0: 00 00 00 00 00 00 01 b0 00 00 00 00 00 00 01 b8 ................
-000001c0: 00 00 00 00 00 00 01 c0 00 00 00 00 00 00 01 c8 ................
-000001d0: 00 00 00 00 00 00 01 d0 00 00 00 00 00 00 01 d8 ................
-000001e0: 00 00 00 00 00 00 01 e0 00 00 00 00 00 00 01 e8 ................
-000001f0: 00 00 00 00 00 00 01 f0 00 00 00 00 00 00 01 f8 ................" ]
-then
- echo "$0: unexpected pattern:"
- cat pattern.out
- exit 1
-fi
+requires_run
+requires_nbdsh_uri
+
+# 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):
+ assert start % 8 == 0
+ assert end % 8 == 0
+ expected = bytearray()
+ for i in range(start,end,8):
+ expected = expected + i.to_bytes(8, 'big')
+ return expected
+
+# Check actual == expected, with printing
+def check_same(actual, expected):
+ if actual == expected:
+ print("check_same: passed", flush=True)
+ else:
+ print("actual = %r" % actual, flush=True)
+ print("expected = %r" % expected, flush=True)
+ assert False
+
+# Read an aligned range at the beginning of the disk.
+expected = generated_expected(0, 64)
+actual = h.pread(64, 0)
+check_same(actual, expected)
+
+# Read starting from an unaligned offset.
+actual = h.pread(60, 4)
+check_same(actual, expected[4:])
+
+# Read ending at an unaligned offset.
+actual = h.pread(60, 0)
+check_same(actual, expected[:60])
+
+# Same as above, but around offset 1,000,000.
+expected = generated_expected(1000000,1000000+64)
+actual = h.pread(64, 1000000)
+check_same(actual, expected)
+actual = h.pread(61, 1000003)
+check_same(actual, expected[3:])
+actual = h.pread(60, 1000000)
+check_same(actual, expected[:60])
+
+# Same as above, but around offset 2G.
+offset = 2*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(60, offset)
+check_same(actual, expected[:60])
+
+# 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)
+
+'EOF'
--
2.42.0
3:44 p.m.
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>
--
Eric Blake, Principal Software Engineer
Red Hat, Inc.
Virtualization: qemu.org | libguestfs.org
3:54 p.m.
On Fri, Nov 03, 2023 at 03:44:44PM -0500, Eric Blake wrote:
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>
Thanks, pushed as 9d3c6bcc1 with your whitespace suggestions fixed.
Rich.
--
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-top is 'top' for virtual machines. Tiny program with many
powerful monitoring features, net stats, disk stats, logging, etc.
http://people.redhat.com/~rjones/virt-top
3:09 a.m.
New subject: [PATCH nbdkit v2] pattern: Accurately update b & offset in loop
On Fri, Nov 03, 2023 at 03:44:44PM -0500, Eric Blake wrote:
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.
FWIW, rather than reviewing code style, I can recommend just feeding
any python code through 'black'. It is an opinionated python code
formatter intended to bring a consistent style to all python programs.
With regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
4:33 a.m.
New subject: [PATCH nbdkit v2] pattern: Accurately update b & offset in loop
On Mon, Nov 06, 2023 at 09:09:28AM +0000, Daniel P. Berrangé wrote:
On Fri, Nov 03, 2023 at 03:44:44PM -0500, Eric Blake wrote:
> 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.
FWIW, rather than reviewing code style, I can recommend just feeding
any python code through 'black'. It is an opinionated python code
formatter intended to bring a consistent style to all python programs.
We use flake8 in the test suite, but it doesn't catch code embedded
inside shell scripts as happens here. I had a look at the
documentation just now but it doesn't seem there's any way to do that.
Rich.
--
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-p2v converts physical machines to virtual machines. Boot with a
live CD or over the network (PXE) and turn machines into KVM guests.
http://libguestfs.org/virt-v2v
4:42 a.m.
New subject: [PATCH nbdkit v2] pattern: Accurately update b & offset in loop
On Mon, Nov 06, 2023 at 10:33:13AM +0000, Richard W.M. Jones wrote:
On Mon, Nov 06, 2023 at 09:09:28AM +0000, Daniel P. Berrangé wrote:
> On Fri, Nov 03, 2023 at 03:44:44PM -0500, Eric Blake wrote:
> > 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.
>
> FWIW, rather than reviewing code style, I can recommend just feeding
> any python code through 'black'. It is an opinionated python code
> formatter intended to bring a consistent style to all python programs.
We use flake8 in the test suite, but it doesn't catch code embedded
inside shell scripts as happens here. I had a look at the
documentation just now but it doesn't seem there's any way to do that.
flake8 and black are complementary tools. Black exclusively considers
code formatting/layout, while flake8 does more semantic analysis. So
would typically run both of them. They only clash in a couple of places
and that's avoided with a $GITROOT/.flake8 config file
[flake8]
max-line-length = 88
extend-ignore = E203,W503
With regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
382
days inactive
385
days old
5 comments
3 participants
participants (3)
-
Daniel P. Berrangé -
Eric Blake -
Richard W.M. Jones