Add a test for the NBD client workaround in the previous patch.  It's
not really feasible for an iotest to assume a specific tracing engine,
so we can't really probe for the new
trace_nbd_parse_blockstatus_compliance to see if the server was fixed
vs. whether the client just worked around the server (other than by
rearranging order between code patches and this test). But having a
successful exchange sure beats the previous state of an error message.

Not tested yet, but worth adding to this test in future patches: an
NBD server that can advertise a non-sector-aligned size (such as
nbdkit) causes qemu as the NBD client to misbehave when it rounds the
size up and accesses beyond the advertised size. Qemu as NBD server
never advertises a non-sector-aligned size (since bdrv_getlength()
currently rounds up to sector boundaries); until qemu can act as such
a server, testing that flaw will have to rely on external binaries.

Signed-off-by: Eric Blake <eblake@redhat.com>
---
 tests/qemu-iotests/241     | 71 ++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/241.out |  7 ++++
 tests/qemu-iotests/group   |  1 +
 3 files changed, 79 insertions(+)
 create mode 100755 tests/qemu-iotests/241
 create mode 100644 tests/qemu-iotests/241.out

diff --git a/tests/qemu-iotests/241 b/tests/qemu-iotests/241
new file mode 100755
index 00000000000..b1f6e6452de
--- /dev/null
+++ b/tests/qemu-iotests/241
@@ -0,0 +1,71 @@
+#!/bin/bash
+#
+# Test qemu-nbd vs. unaligned images
+#
+# Copyright (C) 2018-2019 Red Hat, Inc.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+#
+
+seq="$(basename $0)"
+echo "QA output created by $seq"
+
+status=1 # failure is the default!
+
+nbd_unix_socket=$TEST_DIR/test_qemu_nbd_socket
+rm -f "${TEST_DIR}/qemu-nbd.pid"
+
+_cleanup()
+{
+    _cleanup_test_img
+    nbd_server_stop
+}
+trap "_cleanup; exit \$status" 0 1 2 3 15
+
+# get standard environment, filters and checks
+. ./common.rc
+. ./common.filter
+. ./common.nbd
+
+_supported_fmt raw
+_supported_proto nbd
+_supported_os Linux
+_require_command QEMU_NBD
+
+echo
+echo "=== Exporting unaligned raw image ==="
+echo
+
+# can't use _make_test_img, because qemu-img rounds image size up,
+# and because we want to use Unix socket rather than TCP port. Likewise,
+# we have to redirect TEST_IMG to our server.
+# This tests that we can deal with the hole at the end of an unaligned
+# raw file (either because the server doesn't advertise alignment too
+# large, or because the client ignores the server's noncompliance).
+printf %01000d 0 > "$TEST_IMG_FILE"
+nbd_server_start_unix_socket -f $IMGFMT -e 42 -x '' "$TEST_IMG_FILE"
+TEST_IMG="nbd:unix:$nbd_unix_socket"
+
+$QEMU_IMG map --output=json "$TEST_IMG" | _filter_qemu_img_map
+$QEMU_IO -c map "$TEST_IMG"
+
+# Not tested yet: we also want to ensure that qemu as NBD client does
+# not access beyond the end of a server's advertised unaligned size.
+# However, since qemu as server always rounds up to a sector alignment,
+# we would have to use nbdkit to provoke the current client failures.
+
+# success, all done
+echo '*** done'
+rm -f $seq.full
+status=0
diff --git a/tests/qemu-iotests/241.out b/tests/qemu-iotests/241.out
new file mode 100644
index 00000000000..044afc0c6f8
--- /dev/null
+++ b/tests/qemu-iotests/241.out
@@ -0,0 +1,7 @@
+QA output created by 241
+
+=== Exporting unaligned raw image ===
+
+[{ "start": 0, "length": 1024, "depth": 0, "zero": false, "data": true}]
+1 KiB (0x400) bytes     allocated at offset 0 bytes (0x0)
+*** done
diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
index 41da10c6cf5..bae77183809 100644
--- a/tests/qemu-iotests/group
+++ b/tests/qemu-iotests/group
@@ -240,6 +240,7 @@
 238 auto quick
 239 rw auto quick
 240 auto quick
+241 rw auto quick
 242 rw auto quick
 243 rw auto quick
 244 rw auto quick
-- 
2.20.1

