[PATCH v2 0/4] Fix convert to qcow2 compressed to NBD

[PATCH v2 0/4] Fix convert to qcow2 compressed to NBD

[Nir Soffer]

Fix qemu-img convert -O qcow2 -c to NBD URL and add missing test for this
usage.

This already works now, but unfortunately qemu-img fails when trying to
truncate the target image to the same size at the end of the operation.

Changes since v1:
- Include complete code for creating OVA file [Eric]
- Use qcow2 for source file to avoid issues with random CI filesystem [Max]
- Fix many typos [Eric, Max]
- Make qemu_nbd_popen a context manager
- Add more qemu_img_* helpers
- Verify OVA file contents

v1 was here:
https://lists.nongnu.org/archive/html/qemu-block/2020-07/msg01543.html

Nir Soffer (4):
  block: nbd: Fix convert qcow2 compressed to nbd
  iotests: Make qemu_nbd_popen() a contextmanager
  iotests: Add more qemu_img helpers
  iotests: Test convert to qcow2 compressed to NBD

 block/nbd.c                   |  30 ++++++++
 tests/qemu-iotests/264        |  76 ++++++++------------
 tests/qemu-iotests/264.out    |   2 +
 tests/qemu-iotests/302        | 127 ++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/302.out    |  31 +++++++++
 tests/qemu-iotests/group      |   1 +
 tests/qemu-iotests/iotests.py |  34 ++++++++-
 7 files changed, 251 insertions(+), 50 deletions(-)
 create mode 100755 tests/qemu-iotests/302
 create mode 100644 tests/qemu-iotests/302.out

-- 
2.25.4

[PATCH v2 1/4] block: nbd: Fix convert qcow2 compressed to nbd

[Nir Soffer]

When converting to qcow2 compressed format, the last step is a special
zero length compressed write, ending in call to bdrv_co_truncate(). This
call always fails for the nbd driver since it does not implement
bdrv_co_truncate().

For block devices, which have the same limits, the call succeeds since
file driver implements bdrv_co_truncate(). If the caller asked to
truncate to the same or smaller size with exact=false, the truncate
succeeds. Implement the same logic for nbd.

Example failing without this change:

In one shell starts qemu-nbd:

$ truncate -s 1g test.tar
$ qemu-nbd --socket=/tmp/nbd.sock --persistent --format=raw --offset 1536 test.tar

In another shell convert an image to qcow2 compressed via NBD:

$ echo "disk data" > disk.raw
$ truncate -s 1g disk.raw
$ qemu-img convert -f raw -O qcow2 -c disk1.raw nbd+unix:///?socket=/tmp/nbd.sock; echo $?
1

qemu-img failed, but the conversion was successful:

$ qemu-img info nbd+unix:///?socket=/tmp/nbd.sock
image: nbd+unix://?socket=/tmp/nbd.sock
file format: qcow2
virtual size: 1 GiB (1073741824 bytes)
...

$ qemu-img check nbd+unix:///?socket=/tmp/nbd.sock
No errors were found on the image.
1/16384 = 0.01% allocated, 100.00% fragmented, 100.00% compressed clusters
Image end offset: 393216

$ qemu-img compare disk.raw nbd+unix:///?socket=/tmp/nbd.sock
Images are identical.

Fixes: https://bugzilla.redhat.com/1860627
Signed-off-by: Nir Soffer <nsoffer@redhat.com>
---
 block/nbd.c | 30 ++++++++++++++++++++++++++++++
 1 file changed, 30 insertions(+)

diff --git a/block/nbd.c b/block/nbd.c
index 65a4f56924..dcb0b03641 100644
--- a/block/nbd.c
+++ b/block/nbd.c
@@ -1966,6 +1966,33 @@ static void nbd_close(BlockDriverState *bs)
     nbd_clear_bdrvstate(s);
 }
 