28.03.2019 1:39, Eric Blake wrote:
> Add a test for the NBD client workaround in the previous patch.  It's
> not really feasible for an iotest to assume a specific tracing engine,
> so we can't really probe for the new
> trace_nbd_parse_blockstatus_compliance to see if the server was fixed
> vs. whether the client just worked around the server (other than by
> rearranging order between code patches and this test). But having a
> successful exchange sure beats the previous state of an error message.
> 
> Not tested yet, but worth adding to this test in future patches: an
> NBD server that can advertise a non-sector-aligned size (such as
> nbdkit) causes qemu as the NBD client to misbehave when it rounds the
> size up and accesses beyond the advertised size. Qemu as NBD server
> never advertises a non-sector-aligned size (since bdrv_getlength()
> currently rounds up to sector boundaries); until qemu can act as such
> a server, testing that flaw will have to rely on external binaries.
> 
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
>   tests/qemu-iotests/241     | 71 ++++++++++++++++++++++++++++++++++++++
>   tests/qemu-iotests/241.out |  7 ++++
>   tests/qemu-iotests/group   |  1 +
>   3 files changed, 79 insertions(+)
>   create mode 100755 tests/qemu-iotests/241
>   create mode 100644 tests/qemu-iotests/241.out
> 
> diff --git a/tests/qemu-iotests/241 b/tests/qemu-iotests/241
> new file mode 100755
> index 00000000000..b1f6e6452de
> --- /dev/null
> +++ b/tests/qemu-iotests/241
> @@ -0,0 +1,71 @@
> +#!/bin/bash
> +#
> +# Test qemu-nbd vs. unaligned images
> +#
> +# Copyright (C) 2018-2019 Red Hat, Inc.
> +#
> +# This program is free software; you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation; either version 2 of the License, or
> +# (at your option) any later version.
> +#
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program.  If not, see <http://www.gnu.org/licenses/>.
> +#
> +
> +seq="$(basename $0)"
> +echo "QA output created by $seq"
> +
> +status=1 # failure is the default!
> +
> +nbd_unix_socket=$TEST_DIR/test_qemu_nbd_socket
> +rm -f "${TEST_DIR}/qemu-nbd.pid"

hmm, strange that we need to remove something from test directory at start.

> +
> +_cleanup()
> +{
> +    _cleanup_test_img
> +    nbd_server_stop
> +}
> +trap "_cleanup; exit \$status" 0 1 2 3 15
> +
> +# get standard environment, filters and checks
> +. ./common.rc
> +. ./common.filter
> +. ./common.nbd
> +
> +_supported_fmt raw
> +_supported_proto nbd
> +_supported_os Linux
> +_require_command QEMU_NBD
> +
> +echo
> +echo "=== Exporting unaligned raw image ==="
> +echo
> +
> +# can't use _make_test_img, because qemu-img rounds image size up,
> +# and because we want to use Unix socket rather than TCP port. Likewise,
> +# we have to redirect TEST_IMG to our server.
> +# This tests that we can deal with the hole at the end of an unaligned
> +# raw file (either because the server doesn't advertise alignment too
> +# large, or because the client ignores the server's noncompliance).
> +printf %01000d 0 > "$TEST_IMG_FILE"
> +nbd_server_start_unix_socket -f $IMGFMT -e 42 -x '' "$TEST_IMG_FILE"

why do we need -e 42? we don't have more than one client simultaneously..
and -x '' is the default anyway

> +TEST_IMG="nbd:unix:$nbd_unix_socket"
> +
> +$QEMU_IMG map --output=json "$TEST_IMG" | _filter_qemu_img_map

is filter needed? seems like not

> +$QEMU_IO -c map "$TEST_IMG"
> +
> +# Not tested yet: we also want to ensure that qemu as NBD client does
> +# not access beyond the end of a server's advertised unaligned size.
> +# However, since qemu as server always rounds up to a sector alignment,
> +# we would have to use nbdkit to provoke the current client failures.
> +
> +# success, all done
> +echo '*** done'
> +rm -f $seq.full
> +status=0
> diff --git a/tests/qemu-iotests/241.out b/tests/qemu-iotests/241.out
> new file mode 100644
> index 00000000000..044afc0c6f8
> --- /dev/null
> +++ b/tests/qemu-iotests/241.out
> @@ -0,0 +1,7 @@
> +QA output created by 241
> +
> +=== Exporting unaligned raw image ===
> +
> +[{ "start": 0, "length": 1024, "depth": 0, "zero": false, "data": true}]
> +1 KiB (0x400) bytes     allocated at offset 0 bytes (0x0)
> +*** done
> diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
> index 41da10c6cf5..bae77183809 100644
> --- a/tests/qemu-iotests/group
> +++ b/tests/qemu-iotests/group
> @@ -240,6 +240,7 @@
>   238 auto quick
>   239 rw auto quick
>   240 auto quick
> +241 rw auto quick
>   242 rw auto quick
>   243 rw auto quick
>   244 rw auto quick
> 


-- 
Best regards,
Vladimir