+/*
+ * NBD cannot truncate, but if the caller asks to truncate to the same size, or
+ * to a smaller size with exact=false, there is no reason to fail the
+ * operation.
+ *
+ * Preallocation mode is ignored since it does not seems useful to fail when
+ * when never change anything.
+ */
+static int coroutine_fn nbd_co_truncate(BlockDriverState *bs, int64_t offset,
+                                        bool exact, PreallocMode prealloc,
+                                        BdrvRequestFlags flags, Error **errp)
+{
+    BDRVNBDState *s = bs->opaque;
+
+    if (offset != s->info.size && exact) {
+        error_setg(errp, "Cannot resize NBD nodes");
+        return -ENOTSUP;
+    }
+
+    if (offset > s->info.size) {
+        error_setg(errp, "Cannot grow NBD nodes");
+        return -EINVAL;
+    }
+
+    return 0;
+}
+
 static int64_t nbd_getlength(BlockDriverState *bs)
 {
     BDRVNBDState *s = bs->opaque;
@@ -2045,6 +2072,7 @@ static BlockDriver bdrv_nbd = {
     .bdrv_co_flush_to_os        = nbd_co_flush,
     .bdrv_co_pdiscard           = nbd_client_co_pdiscard,
     .bdrv_refresh_limits        = nbd_refresh_limits,
+    .bdrv_co_truncate           = nbd_co_truncate,
     .bdrv_getlength             = nbd_getlength,
     .bdrv_detach_aio_context    = nbd_client_detach_aio_context,
     .bdrv_attach_aio_context    = nbd_client_attach_aio_context,
@@ -2072,6 +2100,7 @@ static BlockDriver bdrv_nbd_tcp = {
     .bdrv_co_flush_to_os        = nbd_co_flush,
     .bdrv_co_pdiscard           = nbd_client_co_pdiscard,
     .bdrv_refresh_limits        = nbd_refresh_limits,
+    .bdrv_co_truncate           = nbd_co_truncate,
     .bdrv_getlength             = nbd_getlength,
     .bdrv_detach_aio_context    = nbd_client_detach_aio_context,
     .bdrv_attach_aio_context    = nbd_client_attach_aio_context,
@@ -2099,6 +2128,7 @@ static BlockDriver bdrv_nbd_unix = {
     .bdrv_co_flush_to_os        = nbd_co_flush,
     .bdrv_co_pdiscard           = nbd_client_co_pdiscard,
     .bdrv_refresh_limits        = nbd_refresh_limits,
+    .bdrv_co_truncate           = nbd_co_truncate,
     .bdrv_getlength             = nbd_getlength,
     .bdrv_detach_aio_context    = nbd_client_detach_aio_context,
     .bdrv_attach_aio_context    = nbd_client_attach_aio_context,
-- 
2.25.4

[PATCH v2 2/4] iotests: Make qemu_nbd_popen() a contextmanager

[Nir Soffer]

Instead of duplicating the code to wait until the server is ready and
remember to terminate the server and wait for it, make it possible to
use like this:

    with qemu_nbd_popen('-k', sock, image):
        # Access image via qemu-nbd socket...

Only test 264 used this helper, but I had to modify the output since it
did not consistently when starting and stopping qemu-nbd.

Signed-off-by: Nir Soffer <nsoffer@redhat.com>
---
 tests/qemu-iotests/264        | 76 +++++++++++++----------------------
 tests/qemu-iotests/264.out    |  2 +
 tests/qemu-iotests/iotests.py | 28 ++++++++++++-
 3 files changed, 56 insertions(+), 50 deletions(-)

diff --git a/tests/qemu-iotests/264 b/tests/qemu-iotests/264
index 304a7443d7..666f164ed8 100755
--- a/tests/qemu-iotests/264
+++ b/tests/qemu-iotests/264
@@ -36,48 +36,32 @@ wait_step = 0.2
 
 qemu_img_create('-f', iotests.imgfmt, disk_a, str(size))
 qemu_img_create('-f', iotests.imgfmt, disk_b, str(size))
-srv = qemu_nbd_popen('-k', nbd_sock, '-f', iotests.imgfmt, disk_b)
 
-# Wait for NBD server availability
-t = 0
-ok = False
-while t < wait_limit:
-    ok = qemu_io_silent_check('-f', 'raw', '-c', 'read 0 512', nbd_uri)
-    if ok:
-        break
-    time.sleep(wait_step)
-    t += wait_step
+with qemu_nbd_popen('-k', nbd_sock, '-f', iotests.imgfmt, disk_b):
+    vm = iotests.VM().add_drive(disk_a)
+    vm.launch()
+    vm.hmp_qemu_io('drive0', 'write 0 {}'.format(size))
+
+    vm.qmp_log('blockdev-add', filters=[iotests.filter_qmp_testfiles],
+               **{'node_name': 'backup0',
+                  'driver': 'raw',
+                  'file': {'driver': 'nbd',
+                           'server': {'type': 'unix', 'path': nbd_sock},
+                           'reconnect-delay': 10}})
+    vm.qmp_log('blockdev-backup', device='drive0', sync='full', target='backup0',
+               speed=(1 * 1024 * 1024))
+
+    # Wait for some progress
+    t = 0
+    while t < wait_limit:
+        jobs = vm.qmp('query-block-jobs')['return']
+        if jobs and jobs[0]['offset'] > 0:
+            break
+        time.sleep(wait_step)
+        t += wait_step
 
-assert ok
-
-vm = iotests.VM().add_drive(disk_a)
-vm.launch()
-vm.hmp_qemu_io('drive0', 'write 0 {}'.format(size))
-
-vm.qmp_log('blockdev-add', filters=[iotests.filter_qmp_testfiles],
-           **{'node_name': 'backup0',
-              'driver': 'raw',
-              'file': {'driver': 'nbd',
-                       'server': {'type': 'unix', 'path': nbd_sock},
-                       'reconnect-delay': 10}})
-vm.qmp_log('blockdev-backup', device='drive0', sync='full', target='backup0',
-           speed=(1 * 1024 * 1024))
-
-# Wait for some progress
-t = 0
-while t < wait_limit:
-    jobs = vm.qmp('query-block-jobs')['return']
     if jobs and jobs[0]['offset'] > 0:
-        break
-    time.sleep(wait_step)
-    t += wait_step
-
-if jobs and jobs[0]['offset'] > 0:
-    log('Backup job is started')
-
-log('Kill NBD server')
-srv.kill()
-srv.wait()
+        log('Backup job is started')
 
 jobs = vm.qmp('query-block-jobs')['return']
 if jobs and jobs[0]['offset'] < jobs[0]['len']:
@@ -88,12 +72,8 @@ vm.qmp_log('block-job-set-speed', device='drive0', speed=0)
 # Emulate server down time for 1 second
 time.sleep(1)
 
-log('Start NBD server')
-srv = qemu_nbd_popen('-k', nbd_sock, '-f', iotests.imgfmt, disk_b)
-
-e = vm.event_wait('BLOCK_JOB_COMPLETED')
-log('Backup completed: {}'.format(e['data']['offset']))
-
-vm.qmp_log('blockdev-del', node_name='backup0')
-srv.kill()
-vm.shutdown()
+with qemu_nbd_popen('-k', nbd_sock, '-f', iotests.imgfmt, disk_b):
+    e = vm.event_wait('BLOCK_JOB_COMPLETED')
+    log('Backup completed: {}'.format(e['data']['offset']))
+    vm.qmp_log('blockdev-del', node_name='backup0')
+    vm.shutdown()
diff --git a/tests/qemu-iotests/264.out b/tests/qemu-iotests/264.out
index 3000944b09..c45b1e81ef 100644
--- a/tests/qemu-iotests/264.out
+++ b/tests/qemu-iotests/264.out
@@ -1,3 +1,4 @@
+Start NBD server
 {"execute": "blockdev-add", "arguments": {"driver": "raw", "file": {"driver": "nbd", "reconnect-delay": 10, "server": {"path": "TEST_DIR/PID-nbd-sock", "type": "unix"}}, "node-name": "backup0"}}
 {"return": {}}
 {"execute": "blockdev-backup", "arguments": {"device": "drive0", "speed": 1048576, "sync": "full", "target": "backup0"}}
@@ -11,3 +12,4 @@ Start NBD server
 Backup completed: 5242880
 {"execute": "blockdev-del", "arguments": {"node-name": "backup0"}}
 {"return": {}}
+Kill NBD server
diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index 3590ed78a0..8f79668435 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -28,10 +28,13 @@ import signal
 import struct
 import subprocess
 import sys
+import time
 from typing import (Any, Callable, Dict, Iterable,
                     List, Optional, Sequence, Tuple, TypeVar)
 import unittest
 
+from contextlib import contextmanager
+
 # pylint: disable=import-error, wrong-import-position
 sys.path.append(os.path.join(os.path.dirname(__file__), '..', '..', 'python'))
 from qemu import qtest
@@ -270,9 +273,30 @@ def qemu_nbd_early_pipe(*args):
 
     return subp.returncode, output if subp.returncode else ''
 
+@contextmanager
 def qemu_nbd_popen(*args):
-    '''Run qemu-nbd in daemon mode and return the parent's exit code'''
-    return subprocess.Popen(qemu_nbd_args + ['--persistent'] + list(args))
+    '''Context manager running qemu-nbd within the context'''
+    pid_file = file_path("pid")
+
+    cmd = list(qemu_nbd_args)
+    cmd.extend(('--persistent', '--pid-file', pid_file))
+    cmd.extend(args)
+
+    log('Start NBD server')
+    p = subprocess.Popen(cmd)
+    try:
+        while not os.path.exists(pid_file):
+            if p.poll() is not None:
+                raise RuntimeError(
+                    "qemu-nbd terminated with exit code {}: {}"
+                    .format(p.returncode, ' '.join(cmd)))
+
+            time.sleep(0.01)
+        yield
+    finally:
+        log('Kill NBD server')
+        p.kill()
+        p.wait()
 
 def compare_images(img1, img2, fmt1=imgfmt, fmt2=imgfmt):
     '''Return True if two image files are identical'''
-- 
2.25.4

[PATCH v2 3/4] iotests: Add more qemu_img helpers

[Nir Soffer]

Add 2 helpers for measuring and checking images:
- qemu_img_measure()
- qemu_img_check()

Both use --output-json and parse the returned json to make easy to use
in other tests. I'm going to use them in a new test, and I hope they
will be useful in may other tests.

Signed-off-by: Nir Soffer <nsoffer@redhat.com>
---
 tests/qemu-iotests/iotests.py | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index 8f79668435..717b5b652c 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -141,6 +141,12 @@ def qemu_img_create(*args):
 
     return qemu_img(*args)
 
+def qemu_img_measure(*args):
+    return json.loads(qemu_img_pipe("measure", "--output", "json", *args))
+
+def qemu_img_check(*args):
+    return json.loads(qemu_img_pipe("check", "--output", "json", *args))
+
 def qemu_img_verbose(*args):
     '''Run qemu-img without suppressing its output and return the exit code'''
     exitcode = subprocess.call(qemu_img_args + list(args))
-- 
2.25.4

[PATCH v2 4/4] iotests: Test convert to qcow2 compressed to NBD

[Nir Soffer]

Add test for "qemu-img convert -O qcow2 -c" to NBD target. The tests    
create a OVA file and write compressed qcow2 disk content directly into
the OVA file via qemu-nbd.

Signed-off-by: Nir Soffer <nsoffer@redhat.com>
---
 tests/qemu-iotests/302     | 127 +++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/302.out |  31 +++++++++
 tests/qemu-iotests/group   |   1 +
 3 files changed, 159 insertions(+)
 create mode 100755 tests/qemu-iotests/302
 create mode 100644 tests/qemu-iotests/302.out

diff --git a/tests/qemu-iotests/302 b/tests/qemu-iotests/302
new file mode 100755
index 0000000000..a8506bda15
--- /dev/null
+++ b/tests/qemu-iotests/302
@@ -0,0 +1,127 @@
+#!/usr/bin/env python3
+#
+# Tests converting qcow2 compressed to NBD
+#
+# Copyright (c) 2020 Nir Soffer <nirsof@gmail.com>
+#
+# 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/>.
+#
+# owner=nirsof@gmail.com
+
+import io
+import tarfile
+
+import iotests
+
+from iotests import (
+    file_path,
+    qemu_img,
+    qemu_img_check,
+    qemu_img_create,
+    qemu_img_log,
+    qemu_img_measure,
+    qemu_io,
+    qemu_nbd_popen,
+)
+
+iotests.script_initialize(supported_fmts=["qcow2"])
+
+# Create source disk. Using qcow2 to enable strict comparing later, and
+# avoid issues with random filesystem on CI environment.
+src_disk = file_path("disk.qcow2")
+qemu_img_create("-f", iotests.imgfmt, src_disk, "1g")
+qemu_io("-f", iotests.imgfmt, "-c", "write 1m 64k", src_disk)
+
+# The use case is writing qcow2 image directly into an ova file, which
+# is a tar file with specific layout. This is tricky since we don't know the
+# size of the image before compressing, so we have to do:
+# 1. Add an ovf file.
+# 2. Find the offset of the next member data.
+# 3. Make room for image data, allocating for the worst case.
+# 4. Write compressed image data into the tar.
+# 5. Add a tar entry with the actual image size.
+# 6. Shrink the tar to the actual size, aligned to 512 bytes.
+
+tar_file = file_path("test.ova")
+
+with tarfile.open(tar_file, "w") as tar:
+
+    # 1. Add an ovf file.
+
+    ovf_data = b"<xml/>"
+    ovf = tarfile.TarInfo("vm.ovf")
+    ovf.size = len(ovf_data)
+    tar.addfile(ovf, io.BytesIO(ovf_data))
+
+    # 2. Find the offset of the next member data.
+
+    offset = tar.fileobj.tell() + 512
+
+    # 3. Make room for image data, allocating for the worst case.
+
+    measure = qemu_img_measure("-O", "qcow2", src_disk)
+    tar.fileobj.truncate(offset + measure["required"])
+
+    # 4. Write compressed image data into the tar.
+
+    nbd_sock = file_path("nbd-sock", base_dir=iotests.sock_dir)
+    nbd_uri = "nbd+unix:///exp?socket=" + nbd_sock
+
+    # Use raw format to allow creating qcow2 directly into tar file.
+    with qemu_nbd_popen(
+            "--socket", nbd_sock,
+            "--export-name", "exp",
+            "--format", "raw",
+            "--offset", str(offset),
+            tar_file):
+
+        iotests.log("=== Target image info ===")
+        qemu_img_log("info", nbd_uri)
+
+        qemu_img(
+            "convert",
+            "-f", iotests.imgfmt,
+            "-O", "qcow2",
+            "-c",
+            src_disk,
+            nbd_uri)
+
+        iotests.log("=== Converted image info ===")
+        qemu_img_log("info", nbd_uri)
+
+        iotests.log("=== Converted image check ===")
+        qemu_img_log("check", nbd_uri)
+
+        iotests.log("=== Comparing to source disk ===")
+        qemu_img_log("compare", src_disk, nbd_uri)
+
+        actual_size = qemu_img_check(nbd_uri)["image-end-offset"]
+
+    # 5. Add a tar entry with the actual image size.
+
+    disk = tarfile.TarInfo("disk")
+    disk.size = actual_size
+    tar.addfile(disk)
+
+    # 6. Shrink the tar to the actual size, aligned to 512 bytes.
+
+    tar_size = offset + (disk.size + 511) & ~511
+    tar.fileobj.seek(tar_size)
+    tar.fileobj.truncate(tar_size)
+
+with tarfile.open(tar_file) as tar:
+    members = [{"name": m.name, "size": m.size, "offset": m.offset_data}
+               for m in tar]
+    iotests.log("=== OVA file contents ===")
+    iotests.log(members)
diff --git a/tests/qemu-iotests/302.out b/tests/qemu-iotests/302.out
new file mode 100644
index 0000000000..e37d3a1030
--- /dev/null
+++ b/tests/qemu-iotests/302.out
@@ -0,0 +1,31 @@
+Start NBD server
+=== Target image info ===
+image: nbd+unix:///exp?socket=SOCK_DIR/PID-nbd-sock
+file format: raw
+virtual size: 448 KiB (458752 bytes)
+disk size: unavailable
+
+=== Converted image info ===
+image: nbd+unix:///exp?socket=SOCK_DIR/PID-nbd-sock
+file format: qcow2
+virtual size: 1 GiB (1073741824 bytes)
+disk size: unavailable
+cluster_size: 65536
+Format specific information:
+    compat: 1.1
+    compression type: zlib
+    lazy refcounts: false
+    refcount bits: 16
+    corrupt: false
+
+=== Converted image check ===
+No errors were found on the image.
+1/16384 = 0.01% allocated, 100.00% fragmented, 100.00% compressed clusters
+Image end offset: 393216
+
+=== Comparing to source disk ===
+Images are identical.
+
+Kill NBD server
+=== OVA file contents ===
+[{"name": "vm.ovf", "offset": 512, "size": 6}, {"name": "disk", "offset": 1536, "size": 393216}]
diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
index 1d0252e1f0..1e1cb27bc8 100644
--- a/tests/qemu-iotests/group
+++ b/tests/qemu-iotests/group
@@ -308,3 +308,4 @@
 297 meta
 299 auto quick
 301 backing quick
+302 quick
-- 
2.25.4

Re: [PATCH v2 1/4] block: nbd: Fix convert qcow2 compressed to nbd

[Vladimir Sementsov-Ogievskiy]

28.07.2020 00:58, Nir Soffer wrote:
> When converting to qcow2 compressed format, the last step is a special
> zero length compressed write, ending in call to bdrv_co_truncate(). This
> call always fails for the nbd driver since it does not implement
> bdrv_co_truncate().
> 
> For block devices, which have the same limits, the call succeeds since
> file driver implements bdrv_co_truncate(). If the caller asked to
> truncate to the same or smaller size with exact=false, the truncate
> succeeds. Implement the same logic for nbd.
> 
> Example failing without this change:
> 
> In one shell starts qemu-nbd:
> 
> $ truncate -s 1g test.tar
> $ qemu-nbd --socket=/tmp/nbd.sock --persistent --format=raw --offset 1536 test.tar
> 
> In another shell convert an image to qcow2 compressed via NBD:
> 
> $ echo "disk data" > disk.raw
> $ truncate -s 1g disk.raw
> $ qemu-img convert -f raw -O qcow2 -c disk1.raw nbd+unix:///?socket=/tmp/nbd.sock; echo $?
> 1
> 
> qemu-img failed, but the conversion was successful:
> 
> $ qemu-img info nbd+unix:///?socket=/tmp/nbd.sock
> image: nbd+unix://?socket=/tmp/nbd.sock
> file format: qcow2
> virtual size: 1 GiB (1073741824 bytes)
> ...
> 
> $ qemu-img check nbd+unix:///?socket=/tmp/nbd.sock
> No errors were found on the image.
> 1/16384 = 0.01% allocated, 100.00% fragmented, 100.00% compressed clusters
> Image end offset: 393216
> 
> $ qemu-img compare disk.raw nbd+unix:///?socket=/tmp/nbd.sock
> Images are identical.
> 
> Fixes: https://bugzilla.redhat.com/1860627
> Signed-off-by: Nir Soffer <nsoffer@redhat.com>
> ---
>   block/nbd.c | 30 ++++++++++++++++++++++++++++++
>   1 file changed, 30 insertions(+)
> 
> diff --git a/block/nbd.c b/block/nbd.c
> index 65a4f56924..dcb0b03641 100644
> --- a/block/nbd.c
> +++ b/block/nbd.c
> @@ -1966,6 +1966,33 @@ static void nbd_close(BlockDriverState *bs)
>       nbd_clear_bdrvstate(s);
>   }
>   
> +/*
> + * NBD cannot truncate, but if the caller asks to truncate to the same size, or
> + * to a smaller size with exact=false, there is no reason to fail the
> + * operation.
> + *
> + * Preallocation mode is ignored since it does not seems useful to fail when
> + * when never change anything.
> + */
> +static int coroutine_fn nbd_co_truncate(BlockDriverState *bs, int64_t offset,
> +                                        bool exact, PreallocMode prealloc,
> +                                        BdrvRequestFlags flags, Error **errp)
> +{
> +    BDRVNBDState *s = bs->opaque;
> +
> +    if (offset != s->info.size && exact) {
> +        error_setg(errp, "Cannot resize NBD nodes");
> +        return -ENOTSUP;
> +    }
> +
> +    if (offset > s->info.size) {
> +        error_setg(errp, "Cannot grow NBD nodes");
> +        return -EINVAL;
> +    }

I think that ENOTSUP actually is valid error code for both cases.. NBD protocol has experimental extension NBD_CMD_RESIZE, so one day we'll implement this. So, I think, it's not invalid, but just not supported yet. Still, not a big deal, so with ENOTSUP or EINVAL:
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

Also, may be better to manage it in generic layer:
If driver doesn't implement bdrv_co_truncate handler (or return ENOTSUP), and we are truncating to the same size, or to smaller size with exact=false, we just report success? Then we'll drop same code from file-posix.c

> +
> +    return 0;
> +}
> +
>   static int64_t nbd_getlength(BlockDriverState *bs)
>   {
>       BDRVNBDState *s = bs->opaque;
> @@ -2045,6 +2072,7 @@ static BlockDriver bdrv_nbd = {
>       .bdrv_co_flush_to_os        = nbd_co_flush,
>       .bdrv_co_pdiscard           = nbd_client_co_pdiscard,
>       .bdrv_refresh_limits        = nbd_refresh_limits,
> +    .bdrv_co_truncate           = nbd_co_truncate,
>       .bdrv_getlength             = nbd_getlength,
>       .bdrv_detach_aio_context    = nbd_client_detach_aio_context,
>       .bdrv_attach_aio_context    = nbd_client_attach_aio_context,
> @@ -2072,6 +2100,7 @@ static BlockDriver bdrv_nbd_tcp = {
>       .bdrv_co_flush_to_os        = nbd_co_flush,
>       .bdrv_co_pdiscard           = nbd_client_co_pdiscard,
>       .bdrv_refresh_limits        = nbd_refresh_limits,
> +    .bdrv_co_truncate           = nbd_co_truncate,
>       .bdrv_getlength             = nbd_getlength,
>       .bdrv_detach_aio_context    = nbd_client_detach_aio_context,
>       .bdrv_attach_aio_context    = nbd_client_attach_aio_context,
> @@ -2099,6 +2128,7 @@ static BlockDriver bdrv_nbd_unix = {
>       .bdrv_co_flush_to_os        = nbd_co_flush,
>       .bdrv_co_pdiscard           = nbd_client_co_pdiscard,
>       .bdrv_refresh_limits        = nbd_refresh_limits,
> +    .bdrv_co_truncate           = nbd_co_truncate,
>       .bdrv_getlength             = nbd_getlength,
>       .bdrv_detach_aio_context    = nbd_client_detach_aio_context,
>       .bdrv_attach_aio_context    = nbd_client_attach_aio_context,
> 


-- 
Best regards,
Vladimir

Re: [PATCH v2 2/4] iotests: Make qemu_nbd_popen() a contextmanager

[Vladimir Sementsov-Ogievskiy]

28.07.2020 00:58, Nir Soffer wrote:
> Instead of duplicating the code to wait until the server is ready and
> remember to terminate the server and wait for it, make it possible to
> use like this:
> 
>      with qemu_nbd_popen('-k', sock, image):
>          # Access image via qemu-nbd socket...
> 
> Only test 264 used this helper, but I had to modify the output since it
> did not consistently when starting and stopping qemu-nbd.
> 
> Signed-off-by: Nir Soffer <nsoffer@redhat.com>
> ---
>   tests/qemu-iotests/264        | 76 +++++++++++++----------------------
>   tests/qemu-iotests/264.out    |  2 +
>   tests/qemu-iotests/iotests.py | 28 ++++++++++++-
>   3 files changed, 56 insertions(+), 50 deletions(-)
> 
> diff --git a/tests/qemu-iotests/264 b/tests/qemu-iotests/264
> index 304a7443d7..666f164ed8 100755
> --- a/tests/qemu-iotests/264
> +++ b/tests/qemu-iotests/264
> @@ -36,48 +36,32 @@ wait_step = 0.2
>   
>   qemu_img_create('-f', iotests.imgfmt, disk_a, str(size))
>   qemu_img_create('-f', iotests.imgfmt, disk_b, str(size))
> -srv = qemu_nbd_popen('-k', nbd_sock, '-f', iotests.imgfmt, disk_b)
>   
> -# Wait for NBD server availability
> -t = 0
> -ok = False
> -while t < wait_limit:
> -    ok = qemu_io_silent_check('-f', 'raw', '-c', 'read 0 512', nbd_uri)
> -    if ok:
> -        break
> -    time.sleep(wait_step)
> -    t += wait_step
> +with qemu_nbd_popen('-k', nbd_sock, '-f', iotests.imgfmt, disk_b):
> +    vm = iotests.VM().add_drive(disk_a)
> +    vm.launch()
> +    vm.hmp_qemu_io('drive0', 'write 0 {}'.format(size))
> +
> +    vm.qmp_log('blockdev-add', filters=[iotests.filter_qmp_testfiles],
> +               **{'node_name': 'backup0',
> +                  'driver': 'raw',
> +                  'file': {'driver': 'nbd',
> +                           'server': {'type': 'unix', 'path': nbd_sock},
> +                           'reconnect-delay': 10}})
> +    vm.qmp_log('blockdev-backup', device='drive0', sync='full', target='backup0',
> +               speed=(1 * 1024 * 1024))
> +
> +    # Wait for some progress
> +    t = 0
> +    while t < wait_limit:
> +        jobs = vm.qmp('query-block-jobs')['return']
> +        if jobs and jobs[0]['offset'] > 0:
> +            break
> +        time.sleep(wait_step)
> +        t += wait_step
>   
> -assert ok
> -
> -vm = iotests.VM().add_drive(disk_a)
> -vm.launch()
> -vm.hmp_qemu_io('drive0', 'write 0 {}'.format(size))
> -
> -vm.qmp_log('blockdev-add', filters=[iotests.filter_qmp_testfiles],
> -           **{'node_name': 'backup0',
> -              'driver': 'raw',
> -              'file': {'driver': 'nbd',
> -                       'server': {'type': 'unix', 'path': nbd_sock},
> -                       'reconnect-delay': 10}})
> -vm.qmp_log('blockdev-backup', device='drive0', sync='full', target='backup0',
> -           speed=(1 * 1024 * 1024))
> -
> -# Wait for some progress
> -t = 0
> -while t < wait_limit:
> -    jobs = vm.qmp('query-block-jobs')['return']
>       if jobs and jobs[0]['offset'] > 0:
> -        break
> -    time.sleep(wait_step)
> -    t += wait_step
> -
> -if jobs and jobs[0]['offset'] > 0:
> -    log('Backup job is started')
> -
> -log('Kill NBD server')
> -srv.kill()
> -srv.wait()
> +        log('Backup job is started')
>   
>   jobs = vm.qmp('query-block-jobs')['return']
>   if jobs and jobs[0]['offset'] < jobs[0]['len']:
> @@ -88,12 +72,8 @@ vm.qmp_log('block-job-set-speed', device='drive0', speed=0)
>   # Emulate server down time for 1 second
>   time.sleep(1)
>   
> -log('Start NBD server')
> -srv = qemu_nbd_popen('-k', nbd_sock, '-f', iotests.imgfmt, disk_b)
> -
> -e = vm.event_wait('BLOCK_JOB_COMPLETED')
> -log('Backup completed: {}'.format(e['data']['offset']))
> -
> -vm.qmp_log('blockdev-del', node_name='backup0')
> -srv.kill()
> -vm.shutdown()
> +with qemu_nbd_popen('-k', nbd_sock, '-f', iotests.imgfmt, disk_b):
> +    e = vm.event_wait('BLOCK_JOB_COMPLETED')
> +    log('Backup completed: {}'.format(e['data']['offset']))
> +    vm.qmp_log('blockdev-del', node_name='backup0')
> +    vm.shutdown()
> diff --git a/tests/qemu-iotests/264.out b/tests/qemu-iotests/264.out
> index 3000944b09..c45b1e81ef 100644
> --- a/tests/qemu-iotests/264.out
> +++ b/tests/qemu-iotests/264.out
> @@ -1,3 +1,4 @@
> +Start NBD server
>   {"execute": "blockdev-add", "arguments": {"driver": "raw", "file": {"driver": "nbd", "reconnect-delay": 10, "server": {"path": "TEST_DIR/PID-nbd-sock", "type": "unix"}}, "node-name": "backup0"}}
>   {"return": {}}
>   {"execute": "blockdev-backup", "arguments": {"device": "drive0", "speed": 1048576, "sync": "full", "target": "backup0"}}
> @@ -11,3 +12,4 @@ Start NBD server
>   Backup completed: 5242880
>   {"execute": "blockdev-del", "arguments": {"node-name": "backup0"}}
>   {"return": {}}
> +Kill NBD server
> diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
> index 3590ed78a0..8f79668435 100644
> --- a/tests/qemu-iotests/iotests.py
> +++ b/tests/qemu-iotests/iotests.py
> @@ -28,10 +28,13 @@ import signal
>   import struct
>   import subprocess
>   import sys
> +import time
>   from typing import (Any, Callable, Dict, Iterable,
>                       List, Optional, Sequence, Tuple, TypeVar)
>   import unittest
>   
> +from contextlib import contextmanager
> +
>   # pylint: disable=import-error, wrong-import-position
>   sys.path.append(os.path.join(os.path.dirname(__file__), '..', '..', 'python'))
>   from qemu import qtest
> @@ -270,9 +273,30 @@ def qemu_nbd_early_pipe(*args):
>   
>       return subp.returncode, output if subp.returncode else ''
>   
> +@contextmanager
>   def qemu_nbd_popen(*args):
> -    '''Run qemu-nbd in daemon mode and return the parent's exit code'''
> -    return subprocess.Popen(qemu_nbd_args + ['--persistent'] + list(args))
> +    '''Context manager running qemu-nbd within the context'''

PEP8 (or some another PEP referenced in PEP8) asks to use """ for doc-strings

> +    pid_file = file_path("pid")
> +
> +    cmd = list(qemu_nbd_args)
> +    cmd.extend(('--persistent', '--pid-file', pid_file))
> +    cmd.extend(args)
> +
> +    log('Start NBD server')
> +    p = subprocess.Popen(cmd)
> +    try:
> +        while not os.path.exists(pid_file):
> +            if p.poll() is not None:
> +                raise RuntimeError(
> +                    "qemu-nbd terminated with exit code {}: {}"
> +                    .format(p.returncode, ' '.join(cmd)))
> +
> +            time.sleep(0.01)
> +        yield
> +    finally:
> +        log('Kill NBD server')
> +        p.kill()
> +        p.wait()

why do we need try-finally? I think, the only possible exception is your "raise RuntimeError", and in this case the process is alredy dead, no need to kill it (and print the log message)

>   
>   def compare_images(img1, img2, fmt1=imgfmt, fmt2=imgfmt):
>       '''Return True if two image files are identical'''
> 

anyway:
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

-- 
Best regards,
Vladimir

Re: [PATCH v2 3/4] iotests: Add more qemu_img helpers

[Vladimir Sementsov-Ogievskiy]

28.07.2020 00:58, Nir Soffer wrote:
> Add 2 helpers for measuring and checking images:
> - qemu_img_measure()
> - qemu_img_check()
> 
> Both use --output-json and parse the returned json to make easy to use
> in other tests. I'm going to use them in a new test, and I hope they
> will be useful in may other tests.
> 
> Signed-off-by: Nir Soffer <nsoffer@redhat.com>
> ---
>   tests/qemu-iotests/iotests.py | 6 ++++++
>   1 file changed, 6 insertions(+)
> 
> diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
> index 8f79668435..717b5b652c 100644
> --- a/tests/qemu-iotests/iotests.py
> +++ b/tests/qemu-iotests/iotests.py
> @@ -141,6 +141,12 @@ def qemu_img_create(*args):
>   
>       return qemu_img(*args)
>   
> +def qemu_img_measure(*args):
> +    return json.loads(qemu_img_pipe("measure", "--output", "json", *args))
> +
> +def qemu_img_check(*args):
> +    return json.loads(qemu_img_pipe("check", "--output", "json", *args))
> +

qemu_img_pipe has type hints, so I assume we should add them here too.

Also, qemu-img don't report errors in json format, so in case of error, this will raise a problem about something that json can't parse. Probably we need better error handling.

Still, for 5.1 it's OK as is I think, so if we are in a hurry:
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

>   def qemu_img_verbose(*args):
>       '''Run qemu-img without suppressing its output and return the exit code'''
>       exitcode = subprocess.call(qemu_img_args + list(args))
> 


-- 
Best regards,
Vladimir

Re: [PATCH v2 1/4] block: nbd: Fix convert qcow2 compressed to nbd

[Eric Blake]

On 7/27/20 4:58 PM, Nir Soffer wrote:
> When converting to qcow2 compressed format, the last step is a special
> zero length compressed write, ending in call to bdrv_co_truncate(). This

in a call

> call always fails for the nbd driver since it does not implement
> bdrv_co_truncate().
> 
> For block devices, which have the same limits, the call succeeds since
> file driver implements bdrv_co_truncate(). If the caller asked to

since the file

> truncate to the same or smaller size with exact=false, the truncate
> succeeds. Implement the same logic for nbd.
> 
> Example failing without this change:
> 
> In one shell starts qemu-nbd:

start

> 
> $ truncate -s 1g test.tar
> $ qemu-nbd --socket=/tmp/nbd.sock --persistent --format=raw --offset 1536 test.tar
> 
> In another shell convert an image to qcow2 compressed via NBD:
> 
> $ echo "disk data" > disk.raw
> $ truncate -s 1g disk.raw
> $ qemu-img convert -f raw -O qcow2 -c disk1.raw nbd+unix:///?socket=/tmp/nbd.sock; echo $?
> 1
> 
> qemu-img failed, but the conversion was successful:
> 
> $ qemu-img info nbd+unix:///?socket=/tmp/nbd.sock
> image: nbd+unix://?socket=/tmp/nbd.sock
> file format: qcow2
> virtual size: 1 GiB (1073741824 bytes)
> ...
> 
> $ qemu-img check nbd+unix:///?socket=/tmp/nbd.sock
> No errors were found on the image.
> 1/16384 = 0.01% allocated, 100.00% fragmented, 100.00% compressed clusters
> Image end offset: 393216
> 
> $ qemu-img compare disk.raw nbd+unix:///?socket=/tmp/nbd.sock
> Images are identical.
> 
> Fixes: https://bugzilla.redhat.com/1860627
> Signed-off-by: Nir Soffer <nsoffer@redhat.com>
> ---
>   block/nbd.c | 30 ++++++++++++++++++++++++++++++
>   1 file changed, 30 insertions(+)

Reviewed-by: Eric Blake <eblake@redhat.com>

> 
> diff --git a/block/nbd.c b/block/nbd.c
> index 65a4f56924..dcb0b03641 100644
> --- a/block/nbd.c
> +++ b/block/nbd.c
> @@ -1966,6 +1966,33 @@ static void nbd_close(BlockDriverState *bs)
>       nbd_clear_bdrvstate(s);
>   }
>   
> +/*
> + * NBD cannot truncate, but if the caller asks to truncate to the same size, or
> + * to a smaller size with exact=false, there is no reason to fail the
> + * operation.
> + *
> + * Preallocation mode is ignored since it does not seems useful to fail when
> + * when never change anything.

s/when when/when we/

I tested with a quick hack to qemu-io-cmds.c:

diff --git i/qemu-io-cmds.c w/qemu-io-cmds.c
index 851f07e8f8b9..baeae86d8c85 100644
--- i/qemu-io-cmds.c
+++ w/qemu-io-cmds.c
@@ -1715,7 +1715,7 @@ static int truncate_f(BlockBackend *blk, int argc, 
char **argv)
       * exact=true.  It is better to err on the "emit more errors" side
       * than to be overly permissive.
       */
-    ret = blk_truncate(blk, offset, true, PREALLOC_MODE_OFF, 0, 
&local_err);
+    ret = blk_truncate(blk, offset, false, PREALLOC_MODE_OFF, 0, 
&local_err);
      if (ret < 0) {
          error_report_err(local_err);
          return ret;

[We should _really_ improve that command to take options, so you can 
control whether to be exact and what prealloc mode on the fly rather 
than hard-coded, but that's a different patch for a later day]

and with that hack in place, I observed:
$ truncate --size=3m file
$ ./qemu-nbd -f raw file
$ ./qemu-io -f raw nbd://localhost:10809
qemu-io> length
3 MiB
qemu-io> truncate 2m
qemu-io> length
3 MiB
qemu-io> truncate 4m
qemu-io: Cannot grow NBD nodes
qemu-io> length
3 MiB

so my initial worry that qemu would see the shrunken size, and therefore 
a later resize back up to the actual NBD limit would have to pay 
attention to preallocation mode for that portion of the file, appears to 
not matter.  But if we can find a case where it does matter, I guess 
it's still appropriate for a followup for -rc3.  Meanwhile, I'm queuing 
this for -rc2.

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

Re: [PATCH v2 1/4] block: nbd: Fix convert qcow2 compressed to nbd

[Eric Blake]

On 7/28/20 8:15 AM, Vladimir Sementsov-Ogievskiy wrote:
> 28.07.2020 00:58, Nir Soffer wrote:
>> When converting to qcow2 compressed format, the last step is a special
>> zero length compressed write, ending in call to bdrv_co_truncate(). This
>> call always fails for the nbd driver since it does not implement
>> bdrv_co_truncate().
>>

>> +static int coroutine_fn nbd_co_truncate(BlockDriverState *bs, int64_t 
>> offset,
>> +                                        bool exact, PreallocMode 
>> prealloc,
>> +                                        BdrvRequestFlags flags, Error 
>> **errp)
>> +{
>> +    BDRVNBDState *s = bs->opaque;
>> +
>> +    if (offset != s->info.size && exact) {
>> +        error_setg(errp, "Cannot resize NBD nodes");
>> +        return -ENOTSUP;
>> +    }
>> +
>> +    if (offset > s->info.size) {
>> +        error_setg(errp, "Cannot grow NBD nodes");
>> +        return -EINVAL;
>> +    }
> 
> I think that ENOTSUP actually is valid error code for both cases.. NBD 
> protocol has experimental extension NBD_CMD_RESIZE, so one day we'll 
> implement this. So, I think, it's not invalid, but just not supported 
> yet. Still, not a big deal, so with ENOTSUP or EINVAL:
> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

Indeed, if we ever get around to fully specifying how NBD_CMD_RESIZE 
should even work, we'll be revisiting this code to implement that.

> 
> Also, may be better to manage it in generic layer:
> If driver doesn't implement bdrv_co_truncate handler (or return 
> ENOTSUP), and we are truncating to the same size, or to smaller size 
> with exact=false, we just report success? Then we'll drop same code from 
> file-posix.c

That was also my question on v1; but given the closeness to the release, 
this is a minimal change appropriate for -rc2, while changing the 
generic layer may have unintended consequences.

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

Re: [PATCH v2 2/4] iotests: Make qemu_nbd_popen() a contextmanager

[Eric Blake]

On 7/28/20 8:42 AM, Vladimir Sementsov-Ogievskiy wrote:
> 28.07.2020 00:58, Nir Soffer wrote:
>> Instead of duplicating the code to wait until the server is ready and
>> remember to terminate the server and wait for it, make it possible to
>> use like this:
>>
>>      with qemu_nbd_popen('-k', sock, image):
>>          # Access image via qemu-nbd socket...
>>
>> Only test 264 used this helper, but I had to modify the output since it
>> did not consistently when starting and stopping qemu-nbd.
>>
>> Signed-off-by: Nir Soffer <nsoffer@redhat.com>
>> ---

>> +++ b/tests/qemu-iotests/iotests.py
>> @@ -28,10 +28,13 @@ import signal
>>   import struct
>>   import subprocess
>>   import sys
>> +import time
>>   from typing import (Any, Callable, Dict, Iterable,
>>                       List, Optional, Sequence, Tuple, TypeVar)
>>   import unittest
>> +from contextlib import contextmanager
>> +
>>   # pylint: disable=import-error, wrong-import-position
>>   sys.path.append(os.path.join(os.path.dirname(__file__), '..', '..', 
>> 'python'))
>>   from qemu import qtest
>> @@ -270,9 +273,30 @@ def qemu_nbd_early_pipe(*args):
>>       return subp.returncode, output if subp.returncode else ''
>> +@contextmanager
>>   def qemu_nbd_popen(*args):
>> -    '''Run qemu-nbd in daemon mode and return the parent's exit code'''
>> -    return subprocess.Popen(qemu_nbd_args + ['--persistent'] + 
>> list(args))
>> +    '''Context manager running qemu-nbd within the context'''
> 
> PEP8 (or some another PEP referenced in PEP8) asks to use """ for 
> doc-strings

But at least './check 297' still passes, so we are consistent enough 
with our current set of syntax checks to be acceptable.

> 
>> +    pid_file = file_path("pid")
>> +
>> +    cmd = list(qemu_nbd_args)
>> +    cmd.extend(('--persistent', '--pid-file', pid_file))
>> +    cmd.extend(args)
>> +
>> +    log('Start NBD server')
>> +    p = subprocess.Popen(cmd)
>> +    try:
>> +        while not os.path.exists(pid_file):
>> +            if p.poll() is not None:
>> +                raise RuntimeError(
>> +                    "qemu-nbd terminated with exit code {}: {}"
>> +                    .format(p.returncode, ' '.join(cmd)))
>> +
>> +            time.sleep(0.01)
>> +        yield
>> +    finally:
>> +        log('Kill NBD server')
>> +        p.kill()
>> +        p.wait()
> 
> why do we need try-finally? I think, the only possible exception is your 
> "raise RuntimeError", and in this case the process is alredy dead, no 
> need to kill it (and print the log message)
> 
>>   def compare_images(img1, img2, fmt1=imgfmt, fmt2=imgfmt):
>>       '''Return True if two image files are identical'''
>>
> 
> anyway:
> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> 

Thanks; I trust your python review more than mine.  Queuing for -rc2.

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

Re: [PATCH v2 4/4] iotests: Test convert to qcow2 compressed to NBD

[Eric Blake]

On 7/27/20 4:58 PM, Nir Soffer wrote:
> Add test for "qemu-img convert -O qcow2 -c" to NBD target. The tests
> create a OVA file and write compressed qcow2 disk content directly into
> the OVA file via qemu-nbd.
> 
> Signed-off-by: Nir Soffer <nsoffer@redhat.com>
> ---
>   tests/qemu-iotests/302     | 127 +++++++++++++++++++++++++++++++++++++
>   tests/qemu-iotests/302.out |  31 +++++++++
>   tests/qemu-iotests/group   |   1 +
>   3 files changed, 159 insertions(+)
>   create mode 100755 tests/qemu-iotests/302
>   create mode 100644 tests/qemu-iotests/302.out
> 

Tested-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>

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

Re: [PATCH v2 2/4] iotests: Make qemu_nbd_popen() a contextmanager

[Nir Soffer]

On Tue, Jul 28, 2020 at 4:43 PM Vladimir Sementsov-Ogievskiy
<vsementsov@virtuozzo.com> wrote:
>
> 28.07.2020 00:58, Nir Soffer wrote:
> > Instead of duplicating the code to wait until the server is ready and
> > remember to terminate the server and wait for it, make it possible to
> > use like this:
> >
> >      with qemu_nbd_popen('-k', sock, image):
> >          # Access image via qemu-nbd socket...
> >
> > Only test 264 used this helper, but I had to modify the output since it
> > did not consistently when starting and stopping qemu-nbd.
> >
> > Signed-off-by: Nir Soffer <nsoffer@redhat.com>
> > ---
> >   tests/qemu-iotests/264        | 76 +++++++++++++----------------------
> >   tests/qemu-iotests/264.out    |  2 +
> >   tests/qemu-iotests/iotests.py | 28 ++++++++++++-
> >   3 files changed, 56 insertions(+), 50 deletions(-)
> >
> > diff --git a/tests/qemu-iotests/264 b/tests/qemu-iotests/264
> > index 304a7443d7..666f164ed8 100755
> > --- a/tests/qemu-iotests/264
> > +++ b/tests/qemu-iotests/264
> > @@ -36,48 +36,32 @@ wait_step = 0.2
> >
> >   qemu_img_create('-f', iotests.imgfmt, disk_a, str(size))
> >   qemu_img_create('-f', iotests.imgfmt, disk_b, str(size))
> > -srv = qemu_nbd_popen('-k', nbd_sock, '-f', iotests.imgfmt, disk_b)
> >
> > -# Wait for NBD server availability
> > -t = 0
> > -ok = False
> > -while t < wait_limit:
> > -    ok = qemu_io_silent_check('-f', 'raw', '-c', 'read 0 512', nbd_uri)
> > -    if ok:
> > -        break
> > -    time.sleep(wait_step)
> > -    t += wait_step
> > +with qemu_nbd_popen('-k', nbd_sock, '-f', iotests.imgfmt, disk_b):
> > +    vm = iotests.VM().add_drive(disk_a)
> > +    vm.launch()
> > +    vm.hmp_qemu_io('drive0', 'write 0 {}'.format(size))
> > +
> > +    vm.qmp_log('blockdev-add', filters=[iotests.filter_qmp_testfiles],
> > +               **{'node_name': 'backup0',
> > +                  'driver': 'raw',
> > +                  'file': {'driver': 'nbd',
> > +                           'server': {'type': 'unix', 'path': nbd_sock},
> > +                           'reconnect-delay': 10}})
> > +    vm.qmp_log('blockdev-backup', device='drive0', sync='full', target='backup0',
> > +               speed=(1 * 1024 * 1024))
> > +
> > +    # Wait for some progress
> > +    t = 0
> > +    while t < wait_limit:
> > +        jobs = vm.qmp('query-block-jobs')['return']
> > +        if jobs and jobs[0]['offset'] > 0:
> > +            break
> > +        time.sleep(wait_step)
> > +        t += wait_step
> >
> > -assert ok
> > -
> > -vm = iotests.VM().add_drive(disk_a)
> > -vm.launch()
> > -vm.hmp_qemu_io('drive0', 'write 0 {}'.format(size))
> > -
> > -vm.qmp_log('blockdev-add', filters=[iotests.filter_qmp_testfiles],
> > -           **{'node_name': 'backup0',
> > -              'driver': 'raw',
> > -              'file': {'driver': 'nbd',
> > -                       'server': {'type': 'unix', 'path': nbd_sock},
> > -                       'reconnect-delay': 10}})
> > -vm.qmp_log('blockdev-backup', device='drive0', sync='full', target='backup0',
> > -           speed=(1 * 1024 * 1024))
> > -
> > -# Wait for some progress
> > -t = 0
> > -while t < wait_limit:
> > -    jobs = vm.qmp('query-block-jobs')['return']
> >       if jobs and jobs[0]['offset'] > 0:
> > -        break
> > -    time.sleep(wait_step)
> > -    t += wait_step
> > -
> > -if jobs and jobs[0]['offset'] > 0:
> > -    log('Backup job is started')
> > -
> > -log('Kill NBD server')
> > -srv.kill()
> > -srv.wait()
> > +        log('Backup job is started')
> >
> >   jobs = vm.qmp('query-block-jobs')['return']
> >   if jobs and jobs[0]['offset'] < jobs[0]['len']:
> > @@ -88,12 +72,8 @@ vm.qmp_log('block-job-set-speed', device='drive0', speed=0)
> >   # Emulate server down time for 1 second
> >   time.sleep(1)
> >
> > -log('Start NBD server')
> > -srv = qemu_nbd_popen('-k', nbd_sock, '-f', iotests.imgfmt, disk_b)
> > -
> > -e = vm.event_wait('BLOCK_JOB_COMPLETED')
> > -log('Backup completed: {}'.format(e['data']['offset']))
> > -
> > -vm.qmp_log('blockdev-del', node_name='backup0')
> > -srv.kill()
> > -vm.shutdown()
> > +with qemu_nbd_popen('-k', nbd_sock, '-f', iotests.imgfmt, disk_b):
> > +    e = vm.event_wait('BLOCK_JOB_COMPLETED')
> > +    log('Backup completed: {}'.format(e['data']['offset']))
> > +    vm.qmp_log('blockdev-del', node_name='backup0')
> > +    vm.shutdown()
> > diff --git a/tests/qemu-iotests/264.out b/tests/qemu-iotests/264.out
> > index 3000944b09..c45b1e81ef 100644
> > --- a/tests/qemu-iotests/264.out
> > +++ b/tests/qemu-iotests/264.out
> > @@ -1,3 +1,4 @@
> > +Start NBD server
> >   {"execute": "blockdev-add", "arguments": {"driver": "raw", "file": {"driver": "nbd", "reconnect-delay": 10, "server": {"path": "TEST_DIR/PID-nbd-sock", "type": "unix"}}, "node-name": "backup0"}}
> >   {"return": {}}
> >   {"execute": "blockdev-backup", "arguments": {"device": "drive0", "speed": 1048576, "sync": "full", "target": "backup0"}}
> > @@ -11,3 +12,4 @@ Start NBD server
> >   Backup completed: 5242880
> >   {"execute": "blockdev-del", "arguments": {"node-name": "backup0"}}
> >   {"return": {}}
> > +Kill NBD server
> > diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
> > index 3590ed78a0..8f79668435 100644
> > --- a/tests/qemu-iotests/iotests.py
> > +++ b/tests/qemu-iotests/iotests.py
> > @@ -28,10 +28,13 @@ import signal
> >   import struct
> >   import subprocess
> >   import sys
> > +import time
> >   from typing import (Any, Callable, Dict, Iterable,
> >                       List, Optional, Sequence, Tuple, TypeVar)
> >   import unittest
> >
> > +from contextlib import contextmanager
> > +
> >   # pylint: disable=import-error, wrong-import-position
> >   sys.path.append(os.path.join(os.path.dirname(__file__), '..', '..', 'python'))
> >   from qemu import qtest
> > @@ -270,9 +273,30 @@ def qemu_nbd_early_pipe(*args):
> >
> >       return subp.returncode, output if subp.returncode else ''
> >
> > +@contextmanager
> >   def qemu_nbd_popen(*args):
> > -    '''Run qemu-nbd in daemon mode and return the parent's exit code'''
> > -    return subprocess.Popen(qemu_nbd_args + ['--persistent'] + list(args))
> > +    '''Context manager running qemu-nbd within the context'''
>
> PEP8 (or some another PEP referenced in PEP8) asks to use """ for doc-strings

Both are valid, but I agree that """ is nicer.

This module needs more work:

$ flake8 --statistics --quiet tests/qemu-iotests/iotests.py
tests/qemu-iotests/iotests.py
1     E261 at least two spaces before inline comment
3     E301 expected 1 blank line, found 0
64    E302 expected 2 blank lines, found 1
1     E303 too many blank lines (2)
5     E305 expected 2 blank lines after class or function definition, found 1
2     E402 module level import not at top of file

> > +    pid_file = file_path("pid")
> > +
> > +    cmd = list(qemu_nbd_args)
> > +    cmd.extend(('--persistent', '--pid-file', pid_file))
> > +    cmd.extend(args)
> > +
> > +    log('Start NBD server')
> > +    p = subprocess.Popen(cmd)
> > +    try:
> > +        while not os.path.exists(pid_file):
> > +            if p.poll() is not None:
> > +                raise RuntimeError(
> > +                    "qemu-nbd terminated with exit code {}: {}"
> > +                    .format(p.returncode, ' '.join(cmd)))
> > +
> > +            time.sleep(0.01)
> > +        yield
> > +    finally:
> > +        log('Kill NBD server')
> > +        p.kill()
> > +        p.wait()
>
> why do we need try-finally? I think, the only possible exception is your "raise RuntimeError", and in this case the process is alredy dead, no need to kill it (and print the log message)

The try-finally is needed for errors in user code like:

    with qemu_nbd_popen():
         assert 0

Without try-finally qemu-nbd will continue to run after the assert
fails, which may
cause trouble, depending on the test.

In the case of the RuntimeError inside the try, the cleanup in finally is
not needed but harmless.

Looking in python source:

        def send_signal(self, sig):
            """Send a signal to the process."""
            # Skip signalling a process that we know has already died.
            if self.returncode is None:
                os.kill(self.pid, sig)

        def kill(self):
            """Kill the process with SIGKILL
            """
            self.send_signal(signal.SIGKILL)

So the kill() will do nothing, and wait() will return immediately.

> >   def compare_images(img1, img2, fmt1=imgfmt, fmt2=imgfmt):
> >       '''Return True if two image files are identical'''
> >
>
> anyway:
> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>
> --
> Best regards,
> Vladimir
>

Re: [PATCH v2 3/4] iotests: Add more qemu_img helpers

[Nir Soffer]

On Tue, Jul 28, 2020 at 4:50 PM Vladimir Sementsov-Ogievskiy
<vsementsov@virtuozzo.com> wrote:
>
> 28.07.2020 00:58, Nir Soffer wrote:
> > Add 2 helpers for measuring and checking images:
> > - qemu_img_measure()
> > - qemu_img_check()
> >
> > Both use --output-json and parse the returned json to make easy to use
> > in other tests. I'm going to use them in a new test, and I hope they
> > will be useful in may other tests.
> >
> > Signed-off-by: Nir Soffer <nsoffer@redhat.com>
> > ---
> >   tests/qemu-iotests/iotests.py | 6 ++++++
> >   1 file changed, 6 insertions(+)
> >
> > diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
> > index 8f79668435..717b5b652c 100644
> > --- a/tests/qemu-iotests/iotests.py
> > +++ b/tests/qemu-iotests/iotests.py
> > @@ -141,6 +141,12 @@ def qemu_img_create(*args):
> >
> >       return qemu_img(*args)
> >
> > +def qemu_img_measure(*args):
> > +    return json.loads(qemu_img_pipe("measure", "--output", "json", *args))
> > +
> > +def qemu_img_check(*args):
> > +    return json.loads(qemu_img_pipe("check", "--output", "json", *args))
> > +
>
> qemu_img_pipe has type hints, so I assume we should add them here too.

True, but type hints are not use consistently in this module (e.g.
qemu_img_verbose).

>
> Also, qemu-img don't report errors in json format, so in case of error, this will raise a problem about something that json can't parse. Probably we need better error handling.

Yes, this fails in an ugly and unhelpful way now.

Ideally failing command will raise a detailed error with the command,
exit code, output,
and error. Code that want to check for specific return code would do:

    try:
        iotests.qemu_img_check(disk)
    except iotest.Error as e:
        if e.rc == 2:
           ...

But most callers do not need this so they will fail loudly with all the details.

What do you think?

> Still, for 5.1 it's OK as is I think, so if we are in a hurry:
> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>
> >   def qemu_img_verbose(*args):
> >       '''Run qemu-img without suppressing its output and return the exit code'''
> >       exitcode = subprocess.call(qemu_img_args + list(args))
> >
>
>
> --
> Best regards,
> Vladimir
>

Re: [PATCH v2 2/4] iotests: Make qemu_nbd_popen() a contextmanager

[Vladimir Sementsov-Ogievskiy]

28.07.2020 19:05, Nir Soffer wrote:
> On Tue, Jul 28, 2020 at 4:43 PM Vladimir Sementsov-Ogievskiy
> <vsementsov@virtuozzo.com> wrote:
>>
>> 28.07.2020 00:58, Nir Soffer wrote:
>>> Instead of duplicating the code to wait until the server is ready and
>>> remember to terminate the server and wait for it, make it possible to
>>> use like this:
>>>
>>>       with qemu_nbd_popen('-k', sock, image):
>>>           # Access image via qemu-nbd socket...
>>>
>>> Only test 264 used this helper, but I had to modify the output since it
>>> did not consistently when starting and stopping qemu-nbd.
>>>
>>> Signed-off-by: Nir Soffer <nsoffer@redhat.com>
>>> ---
>>>    tests/qemu-iotests/264        | 76 +++++++++++++----------------------
>>>    tests/qemu-iotests/264.out    |  2 +
>>>    tests/qemu-iotests/iotests.py | 28 ++++++++++++-
>>>    3 files changed, 56 insertions(+), 50 deletions(-)
>>>
>>> diff --git a/tests/qemu-iotests/264 b/tests/qemu-iotests/264
>>> index 304a7443d7..666f164ed8 100755
>>> --- a/tests/qemu-iotests/264
>>> +++ b/tests/qemu-iotests/264
>>> @@ -36,48 +36,32 @@ wait_step = 0.2
>>>
>>>    qemu_img_create('-f', iotests.imgfmt, disk_a, str(size))
>>>    qemu_img_create('-f', iotests.imgfmt, disk_b, str(size))
>>> -srv = qemu_nbd_popen('-k', nbd_sock, '-f', iotests.imgfmt, disk_b)
>>>
>>> -# Wait for NBD server availability
>>> -t = 0
>>> -ok = False
>>> -while t < wait_limit:
>>> -    ok = qemu_io_silent_check('-f', 'raw', '-c', 'read 0 512', nbd_uri)
>>> -    if ok:
>>> -        break
>>> -    time.sleep(wait_step)
>>> -    t += wait_step
>>> +with qemu_nbd_popen('-k', nbd_sock, '-f', iotests.imgfmt, disk_b):
>>> +    vm = iotests.VM().add_drive(disk_a)
>>> +    vm.launch()
>>> +    vm.hmp_qemu_io('drive0', 'write 0 {}'.format(size))
>>> +
>>> +    vm.qmp_log('blockdev-add', filters=[iotests.filter_qmp_testfiles],
>>> +               **{'node_name': 'backup0',
>>> +                  'driver': 'raw',
>>> +                  'file': {'driver': 'nbd',
>>> +                           'server': {'type': 'unix', 'path': nbd_sock},
>>> +                           'reconnect-delay': 10}})
>>> +    vm.qmp_log('blockdev-backup', device='drive0', sync='full', target='backup0',
>>> +               speed=(1 * 1024 * 1024))
>>> +
>>> +    # Wait for some progress
>>> +    t = 0
>>> +    while t < wait_limit:
>>> +        jobs = vm.qmp('query-block-jobs')['return']
>>> +        if jobs and jobs[0]['offset'] > 0:
>>> +            break
>>> +        time.sleep(wait_step)
>>> +        t += wait_step
>>>
>>> -assert ok
>>> -
>>> -vm = iotests.VM().add_drive(disk_a)
>>> -vm.launch()
>>> -vm.hmp_qemu_io('drive0', 'write 0 {}'.format(size))
>>> -
>>> -vm.qmp_log('blockdev-add', filters=[iotests.filter_qmp_testfiles],
>>> -           **{'node_name': 'backup0',
>>> -              'driver': 'raw',
>>> -              'file': {'driver': 'nbd',
>>> -                       'server': {'type': 'unix', 'path': nbd_sock},
>>> -                       'reconnect-delay': 10}})
>>> -vm.qmp_log('blockdev-backup', device='drive0', sync='full', target='backup0',
>>> -           speed=(1 * 1024 * 1024))
>>> -
>>> -# Wait for some progress
>>> -t = 0
>>> -while t < wait_limit:
>>> -    jobs = vm.qmp('query-block-jobs')['return']
>>>        if jobs and jobs[0]['offset'] > 0:
>>> -        break
>>> -    time.sleep(wait_step)
>>> -    t += wait_step
>>> -
>>> -if jobs and jobs[0]['offset'] > 0:
>>> -    log('Backup job is started')
>>> -
>>> -log('Kill NBD server')
>>> -srv.kill()
>>> -srv.wait()
>>> +        log('Backup job is started')
>>>
>>>    jobs = vm.qmp('query-block-jobs')['return']
>>>    if jobs and jobs[0]['offset'] < jobs[0]['len']:
>>> @@ -88,12 +72,8 @@ vm.qmp_log('block-job-set-speed', device='drive0', speed=0)
>>>    # Emulate server down time for 1 second
>>>    time.sleep(1)
>>>
>>> -log('Start NBD server')
>>> -srv = qemu_nbd_popen('-k', nbd_sock, '-f', iotests.imgfmt, disk_b)
>>> -
>>> -e = vm.event_wait('BLOCK_JOB_COMPLETED')
>>> -log('Backup completed: {}'.format(e['data']['offset']))
>>> -
>>> -vm.qmp_log('blockdev-del', node_name='backup0')
>>> -srv.kill()
>>> -vm.shutdown()
>>> +with qemu_nbd_popen('-k', nbd_sock, '-f', iotests.imgfmt, disk_b):
>>> +    e = vm.event_wait('BLOCK_JOB_COMPLETED')
>>> +    log('Backup completed: {}'.format(e['data']['offset']))
>>> +    vm.qmp_log('blockdev-del', node_name='backup0')
>>> +    vm.shutdown()
>>> diff --git a/tests/qemu-iotests/264.out b/tests/qemu-iotests/264.out
>>> index 3000944b09..c45b1e81ef 100644
>>> --- a/tests/qemu-iotests/264.out
>>> +++ b/tests/qemu-iotests/264.out
>>> @@ -1,3 +1,4 @@
>>> +Start NBD server
>>>    {"execute": "blockdev-add", "arguments": {"driver": "raw", "file": {"driver": "nbd", "reconnect-delay": 10, "server": {"path": "TEST_DIR/PID-nbd-sock", "type": "unix"}}, "node-name": "backup0"}}
>>>    {"return": {}}
>>>    {"execute": "blockdev-backup", "arguments": {"device": "drive0", "speed": 1048576, "sync": "full", "target": "backup0"}}
>>> @@ -11,3 +12,4 @@ Start NBD server
>>>    Backup completed: 5242880
>>>    {"execute": "blockdev-del", "arguments": {"node-name": "backup0"}}
>>>    {"return": {}}
>>> +Kill NBD server
>>> diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
>>> index 3590ed78a0..8f79668435 100644
>>> --- a/tests/qemu-iotests/iotests.py
>>> +++ b/tests/qemu-iotests/iotests.py
>>> @@ -28,10 +28,13 @@ import signal
>>>    import struct
>>>    import subprocess
>>>    import sys
>>> +import time
>>>    from typing import (Any, Callable, Dict, Iterable,
>>>                        List, Optional, Sequence, Tuple, TypeVar)
>>>    import unittest
>>>
>>> +from contextlib import contextmanager
>>> +
>>>    # pylint: disable=import-error, wrong-import-position
>>>    sys.path.append(os.path.join(os.path.dirname(__file__), '..', '..', 'python'))
>>>    from qemu import qtest
>>> @@ -270,9 +273,30 @@ def qemu_nbd_early_pipe(*args):
>>>
>>>        return subp.returncode, output if subp.returncode else ''
>>>
>>> +@contextmanager
>>>    def qemu_nbd_popen(*args):
>>> -    '''Run qemu-nbd in daemon mode and return the parent's exit code'''
>>> -    return subprocess.Popen(qemu_nbd_args + ['--persistent'] + list(args))
>>> +    '''Context manager running qemu-nbd within the context'''
>>
>> PEP8 (or some another PEP referenced in PEP8) asks to use """ for doc-strings
> 
> Both are valid, but I agree that """ is nicer.

PEP 8:

     For triple-quoted strings, always use double quote characters to be consistent with the docstring convention in PEP 257

PEP 257:

     For consistency, always use """triple double quotes""" around docstrings

> 
> This module needs more work:
> 
> $ flake8 --statistics --quiet tests/qemu-iotests/iotests.py
> tests/qemu-iotests/iotests.py
> 1     E261 at least two spaces before inline comment
> 3     E301 expected 1 blank line, found 0
> 64    E302 expected 2 blank lines, found 1
> 1     E303 too many blank lines (2)
> 5     E305 expected 2 blank lines after class or function definition, found 1
> 2     E402 module level import not at top of file
> 
>>> +    pid_file = file_path("pid")
>>> +
>>> +    cmd = list(qemu_nbd_args)
>>> +    cmd.extend(('--persistent', '--pid-file', pid_file))
>>> +    cmd.extend(args)
>>> +
>>> +    log('Start NBD server')
>>> +    p = subprocess.Popen(cmd)
>>> +    try:
>>> +        while not os.path.exists(pid_file):
>>> +            if p.poll() is not None:
>>> +                raise RuntimeError(
>>> +                    "qemu-nbd terminated with exit code {}: {}"
>>> +                    .format(p.returncode, ' '.join(cmd)))
>>> +
>>> +            time.sleep(0.01)
>>> +        yield
>>> +    finally:
>>> +        log('Kill NBD server')
>>> +        p.kill()
>>> +        p.wait()
>>
>> why do we need try-finally? I think, the only possible exception is your "raise RuntimeError", and in this case the process is alredy dead, no need to kill it (and print the log message)
> 
> The try-finally is needed for errors in user code like:
> 
>      with qemu_nbd_popen():
>           assert 0
> 

Ahh, so this "finally" will work, on some exception during yield? OK then.

> Without try-finally qemu-nbd will continue to run after the assert
> fails, which may
> cause trouble, depending on the test.
> 
> In the case of the RuntimeError inside the try, the cleanup in finally is
> not needed but harmless.
> 
> Looking in python source:
> 
>          def send_signal(self, sig):
>              """Send a signal to the process."""
>              # Skip signalling a process that we know has already died.
>              if self.returncode is None:
>                  os.kill(self.pid, sig)
> 
>          def kill(self):
>              """Kill the process with SIGKILL
>              """
>              self.send_signal(signal.SIGKILL)
> 
> So the kill() will do nothing, and wait() will return immediately.
> 
>>>    def compare_images(img1, img2, fmt1=imgfmt, fmt2=imgfmt):
>>>        '''Return True if two image files are identical'''
>>>
>>
>> anyway:
>> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>
>> --
>> Best regards,
>> Vladimir
>>
> 


-- 
Best regards,
Vladimir

Re: [PATCH v2 2/4] iotests: Make qemu_nbd_popen() a contextmanager

[Nir Soffer]

On Wed, Aug 5, 2020 at 10:38 AM Vladimir Sementsov-Ogievskiy
<vsementsov@virtuozzo.com> wrote:
>
> 28.07.2020 19:05, Nir Soffer wrote:
> > On Tue, Jul 28, 2020 at 4:43 PM Vladimir Sementsov-Ogievskiy
> > <vsementsov@virtuozzo.com> wrote:
> >>
> >> 28.07.2020 00:58, Nir Soffer wrote:
> >>> Instead of duplicating the code to wait until the server is ready and
> >>> remember to terminate the server and wait for it, make it possible to
> >>> use like this:
> >>>
> >>>       with qemu_nbd_popen('-k', sock, image):
> >>>           # Access image via qemu-nbd socket...
> >>>
> >>> Only test 264 used this helper, but I had to modify the output since it
> >>> did not consistently when starting and stopping qemu-nbd.
> >>>
> >>> Signed-off-by: Nir Soffer <nsoffer@redhat.com>
> >>> ---
> >>>    tests/qemu-iotests/264        | 76 +++++++++++++----------------------
> >>>    tests/qemu-iotests/264.out    |  2 +
> >>>    tests/qemu-iotests/iotests.py | 28 ++++++++++++-
> >>>    3 files changed, 56 insertions(+), 50 deletions(-)
> >>>
> >>> diff --git a/tests/qemu-iotests/264 b/tests/qemu-iotests/264
> >>> index 304a7443d7..666f164ed8 100755
> >>> --- a/tests/qemu-iotests/264
> >>> +++ b/tests/qemu-iotests/264
> >>> @@ -36,48 +36,32 @@ wait_step = 0.2
> >>>
> >>>    qemu_img_create('-f', iotests.imgfmt, disk_a, str(size))
> >>>    qemu_img_create('-f', iotests.imgfmt, disk_b, str(size))
> >>> -srv = qemu_nbd_popen('-k', nbd_sock, '-f', iotests.imgfmt, disk_b)
> >>>
> >>> -# Wait for NBD server availability
> >>> -t = 0
> >>> -ok = False
> >>> -while t < wait_limit:
> >>> -    ok = qemu_io_silent_check('-f', 'raw', '-c', 'read 0 512', nbd_uri)
> >>> -    if ok:
> >>> -        break
> >>> -    time.sleep(wait_step)
> >>> -    t += wait_step
> >>> +with qemu_nbd_popen('-k', nbd_sock, '-f', iotests.imgfmt, disk_b):
> >>> +    vm = iotests.VM().add_drive(disk_a)
> >>> +    vm.launch()
> >>> +    vm.hmp_qemu_io('drive0', 'write 0 {}'.format(size))
> >>> +
> >>> +    vm.qmp_log('blockdev-add', filters=[iotests.filter_qmp_testfiles],
> >>> +               **{'node_name': 'backup0',
> >>> +                  'driver': 'raw',
> >>> +                  'file': {'driver': 'nbd',
> >>> +                           'server': {'type': 'unix', 'path': nbd_sock},
> >>> +                           'reconnect-delay': 10}})
> >>> +    vm.qmp_log('blockdev-backup', device='drive0', sync='full', target='backup0',
> >>> +               speed=(1 * 1024 * 1024))
> >>> +
> >>> +    # Wait for some progress
> >>> +    t = 0
> >>> +    while t < wait_limit:
> >>> +        jobs = vm.qmp('query-block-jobs')['return']
> >>> +        if jobs and jobs[0]['offset'] > 0:
> >>> +            break
> >>> +        time.sleep(wait_step)
> >>> +        t += wait_step
> >>>
> >>> -assert ok
> >>> -
> >>> -vm = iotests.VM().add_drive(disk_a)
> >>> -vm.launch()
> >>> -vm.hmp_qemu_io('drive0', 'write 0 {}'.format(size))
> >>> -
> >>> -vm.qmp_log('blockdev-add', filters=[iotests.filter_qmp_testfiles],
> >>> -           **{'node_name': 'backup0',
> >>> -              'driver': 'raw',
> >>> -              'file': {'driver': 'nbd',
> >>> -                       'server': {'type': 'unix', 'path': nbd_sock},
> >>> -                       'reconnect-delay': 10}})
> >>> -vm.qmp_log('blockdev-backup', device='drive0', sync='full', target='backup0',
> >>> -           speed=(1 * 1024 * 1024))
> >>> -
> >>> -# Wait for some progress
> >>> -t = 0
> >>> -while t < wait_limit:
> >>> -    jobs = vm.qmp('query-block-jobs')['return']
> >>>        if jobs and jobs[0]['offset'] > 0:
> >>> -        break
> >>> -    time.sleep(wait_step)
> >>> -    t += wait_step
> >>> -
> >>> -if jobs and jobs[0]['offset'] > 0:
> >>> -    log('Backup job is started')
> >>> -
> >>> -log('Kill NBD server')
> >>> -srv.kill()
> >>> -srv.wait()
> >>> +        log('Backup job is started')
> >>>
> >>>    jobs = vm.qmp('query-block-jobs')['return']
> >>>    if jobs and jobs[0]['offset'] < jobs[0]['len']:
> >>> @@ -88,12 +72,8 @@ vm.qmp_log('block-job-set-speed', device='drive0', speed=0)
> >>>    # Emulate server down time for 1 second
> >>>    time.sleep(1)
> >>>
> >>> -log('Start NBD server')
> >>> -srv = qemu_nbd_popen('-k', nbd_sock, '-f', iotests.imgfmt, disk_b)
> >>> -
> >>> -e = vm.event_wait('BLOCK_JOB_COMPLETED')
> >>> -log('Backup completed: {}'.format(e['data']['offset']))
> >>> -
> >>> -vm.qmp_log('blockdev-del', node_name='backup0')
> >>> -srv.kill()
> >>> -vm.shutdown()
> >>> +with qemu_nbd_popen('-k', nbd_sock, '-f', iotests.imgfmt, disk_b):
> >>> +    e = vm.event_wait('BLOCK_JOB_COMPLETED')
> >>> +    log('Backup completed: {}'.format(e['data']['offset']))
> >>> +    vm.qmp_log('blockdev-del', node_name='backup0')
> >>> +    vm.shutdown()
> >>> diff --git a/tests/qemu-iotests/264.out b/tests/qemu-iotests/264.out
> >>> index 3000944b09..c45b1e81ef 100644
> >>> --- a/tests/qemu-iotests/264.out
> >>> +++ b/tests/qemu-iotests/264.out
> >>> @@ -1,3 +1,4 @@
> >>> +Start NBD server
> >>>    {"execute": "blockdev-add", "arguments": {"driver": "raw", "file": {"driver": "nbd", "reconnect-delay": 10, "server": {"path": "TEST_DIR/PID-nbd-sock", "type": "unix"}}, "node-name": "backup0"}}
> >>>    {"return": {}}
> >>>    {"execute": "blockdev-backup", "arguments": {"device": "drive0", "speed": 1048576, "sync": "full", "target": "backup0"}}
> >>> @@ -11,3 +12,4 @@ Start NBD server
> >>>    Backup completed: 5242880
> >>>    {"execute": "blockdev-del", "arguments": {"node-name": "backup0"}}
> >>>    {"return": {}}
> >>> +Kill NBD server
> >>> diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
> >>> index 3590ed78a0..8f79668435 100644
> >>> --- a/tests/qemu-iotests/iotests.py
> >>> +++ b/tests/qemu-iotests/iotests.py
> >>> @@ -28,10 +28,13 @@ import signal
> >>>    import struct
> >>>    import subprocess
> >>>    import sys
> >>> +import time
> >>>    from typing import (Any, Callable, Dict, Iterable,
> >>>                        List, Optional, Sequence, Tuple, TypeVar)
> >>>    import unittest
> >>>
> >>> +from contextlib import contextmanager
> >>> +
> >>>    # pylint: disable=import-error, wrong-import-position
> >>>    sys.path.append(os.path.join(os.path.dirname(__file__), '..', '..', 'python'))
> >>>    from qemu import qtest
> >>> @@ -270,9 +273,30 @@ def qemu_nbd_early_pipe(*args):
> >>>
> >>>        return subp.returncode, output if subp.returncode else ''
> >>>
> >>> +@contextmanager
> >>>    def qemu_nbd_popen(*args):
> >>> -    '''Run qemu-nbd in daemon mode and return the parent's exit code'''
> >>> -    return subprocess.Popen(qemu_nbd_args + ['--persistent'] + list(args))
> >>> +    '''Context manager running qemu-nbd within the context'''
> >>
> >> PEP8 (or some another PEP referenced in PEP8) asks to use """ for doc-strings
> >
> > Both are valid, but I agree that """ is nicer.
>
> PEP 8:
>
>      For triple-quoted strings, always use double quote characters to be consistent with the docstring convention in PEP 257
>
> PEP 257:
>
>      For consistency, always use """triple double quotes""" around docstrings

These are good recommendations, but they are not enforced by the tools
like flake8.

Python treats ''' and """ as the same thing:

>>> def foo():
...     '''
...     This is my docstring.
...     '''
...
>>> foo.__doc__
'\n\tThis is my docstring.\n\t'

We should use the recommended """, but this kind of change should be done for
the entire module.

I can send a patch for this later.

> > This module needs more work:
> >
> > $ flake8 --statistics --quiet tests/qemu-iotests/iotests.py
> > tests/qemu-iotests/iotests.py
> > 1     E261 at least two spaces before inline comment
> > 3     E301 expected 1 blank line, found 0
> > 64    E302 expected 2 blank lines, found 1
> > 1     E303 too many blank lines (2)
> > 5     E305 expected 2 blank lines after class or function definition, found 1
> > 2     E402 module level import not at top of file
> >
> >>> +    pid_file = file_path("pid")
> >>> +
> >>> +    cmd = list(qemu_nbd_args)
> >>> +    cmd.extend(('--persistent', '--pid-file', pid_file))
> >>> +    cmd.extend(args)
> >>> +
> >>> +    log('Start NBD server')
> >>> +    p = subprocess.Popen(cmd)
> >>> +    try:
> >>> +        while not os.path.exists(pid_file):
> >>> +            if p.poll() is not None:
> >>> +                raise RuntimeError(
> >>> +                    "qemu-nbd terminated with exit code {}: {}"
> >>> +                    .format(p.returncode, ' '.join(cmd)))
> >>> +
> >>> +            time.sleep(0.01)
> >>> +        yield
> >>> +    finally:
> >>> +        log('Kill NBD server')
> >>> +        p.kill()
> >>> +        p.wait()
> >>
> >> why do we need try-finally? I think, the only possible exception is your "raise RuntimeError", and in this case the process is alredy dead, no need to kill it (and print the log message)
> >
> > The try-finally is needed for errors in user code like:
> >
> >      with qemu_nbd_popen():
> >           assert 0
> >
>
> Ahh, so this "finally" will work, on some exception during yield? OK then.
>
> > Without try-finally qemu-nbd will continue to run after the assert
> > fails, which may
> > cause trouble, depending on the test.
> >
> > In the case of the RuntimeError inside the try, the cleanup in finally is
> > not needed but harmless.
> >
> > Looking in python source:
> >
> >          def send_signal(self, sig):
> >              """Send a signal to the process."""
> >              # Skip signalling a process that we know has already died.
> >              if self.returncode is None:
> >                  os.kill(self.pid, sig)
> >
> >          def kill(self):
> >              """Kill the process with SIGKILL
> >              """
> >              self.send_signal(signal.SIGKILL)
> >
> > So the kill() will do nothing, and wait() will return immediately.
> >
> >>>    def compare_images(img1, img2, fmt1=imgfmt, fmt2=imgfmt):
> >>>        '''Return True if two image files are identical'''
> >>>
> >>
> >> anyway:
> >> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> >>
> >> --
> >> Best regards,
> >> Vladimir
> >>
> >
>
>
> --
> Best regards,
> Vladimir
>