On 3/28/19 3:20 AM, Vladimir Sementsov-Ogievskiy wrote:
> 28.03.2019 1:39, Eric Blake wrote:
>> Add a test for the NBD client workaround in the previous patch.  It's
>> not really feasible for an iotest to assume a specific tracing engine,
>> so we can't really probe for the new
>> trace_nbd_parse_blockstatus_compliance to see if the server was fixed
>> vs. whether the client just worked around the server (other than by
>> rearranging order between code patches and this test). But having a
>> successful exchange sure beats the previous state of an error message.
>>

>> +seq="$(basename $0)"
>> +echo "QA output created by $seq"
>> +
>> +status=1 # failure is the default!
>> +
>> +nbd_unix_socket=$TEST_DIR/test_qemu_nbd_socket
>> +rm -f "${TEST_DIR}/qemu-nbd.pid"
> 
> hmm, strange that we need to remove something from test directory at start.

Well, until we follow through with our thread of implementing per-test
scratch directories for iotests, it proved invaluable to me during
testing (as a failed test does not properly clean up after itself).


>> +# can't use _make_test_img, because qemu-img rounds image size up,
>> +# and because we want to use Unix socket rather than TCP port. Likewise,
>> +# we have to redirect TEST_IMG to our server.
>> +# This tests that we can deal with the hole at the end of an unaligned
>> +# raw file (either because the server doesn't advertise alignment too
>> +# large, or because the client ignores the server's noncompliance).
>> +printf %01000d 0 > "$TEST_IMG_FILE"
>> +nbd_server_start_unix_socket -f $IMGFMT -e 42 -x '' "$TEST_IMG_FILE"
> 
> why do we need -e 42? we don't have more than one client simultaneously..

Matches what other tests use. You are right that I could get away
without it, though.

> and -x '' is the default anyway

Ditto.

> 
>> +TEST_IMG="nbd:unix:$nbd_unix_socket"
>> +
>> +$QEMU_IMG map --output=json "$TEST_IMG" | _filter_qemu_img_map
> 
> is filter needed? seems like not

I'd rather keep the filter (makes it easier to copy-and-paste between
other tests, especially if it does matter down the road)

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

Am 28.03.2019 um 19:27 hat Eric Blake geschrieben:
> On 3/28/19 3:20 AM, Vladimir Sementsov-Ogievskiy wrote:
> > 28.03.2019 1:39, Eric Blake wrote:
> >> Add a test for the NBD client workaround in the previous patch.  It's
> >> not really feasible for an iotest to assume a specific tracing engine,
> >> so we can't really probe for the new
> >> trace_nbd_parse_blockstatus_compliance to see if the server was fixed
> >> vs. whether the client just worked around the server (other than by
> >> rearranging order between code patches and this test). But having a
> >> successful exchange sure beats the previous state of an error message.
> >>
> 
> >> +seq="$(basename $0)"
> >> +echo "QA output created by $seq"
> >> +
> >> +status=1 # failure is the default!
> >> +
> >> +nbd_unix_socket=$TEST_DIR/test_qemu_nbd_socket
> >> +rm -f "${TEST_DIR}/qemu-nbd.pid"
> > 
> > hmm, strange that we need to remove something from test directory at start.
> 
> Well, until we follow through with our thread of implementing per-test
> scratch directories for iotests, it proved invaluable to me during
> testing (as a failed test does not properly clean up after itself).

Maybe that's the thing to fix then?

Though I'm not sure why, as you do call nbd_server_stop in _cleanup.
Does this mean that _cleanup wasn't called at all in your failure case?

Kevin

On 3/29/19 2:58 AM, Kevin Wolf wrote:

>>>> +seq="$(basename $0)"
>>>> +echo "QA output created by $seq"
>>>> +
>>>> +status=1 # failure is the default!
>>>> +
>>>> +nbd_unix_socket=$TEST_DIR/test_qemu_nbd_socket
>>>> +rm -f "${TEST_DIR}/qemu-nbd.pid"
>>>
>>> hmm, strange that we need to remove something from test directory at start.
>>
>> Well, until we follow through with our thread of implementing per-test
>> scratch directories for iotests, it proved invaluable to me during
>> testing (as a failed test does not properly clean up after itself).
> 
> Maybe that's the thing to fix then?
> 
> Though I'm not sure why, as you do call nbd_server_stop in _cleanup.
> Does this mean that _cleanup wasn't called at all in your failure case?
> 

I've dropped the line from my v3 posting. When I wrote v1, it was back
in the 3.1 timeframe, and pre-dated Dan's improvements to factor out NBD
functions into a reusable common file. Looks like I may have just missed
that during rebasing.


-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org