[Qemu-devel] [PATCH 00/17] iotests: don't choke on disabled drivers

[Qemu-devel] [PATCH 00/17] iotests: don't choke on disabled drivers

[Roman Kagan]

Some iotests assume availability of certain block drivers, and fail if
the driver is not supported by QEMU because it was disabled at configure
time.

This series tries to address that, by making QEMU report the actual list
of supported block drivers in response to "-drive format=?", and using
this information to skip the parts of the io testsuite that can not be
run in this configuration.

Roman Kagan (17):
  block: iterate_format with account of whitelisting
  iotests: iotests.py: prevent deadlock in subprocess
  iotests: ask qemu for supported formats
  iotest 030: skip quorum test setup/teardown too
  iotest 030: require blkdebug
  iotest 055: skip unsupported backup target formats
  iotest 055: require blkdebug
  iotest 056: skip testcases using blkdebug if disabled
  iotest 071: notrun if blkdebug or blkverify is disabled
  iotest 081: notrun if quorum is disabled
  iotest 087: notrun if null-co is disabled
  iotest 093: notrun if null-co or null-aio is disabled
  iotest 099: notrun if blkdebug or blkverify is disabled
  iotest 124: skip testcases using blkdebug if disabled
  iotest 139: skip testcases using disabled drivers
  iotest 147: notrun if nbd is disabled
  iotest 184: notrun if null-co or throttle is disabled

 include/block/block.h         |  2 +-
 block.c                       | 23 ++++++++++++++++++----
 blockdev.c                    |  4 +++-
 qemu-img.c                    |  2 +-
 tests/qemu-iotests/030        |  7 +++++++
 tests/qemu-iotests/055        | 13 ++++++++++++
 tests/qemu-iotests/056        |  3 +++
 tests/qemu-iotests/071        |  1 +
 tests/qemu-iotests/081        |  1 +
 tests/qemu-iotests/087        |  1 +
 tests/qemu-iotests/093        |  1 +
 tests/qemu-iotests/099        |  1 +
 tests/qemu-iotests/124        |  5 +++++
 tests/qemu-iotests/139        |  4 ++++
 tests/qemu-iotests/147        |  1 +
 tests/qemu-iotests/184        |  1 +
 tests/qemu-iotests/common.rc  | 19 ++++++++++++++++++
 tests/qemu-iotests/iotests.py | 46 ++++++++++++++++++++++++++++++++-----------
 18 files changed, 117 insertions(+), 18 deletions(-)

-- 
2.14.3

[Qemu-devel] [PATCH 01/17] block: iterate_format with account of whitelisting

[Roman Kagan]

bdrv_iterate_format (which is currently only used for printing out the
formats supported by the block layer) doesn't take format whitelisting
into account.

As a result, QEMU lies when asked for the list of block drivers it
supports with "-drive format=?": some of the formats there may be
recognized by qemu-* tools but unusable in qemu proper.

To avoid that, exclude formats that are not whitelisted from
enumeration, if whitelisting is in use.  Since we have separate
whitelists for r/w and r/o, take this as a parameter to
bdrv_iterate_format, and print two lists of supported formats (r/w and
r/o) in main qemu.

Signed-off-by: Roman Kagan <rkagan@virtuozzo.com>
---
 include/block/block.h |  2 +-
 block.c               | 23 +++++++++++++++++++----
 blockdev.c            |  4 +++-
 qemu-img.c            |  2 +-
 4 files changed, 24 insertions(+), 7 deletions(-)

diff --git a/include/block/block.h b/include/block/block.h
index cdec3639a3..e60983248f 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -429,7 +429,7 @@ void bdrv_next_cleanup(BdrvNextIterator *it);
 BlockDriverState *bdrv_next_monitor_owned(BlockDriverState *bs);
 bool bdrv_is_encrypted(BlockDriverState *bs);
 void bdrv_iterate_format(void (*it)(void *opaque, const char *name),
-                         void *opaque);
+                         void *opaque, bool read_only);
 const char *bdrv_get_node_name(const BlockDriverState *bs);
 const char *bdrv_get_device_name(const BlockDriverState *bs);
 const char *bdrv_get_device_or_node_name(const BlockDriverState *bs);
diff --git a/block.c b/block.c
index a2caadf0a0..eaa73edc79 100644
--- a/block.c
+++ b/block.c
@@ -371,7 +371,7 @@ BlockDriver *bdrv_find_format(const char *format_name)
     return bdrv_do_find_format(format_name);
 }
 
-int bdrv_is_whitelisted(BlockDriver *drv, bool read_only)
+static int bdrv_format_is_whitelisted(const char *format_name, bool read_only)
 {
     static const char *whitelist_rw[] = {
         CONFIG_BDRV_RW_WHITELIST
@@ -386,13 +386,13 @@ int bdrv_is_whitelisted(BlockDriver *drv, bool read_only)
     }
 
     for (p = whitelist_rw; *p; p++) {
-        if (!strcmp(drv->format_name, *p)) {
+        if (!strcmp(format_name, *p)) {
             return 1;
         }
     }
     if (read_only) {
         for (p = whitelist_ro; *p; p++) {
-            if (!strcmp(drv->format_name, *p)) {
+            if (!strcmp(format_name, *p)) {
                 return 1;
             }
         }
@@ -400,6 +400,11 @@ int bdrv_is_whitelisted(BlockDriver *drv, bool read_only)
     return 0;
 }
 
+int bdrv_is_whitelisted(BlockDriver *drv, bool read_only)
+{
+    return bdrv_format_is_whitelisted(drv->format_name, read_only);
+}
+
 bool bdrv_uses_whitelist(void)
 {
     return use_bdrv_whitelist;
@@ -3886,7 +3891,7 @@ static int qsort_strcmp(const void *a, const void *b)
 }
 
 void bdrv_iterate_format(void (*it)(void *opaque, const char *name),
-                         void *opaque)
+                         void *opaque, bool read_only)
 {
     BlockDriver *drv;
     int count = 0;
@@ -3897,6 +3902,11 @@ void bdrv_iterate_format(void (*it)(void *opaque, const char *name),
         if (drv->format_name) {
             bool found = false;
             int i = count;
+
+            if (use_bdrv_whitelist && !bdrv_is_whitelisted(drv, read_only)) {
+                continue;
+            }
+
             while (formats && i && !found) {
                 found = !strcmp(formats[--i], drv->format_name);
             }
@@ -3915,6 +3925,11 @@ void bdrv_iterate_format(void (*it)(void *opaque, const char *name),
             bool found = false;
             int j = count;
 
+            if (use_bdrv_whitelist &&
+                !bdrv_format_is_whitelisted(format_name, read_only)) {
+                continue;
+            }
+
             while (formats && j && !found) {
                 found = !strcmp(formats[--j], format_name);
             }
diff --git a/blockdev.c b/blockdev.c
index c31bf3d98d..f43c6bcf27 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -530,7 +530,9 @@ static BlockBackend *blockdev_init(const char *file, QDict *bs_opts,
     if ((buf = qemu_opt_get(opts, "format")) != NULL) {
         if (is_help_option(buf)) {
             error_printf("Supported formats:");
-            bdrv_iterate_format(bdrv_format_print, NULL);
+            bdrv_iterate_format(bdrv_format_print, NULL, false);
+            error_printf("\nSupported formats (read-only):");
+            bdrv_iterate_format(bdrv_format_print, NULL, true);
             error_printf("\n");
             goto early_err;
         }
diff --git a/qemu-img.c b/qemu-img.c
index 855fa52514..a938cb253e 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -201,7 +201,7 @@ static void QEMU_NORETURN help(void)
            "  'skip=N' skip N bs-sized blocks at the start of input\n";
 
     printf("%s\nSupported formats:", help_msg);
-    bdrv_iterate_format(format_print, NULL);
+    bdrv_iterate_format(format_print, NULL, false);
     printf("\n\n" QEMU_HELP_BOTTOM "\n");
     exit(EXIT_SUCCESS);
 }
-- 
2.14.3

[Qemu-devel] [PATCH 02/17] iotests: iotests.py: prevent deadlock in subprocess

[Roman Kagan]

A subprocess whose std{out,err} is subprocess.PIPE may block writing its
output, so .wait() should not be called on it until the pipes are read
completely on the caller's side.

Subprocess.communicate takes care of this.

Signed-off-by: Roman Kagan <rkagan@virtuozzo.com>
---
 tests/qemu-iotests/iotests.py | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index b25d48a91b..e2abf0cb53 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -104,20 +104,20 @@ def qemu_img_pipe(*args):
     subp = subprocess.Popen(qemu_img_args + list(args),
                             stdout=subprocess.PIPE,
                             stderr=subprocess.STDOUT)
-    exitcode = subp.wait()
-    if exitcode < 0:
-        sys.stderr.write('qemu-img received signal %i: %s\n' % (-exitcode, ' '.join(qemu_img_args + list(args))))
-    return subp.communicate()[0]
+    output = subp.communicate()[0]
+    if subp.returncode < 0:
+        sys.stderr.write('qemu-img received signal %i: %s\n' % (-subp.returncode, ' '.join(qemu_img_args + list(args))))
+    return output
 
 def qemu_io(*args):
     '''Run qemu-io and return the stdout data'''
     args = qemu_io_args + list(args)
     subp = subprocess.Popen(args, stdout=subprocess.PIPE,
                             stderr=subprocess.STDOUT)
-    exitcode = subp.wait()
-    if exitcode < 0:
-        sys.stderr.write('qemu-io received signal %i: %s\n' % (-exitcode, ' '.join(args)))
-    return subp.communicate()[0]
+    output = subp.communicate()[0]
+    if subp.returncode < 0:
+        sys.stderr.write('qemu-io received signal %i: %s\n' % (-subp.returncode, ' '.join(args)))
+    return output
 
 
 class QemuIoInteractive:
-- 
2.14.3

[Qemu-devel] [PATCH 03/17] iotests: ask qemu for supported formats

[Roman Kagan]

Add helper functions to query the block drivers actually supported by
QEMU using "-drive format=?".  This allows to skip certain tests that
require drivers not built in or whitelisted in QEMU.

Signed-off-by: Roman Kagan <rkagan@virtuozzo.com>
---
 tests/qemu-iotests/common.rc  | 19 +++++++++++++++++++
 tests/qemu-iotests/iotests.py | 30 +++++++++++++++++++++++++++---
 2 files changed, 46 insertions(+), 3 deletions(-)

diff --git a/tests/qemu-iotests/common.rc b/tests/qemu-iotests/common.rc
index 9a65a11026..fe5a4d1cfd 100644
--- a/tests/qemu-iotests/common.rc
+++ b/tests/qemu-iotests/common.rc
@@ -493,5 +493,24 @@ _require_command()
     [ -x "$c" ] || _notrun "$1 utility required, skipped this test"
 }
 
+# this test requires support for specific formats
+#
+_require_format()
+{
+    supported_formats=$($QEMU_PROG $QEMU_OPTIONS -drive format=\? 2>&1 | \
+        head -1 | cut -d : -f 2)
+    for f; do
+        found=false
+        for sf in $supported_formats; do
+            if [ "$f" = "$sf" ]; then
+                found=true
+                break
+            fi
+        done
+
+        $found || _notrun "$QEMU_PROG doesn't support format $f"
+    done
+}
+
 # make sure this script returns success
 true
diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index e2abf0cb53..698ef2b2c0 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -119,6 +119,17 @@ def qemu_io(*args):
         sys.stderr.write('qemu-io received signal %i: %s\n' % (-subp.returncode, ' '.join(args)))
     return output
 
+def qemu_pipe(*args):
+    '''Run qemu with an option to print something and exit (e.g. a help option),
+    and return its output'''
+    args = [qemu_prog] + qemu_opts + list(args)
+    subp = subprocess.Popen(args, stdout=subprocess.PIPE,
+                            stderr=subprocess.STDOUT)
+    output = subp.communicate()[0]
+    if subp.returncode < 0:
+        sys.stderr.write('qemu received signal %i: %s\n' % (-subp.returncode, ' '.join(args)))
+    return output
+
 
 class QemuIoInteractive:
     def __init__(self, *args):
@@ -550,13 +561,26 @@ def verify_cache_mode(supported_cache_modes=[]):
     if supported_cache_modes and (cachemode not in supported_cache_modes):
         notrun('not suitable for this cache mode: %s' % cachemode)
 
+rw_formats = None
+
+def supports_format(format_name):
+    format_message = qemu_pipe('-drive', 'format=?')
+    global rw_formats
+    if rw_formats is None:
+        rw_formats = format_message.splitlines()[0].split(':')[1].split()
+    return format_name in rw_formats
+
+def require_formats(*formats):
+    for fmt in formats:
+        if not supports_format(fmt):
+            notrun('%s does not support format %s' % (qemu_prog, fmt))
+
 def supports_quorum():
-    return 'quorum' in qemu_img_pipe('--help')
+    return supports_format('quorum')
 
 def verify_quorum():
     '''Skip test suite if quorum support is not available'''
-    if not supports_quorum():
-        notrun('quorum support missing')
+    require_formats('quorum')
 
 def main(supported_fmts=[], supported_oses=['linux'], supported_cache_modes=[],
          unsupported_fmts=[]):
-- 
2.14.3

[Qemu-devel] [PATCH 04/17] iotest 030: skip quorum test setup/teardown too

[Roman Kagan]

If quorum driver is not enabled, test 030 skips the corresponding
testcase.  This, however, is insufficient: quorum is first used in the
testsuite's setUp.

To avoid erroring out here, skip setUp/tearDown, too.

Signed-off-by: Roman Kagan <rkagan@virtuozzo.com>
---
 tests/qemu-iotests/030 | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/tests/qemu-iotests/030 b/tests/qemu-iotests/030
index 640a6dfd10..6b20ff005e 100755
--- a/tests/qemu-iotests/030
+++ b/tests/qemu-iotests/030
@@ -423,6 +423,9 @@ class TestQuorum(iotests.QMPTestCase):
     backing = []
 
     def setUp(self):
+        if not iotests.supports_quorum():
+            return
+
         opts = ['driver=quorum', 'vote-threshold=2']
 
         # Initialize file names and command-line options
@@ -445,6 +448,9 @@ class TestQuorum(iotests.QMPTestCase):
         self.vm.launch()
 
     def tearDown(self):
+        if not iotests.supports_quorum():
+            return
+
         self.vm.shutdown()
         for img in self.children:
             os.remove(img)
-- 
2.14.3

[Qemu-devel] [PATCH 05/17] iotest 030: require blkdebug

[Roman Kagan]

This test uses blkdebug extensively so notrun it if blkdebug is
disabled in QEMU.

Signed-off-by: Roman Kagan <rkagan@virtuozzo.com>
---
 tests/qemu-iotests/030 | 1 +
 1 file changed, 1 insertion(+)

diff --git a/tests/qemu-iotests/030 b/tests/qemu-iotests/030
index 6b20ff005e..ac96331899 100755
--- a/tests/qemu-iotests/030
+++ b/tests/qemu-iotests/030
@@ -843,4 +843,5 @@ class TestSetSpeed(iotests.QMPTestCase):
         self.cancel_and_wait(resume=True)
 
 if __name__ == '__main__':
+    iotests.require_formats('blkdebug')
     iotests.main(supported_fmts=['qcow2', 'qed'])
-- 
2.14.3

[Qemu-devel] [PATCH 06/17] iotest 055: skip unsupported backup target formats

[Roman Kagan]

Signed-off-by: Roman Kagan <rkagan@virtuozzo.com>
---
 tests/qemu-iotests/055 | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/tests/qemu-iotests/055 b/tests/qemu-iotests/055
index 3437c11507..3a47aaa886 100755
--- a/tests/qemu-iotests/055
+++ b/tests/qemu-iotests/055
@@ -490,11 +490,15 @@ class TestDriveCompression(iotests.QMPTestCase):
 
     def test_complete_compress_drive_backup(self):
         for format in TestDriveCompression.fmt_supports_compression:
+            if not iotests.supports_format(format['type']):
+                continue
             self.do_test_compress_complete('drive-backup', format, False,
                                            target=blockdev_target_img, mode='existing')
 
     def test_complete_compress_blockdev_backup(self):
         for format in TestDriveCompression.fmt_supports_compression:
+            if not iotests.supports_format(format['type']):
+                continue
             self.do_test_compress_complete('blockdev-backup', format, True,
                                            target='drive1')
 
@@ -514,11 +518,15 @@ class TestDriveCompression(iotests.QMPTestCase):
 
     def test_compress_cancel_drive_backup(self):
         for format in TestDriveCompression.fmt_supports_compression:
+            if not iotests.supports_format(format['type']):
+                continue
             self.do_test_compress_cancel('drive-backup', format, False,
                                          target=blockdev_target_img, mode='existing')
 
     def test_compress_cancel_blockdev_backup(self):
        for format in TestDriveCompression.fmt_supports_compression:
+            if not iotests.supports_format(format['type']):
+                continue
             self.do_test_compress_cancel('blockdev-backup', format, True,
                                          target='drive1')
 
@@ -554,11 +562,15 @@ class TestDriveCompression(iotests.QMPTestCase):
 
     def test_compress_pause_drive_backup(self):
         for format in TestDriveCompression.fmt_supports_compression:
+            if not iotests.supports_format(format['type']):
+                continue
             self.do_test_compress_pause('drive-backup', format, False,
                                         target=blockdev_target_img, mode='existing')
 
     def test_compress_pause_blockdev_backup(self):
         for format in TestDriveCompression.fmt_supports_compression:
+            if not iotests.supports_format(format['type']):
+                continue
             self.do_test_compress_pause('blockdev-backup', format, True,
                                         target='drive1')
 
-- 
2.14.3

[Qemu-devel] [PATCH 07/17] iotest 055: require blkdebug

[Roman Kagan]

This test uses blkdebug extensively so notrun it if blkdebug is
disabled in QEMU.

Signed-off-by: Roman Kagan <rkagan@virtuozzo.com>
---
 tests/qemu-iotests/055 | 1 +
 1 file changed, 1 insertion(+)

diff --git a/tests/qemu-iotests/055 b/tests/qemu-iotests/055
index 3a47aaa886..02ea3cb72d 100755
--- a/tests/qemu-iotests/055
+++ b/tests/qemu-iotests/055
@@ -575,4 +575,5 @@ class TestDriveCompression(iotests.QMPTestCase):
                                         target='drive1')
 
 if __name__ == '__main__':
+    iotests.require_formats('blkdebug')
     iotests.main(supported_fmts=['raw', 'qcow2'])
-- 
2.14.3

[Qemu-devel] [PATCH 08/17] iotest 056: skip testcases using blkdebug if disabled

[Roman Kagan]

Signed-off-by: Roman Kagan <rkagan@virtuozzo.com>
---
 tests/qemu-iotests/056 | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/tests/qemu-iotests/056 b/tests/qemu-iotests/056
index 223292175a..ff40313851 100755
--- a/tests/qemu-iotests/056
+++ b/tests/qemu-iotests/056
@@ -230,6 +230,9 @@ class BackupTest(iotests.QMPTestCase):
                                  auto_dismiss=False)
 
     def dismissal_failure(self, dismissal_opt):
+        if not iotests.supports_format('blkdebug'):
+            return
+
         res = self.vm.qmp('query-block-jobs')
         self.assert_qmp(res, 'return', [])
         # Give blkdebug something to chew on
-- 
2.14.3

[Qemu-devel] [PATCH 09/17] iotest 071: notrun if blkdebug or blkverify is disabled

[Roman Kagan]

Signed-off-by: Roman Kagan <rkagan@virtuozzo.com>
---
 tests/qemu-iotests/071 | 1 +
 1 file changed, 1 insertion(+)

diff --git a/tests/qemu-iotests/071 b/tests/qemu-iotests/071
index 48b495513f..2385ab33be 100755
--- a/tests/qemu-iotests/071
+++ b/tests/qemu-iotests/071
@@ -40,6 +40,7 @@ trap "_cleanup; exit \$status" 0 1 2 3 15
 _supported_fmt qcow2
 _supported_proto file
 _supported_os Linux
+_require_format blkverify blkdebug
 
 function do_run_qemu()
 {
-- 
2.14.3

[Qemu-devel] [PATCH 10/17] iotest 081: notrun if quorum is disabled

[Roman Kagan]

Signed-off-by: Roman Kagan <rkagan@virtuozzo.com>
---
 tests/qemu-iotests/081 | 1 +
 1 file changed, 1 insertion(+)

diff --git a/tests/qemu-iotests/081 b/tests/qemu-iotests/081
index da3fb0984b..3489f2975f 100755
--- a/tests/qemu-iotests/081
+++ b/tests/qemu-iotests/081
@@ -42,6 +42,7 @@ trap "_cleanup; exit \$status" 0 1 2 3 15
 _supported_fmt raw
 _supported_proto file
 _supported_os Linux
+_require_format quorum
 
 function do_run_qemu()
 {
-- 
2.14.3

[Qemu-devel] [PATCH 11/17] iotest 087: notrun if null-co is disabled

[Roman Kagan]

Signed-off-by: Roman Kagan <rkagan@virtuozzo.com>
---
 tests/qemu-iotests/087 | 1 +
 1 file changed, 1 insertion(+)

diff --git a/tests/qemu-iotests/087 b/tests/qemu-iotests/087
index 2561a14456..83710babe3 100755
--- a/tests/qemu-iotests/087
+++ b/tests/qemu-iotests/087
@@ -34,6 +34,7 @@ status=1	# failure is the default!
 _supported_fmt qcow2
 _supported_proto file
 _supported_os Linux
+_require_format null-co
 
 function do_run_qemu()
 {
-- 
2.14.3

[Qemu-devel] [PATCH 12/17] iotest 093: notrun if null-co or null-aio is disabled

[Roman Kagan]

Signed-off-by: Roman Kagan <rkagan@virtuozzo.com>
---
 tests/qemu-iotests/093 | 1 +
 1 file changed, 1 insertion(+)

diff --git a/tests/qemu-iotests/093 b/tests/qemu-iotests/093
index c3404a3171..6a62f6f01d 100755
--- a/tests/qemu-iotests/093
+++ b/tests/qemu-iotests/093
@@ -372,4 +372,5 @@ class ThrottleTestRemovableMedia(iotests.QMPTestCase):
 
 
 if __name__ == '__main__':
+    iotests.require_formats("null-co", "null-aio")
     iotests.main(supported_fmts=["raw"])
-- 
2.14.3

[Qemu-devel] [PATCH 13/17] iotest 099: notrun if blkdebug or blkverify is disabled

[Roman Kagan]

Signed-off-by: Roman Kagan <rkagan@virtuozzo.com>
---
 tests/qemu-iotests/099 | 1 +
 1 file changed, 1 insertion(+)

diff --git a/tests/qemu-iotests/099 b/tests/qemu-iotests/099
index caaf58eee5..80dab909d5 100755
--- a/tests/qemu-iotests/099
+++ b/tests/qemu-iotests/099
@@ -45,6 +45,7 @@ _supported_proto file
 _supported_os Linux
 _unsupported_imgopts "subformat=monolithicFlat" "subformat=twoGbMaxExtentFlat" \
     "subformat=twoGbMaxExtentSparse"
+_require_format blkdebug blkverify
 
 function do_run_qemu()
 {
-- 
2.14.3

[Qemu-devel] [PATCH 14/17] iotest 124: skip testcases using blkdebug if disabled

[Roman Kagan]

Signed-off-by: Roman Kagan <rkagan@virtuozzo.com>
---
 tests/qemu-iotests/124 | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/tests/qemu-iotests/124 b/tests/qemu-iotests/124
index 8e76e62f93..f26c9170fb 100755
--- a/tests/qemu-iotests/124
+++ b/tests/qemu-iotests/124
@@ -401,6 +401,8 @@ class TestIncrementalBackup(TestIncrementalBackupBase):
 
 
     def do_transaction_failure_test(self, race=False):
+        if not iotests.supports_format('blkdebug'):
+            return
         # Create a second drive, with pattern:
         drive1 = self.add_node('drive1')
         self.img_create(drive1['file'], drive1['fmt'])
@@ -581,6 +583,9 @@ class TestIncrementalBackupBlkdebug(TestIncrementalBackupBase):
         afterwards and verify that the backup created is correct.
         '''
 
+        if not iotests.supports_format('blkdebug'):
+            return
+
         drive0 = self.drives[0]
         result = self.vm.qmp('blockdev-add',
             node_name=drive0['id'],
-- 
2.14.3

[Qemu-devel] [PATCH 15/17] iotest 139: skip testcases using disabled drivers

[Roman Kagan]

Signed-off-by: Roman Kagan <rkagan@virtuozzo.com>
---
 tests/qemu-iotests/139 | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/tests/qemu-iotests/139 b/tests/qemu-iotests/139
index cc7fe337f3..ca932a5c29 100755
--- a/tests/qemu-iotests/139
+++ b/tests/qemu-iotests/139
@@ -326,6 +326,8 @@ class TestBlockdevDel(iotests.QMPTestCase):
         #self.delBlockDriverState('mirror0')
 
     def testBlkDebug(self):
+        if not iotests.supports_format('blkdebug'):
+            return
         self.addBlkDebug('debug0', 'node0')
         # 'node0' is used by the blkdebug node
         self.delBlockDriverState('node0', expect_error = True)
@@ -334,6 +336,8 @@ class TestBlockdevDel(iotests.QMPTestCase):
         self.checkBlockDriverState('node0', False)
 
     def testBlkVerify(self):
+        if not iotests.supports_format('blkverify'):
+            return
         self.addBlkVerify('verify0', 'node0', 'node1')
         # We cannot remove the children of a blkverify device
         self.delBlockDriverState('node0', expect_error = True)
-- 
2.14.3

[Qemu-devel] [PATCH 16/17] iotest 147: notrun if nbd is disabled

[Roman Kagan]

Signed-off-by: Roman Kagan <rkagan@virtuozzo.com>
---
 tests/qemu-iotests/147 | 1 +
 1 file changed, 1 insertion(+)

diff --git a/tests/qemu-iotests/147 b/tests/qemu-iotests/147
index d2081df84b..f66a777351 100755
--- a/tests/qemu-iotests/147
+++ b/tests/qemu-iotests/147
@@ -249,6 +249,7 @@ class BuiltinNBD(NBDBlockdevAddBase):
 
 
 if __name__ == '__main__':
+    iotests.require_formats('nbd')
     # Need to support image creation
     iotests.main(supported_fmts=['vpc', 'parallels', 'qcow', 'vdi', 'qcow2',
                                  'vmdk', 'raw', 'vhdx', 'qed'])
-- 
2.14.3

[Qemu-devel] [PATCH 17/17] iotest 184: notrun if null-co or throttle is disabled

[Roman Kagan]

Signed-off-by: Roman Kagan <rkagan@virtuozzo.com>
---
 tests/qemu-iotests/184 | 1 +
 1 file changed, 1 insertion(+)

diff --git a/tests/qemu-iotests/184 b/tests/qemu-iotests/184
index 2b68284d58..87ae30df46 100755
--- a/tests/qemu-iotests/184
+++ b/tests/qemu-iotests/184
@@ -34,6 +34,7 @@ trap "exit \$status" 0 1 2 3 15
 . ./common.filter
 
 _supported_os Linux
+_require_format null-co throttle
 
 function do_run_qemu()
 {
-- 
2.14.3

Re: [Qemu-devel] [PATCH 00/17] iotests: don't choke on disabled drivers

[no-reply]

Hi,

This series seems to have some coding style problems. See output below for
more information:

Type: series
Message-id: 20180426161958.2872-1-rkagan@virtuozzo.com
Subject: [Qemu-devel] [PATCH 00/17] iotests: don't choke on disabled drivers

=== TEST SCRIPT BEGIN ===
#!/bin/bash

BASE=base
n=1
total=$(git log --oneline $BASE.. | wc -l)
failed=0

git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram

commits="$(git log --format=%H --reverse $BASE..)"
for c in $commits; do
    echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..."
    if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
        failed=1
        echo
    fi
    n=$((n+1))
done

exit $failed
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
 t [tag update]            patchew/1524666934-8064-1-git-send-email-lidongchen@tencent.com -> patchew/1524666934-8064-1-git-send-email-lidongchen@tencent.com
 * [new tag]               patchew/20180426161958.2872-1-rkagan@virtuozzo.com -> patchew/20180426161958.2872-1-rkagan@virtuozzo.com
Switched to a new branch 'test'
073994ede0 iotest 184: notrun if null-co or throttle is disabled
0d6a103a2e iotest 147: notrun if nbd is disabled
3b6c88c400 iotest 139: skip testcases using disabled drivers
e6f12551db iotest 124: skip testcases using blkdebug if disabled
14a2cc874d iotest 099: notrun if blkdebug or blkverify is disabled
f0a42278cc iotest 093: notrun if null-co or null-aio is disabled
eda5b858c7 iotest 087: notrun if null-co is disabled
dd607cdf8a iotest 081: notrun if quorum is disabled
78c6ef361f iotest 071: notrun if blkdebug or blkverify is disabled
a04fb3d3e2 iotest 056: skip testcases using blkdebug if disabled
df221ccdea iotest 055: require blkdebug
710254744d iotest 055: skip unsupported backup target formats
c635939e50 iotest 030: require blkdebug
5338c53d0f iotest 030: skip quorum test setup/teardown too
ca24e281d8 iotests: ask qemu for supported formats
642020ff29 iotests: iotests.py: prevent deadlock in subprocess
0c0f7d7005 block: iterate_format with account of whitelisting

=== OUTPUT BEGIN ===
Checking PATCH 1/17: block: iterate_format with account of whitelisting...
Checking PATCH 2/17: iotests: iotests.py: prevent deadlock in subprocess...
ERROR: line over 90 characters
#29: FILE: tests/qemu-iotests/iotests.py:109:
+        sys.stderr.write('qemu-img received signal %i: %s\n' % (-subp.returncode, ' '.join(qemu_img_args + list(args))))

ERROR: line over 90 characters
#43: FILE: tests/qemu-iotests/iotests.py:119:
+        sys.stderr.write('qemu-io received signal %i: %s\n' % (-subp.returncode, ' '.join(args)))

total: 2 errors, 0 warnings, 28 lines checked

Your patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

Checking PATCH 3/17: iotests: ask qemu for supported formats...
ERROR: line over 90 characters
#58: FILE: tests/qemu-iotests/iotests.py:130:
+        sys.stderr.write('qemu received signal %i: %s\n' % (-subp.returncode, ' '.join(args)))

total: 1 errors, 0 warnings, 70 lines checked

Your patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

Checking PATCH 4/17: iotest 030: skip quorum test setup/teardown too...
Checking PATCH 5/17: iotest 030: require blkdebug...
Checking PATCH 6/17: iotest 055: skip unsupported backup target formats...
Checking PATCH 7/17: iotest 055: require blkdebug...
Checking PATCH 8/17: iotest 056: skip testcases using blkdebug if disabled...
Checking PATCH 9/17: iotest 071: notrun if blkdebug or blkverify is disabled...
Checking PATCH 10/17: iotest 081: notrun if quorum is disabled...
Checking PATCH 11/17: iotest 087: notrun if null-co is disabled...
Checking PATCH 12/17: iotest 093: notrun if null-co or null-aio is disabled...
Checking PATCH 13/17: iotest 099: notrun if blkdebug or blkverify is disabled...
Checking PATCH 14/17: iotest 124: skip testcases using blkdebug if disabled...
Checking PATCH 15/17: iotest 139: skip testcases using disabled drivers...
Checking PATCH 16/17: iotest 147: notrun if nbd is disabled...
Checking PATCH 17/17: iotest 184: notrun if null-co or throttle is disabled...
=== OUTPUT END ===

Test command exited with code: 1


---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-devel@redhat.com

Re: [Qemu-devel] [PATCH 01/17] block: iterate_format with account of whitelisting

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 944 bytes --]

On 04/26/2018 11:19 AM, Roman Kagan wrote:
> bdrv_iterate_format (which is currently only used for printing out the
> formats supported by the block layer) doesn't take format whitelisting
> into account.
> 
> As a result, QEMU lies when asked for the list of block drivers it
> supports with "-drive format=?": some of the formats there may be
> recognized by qemu-* tools but unusable in qemu proper.
> 
> To avoid that, exclude formats that are not whitelisted from
> enumeration, if whitelisting is in use.  Since we have separate
> whitelists for r/w and r/o, take this as a parameter to
> bdrv_iterate_format, and print two lists of supported formats (r/w and
> r/o) in main qemu.
> 
> Signed-off-by: Roman Kagan <rkagan@virtuozzo.com>
> ---

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

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]

Re: [Qemu-devel] [Qemu-block] [PATCH 00/17] iotests: don't choke on disabled drivers

[Roman Kagan]

On Thu, Apr 26, 2018 at 07:19:41PM +0300, Roman Kagan wrote:
> Some iotests assume availability of certain block drivers, and fail if
> the driver is not supported by QEMU because it was disabled at configure
> time.
> 
> This series tries to address that, by making QEMU report the actual list
> of supported block drivers in response to "-drive format=?", and using
> this information to skip the parts of the io testsuite that can not be
> run in this configuration.
> 
> Roman Kagan (17):
>   block: iterate_format with account of whitelisting
>   iotests: iotests.py: prevent deadlock in subprocess
>   iotests: ask qemu for supported formats
>   iotest 030: skip quorum test setup/teardown too
>   iotest 030: require blkdebug
>   iotest 055: skip unsupported backup target formats
>   iotest 055: require blkdebug
>   iotest 056: skip testcases using blkdebug if disabled
>   iotest 071: notrun if blkdebug or blkverify is disabled
>   iotest 081: notrun if quorum is disabled
>   iotest 087: notrun if null-co is disabled
>   iotest 093: notrun if null-co or null-aio is disabled
>   iotest 099: notrun if blkdebug or blkverify is disabled
>   iotest 124: skip testcases using blkdebug if disabled
>   iotest 139: skip testcases using disabled drivers
>   iotest 147: notrun if nbd is disabled
>   iotest 184: notrun if null-co or throttle is disabled
> 
>  include/block/block.h         |  2 +-
>  block.c                       | 23 ++++++++++++++++++----
>  blockdev.c                    |  4 +++-
>  qemu-img.c                    |  2 +-
>  tests/qemu-iotests/030        |  7 +++++++
>  tests/qemu-iotests/055        | 13 ++++++++++++
>  tests/qemu-iotests/056        |  3 +++
>  tests/qemu-iotests/071        |  1 +
>  tests/qemu-iotests/081        |  1 +
>  tests/qemu-iotests/087        |  1 +
>  tests/qemu-iotests/093        |  1 +
>  tests/qemu-iotests/099        |  1 +
>  tests/qemu-iotests/124        |  5 +++++
>  tests/qemu-iotests/139        |  4 ++++
>  tests/qemu-iotests/147        |  1 +
>  tests/qemu-iotests/184        |  1 +
>  tests/qemu-iotests/common.rc  | 19 ++++++++++++++++++
>  tests/qemu-iotests/iotests.py | 46 ++++++++++++++++++++++++++++++++-----------
>  18 files changed, 117 insertions(+), 18 deletions(-)

Ping?

Re: [Qemu-devel] [PATCH 01/17] block: iterate_format with account of whitelisting

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 1023 bytes --]

On 2018-04-26 18:19, Roman Kagan wrote:
> bdrv_iterate_format (which is currently only used for printing out the
> formats supported by the block layer) doesn't take format whitelisting
> into account.
> 
> As a result, QEMU lies when asked for the list of block drivers it
> supports with "-drive format=?": some of the formats there may be
> recognized by qemu-* tools but unusable in qemu proper.
> 
> To avoid that, exclude formats that are not whitelisted from
> enumeration, if whitelisting is in use.  Since we have separate
> whitelists for r/w and r/o, take this as a parameter to
> bdrv_iterate_format, and print two lists of supported formats (r/w and
> r/o) in main qemu.
> 
> Signed-off-by: Roman Kagan <rkagan@virtuozzo.com>
> ---
>  include/block/block.h |  2 +-
>  block.c               | 23 +++++++++++++++++++----
>  blockdev.c            |  4 +++-
>  qemu-img.c            |  2 +-
>  4 files changed, 24 insertions(+), 7 deletions(-)

Reviewed-by: Max Reitz <mreitz@redhat.com>


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [Qemu-devel] [PATCH 02/17] iotests: iotests.py: prevent deadlock in subprocess

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 497 bytes --]

On 2018-04-26 18:19, Roman Kagan wrote:
> A subprocess whose std{out,err} is subprocess.PIPE may block writing its
> output, so .wait() should not be called on it until the pipes are read
> completely on the caller's side.
> 
> Subprocess.communicate takes care of this.
> 
> Signed-off-by: Roman Kagan <rkagan@virtuozzo.com>
> ---
>  tests/qemu-iotests/iotests.py | 16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)

Reviewed-by: Max Reitz <mreitz@redhat.com>


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [Qemu-devel] [PATCH 03/17] iotests: ask qemu for supported formats

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 1986 bytes --]

On 2018-04-26 18:19, Roman Kagan wrote:
> Add helper functions to query the block drivers actually supported by
> QEMU using "-drive format=?".  This allows to skip certain tests that
> require drivers not built in or whitelisted in QEMU.
> 
> Signed-off-by: Roman Kagan <rkagan@virtuozzo.com>
> ---
>  tests/qemu-iotests/common.rc  | 19 +++++++++++++++++++
>  tests/qemu-iotests/iotests.py | 30 +++++++++++++++++++++++++++---
>  2 files changed, 46 insertions(+), 3 deletions(-)

[...]

> diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
> index e2abf0cb53..698ef2b2c0 100644
> --- a/tests/qemu-iotests/iotests.py
> +++ b/tests/qemu-iotests/iotests.py

[...]

> @@ -550,13 +561,26 @@ def verify_cache_mode(supported_cache_modes=[]):
>      if supported_cache_modes and (cachemode not in supported_cache_modes):
>          notrun('not suitable for this cache mode: %s' % cachemode)
>  
> +rw_formats = None
> +
> +def supports_format(format_name):
> +    format_message = qemu_pipe('-drive', 'format=?')
> +    global rw_formats
> +    if rw_formats is None:
> +        rw_formats = format_message.splitlines()[0].split(':')[1].split()

Isn't it sufficient to call qemu_pipe() only if rw_formats is None?

The rest looks good.

Max

> +    return format_name in rw_formats
> +
> +def require_formats(*formats):
> +    for fmt in formats:
> +        if not supports_format(fmt):
> +            notrun('%s does not support format %s' % (qemu_prog, fmt))
> +
>  def supports_quorum():
> -    return 'quorum' in qemu_img_pipe('--help')
> +    return supports_format('quorum')
>  
>  def verify_quorum():
>      '''Skip test suite if quorum support is not available'''
> -    if not supports_quorum():
> -        notrun('quorum support missing')
> +    require_formats('quorum')
>  
>  def main(supported_fmts=[], supported_oses=['linux'], supported_cache_modes=[],
>           unsupported_fmts=[]):
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [Qemu-devel] [PATCH 04/17] iotest 030: skip quorum test setup/teardown too

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 511 bytes --]

On 2018-04-26 18:19, Roman Kagan wrote:
> If quorum driver is not enabled, test 030 skips the corresponding
> testcase.  This, however, is insufficient: quorum is first used in the
> testsuite's setUp.
> 
> To avoid erroring out here, skip setUp/tearDown, too.
> 
> Signed-off-by: Roman Kagan <rkagan@virtuozzo.com>
> ---
>  tests/qemu-iotests/030 | 6 ++++++
>  1 file changed, 6 insertions(+)

Not sure if there is any nicer way of doing this, but:

Reviewed-by: Max Reitz <mreitz@redhat.com>


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [Qemu-devel] [PATCH 05/17] iotest 030: require blkdebug

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 309 bytes --]

On 2018-04-26 18:19, Roman Kagan wrote:
> This test uses blkdebug extensively so notrun it if blkdebug is
> disabled in QEMU.
> 
> Signed-off-by: Roman Kagan <rkagan@virtuozzo.com>
> ---
>  tests/qemu-iotests/030 | 1 +
>  1 file changed, 1 insertion(+)

Reviewed-by: Max Reitz <mreitz@redhat.com>


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [Qemu-devel] [PATCH 06/17] iotest 055: skip unsupported backup target formats

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 231 bytes --]

On 2018-04-26 18:19, Roman Kagan wrote:
> Signed-off-by: Roman Kagan <rkagan@virtuozzo.com>
> ---
>  tests/qemu-iotests/055 | 12 ++++++++++++
>  1 file changed, 12 insertions(+)

Reviewed-by: Max Reitz <mreitz@redhat.com>


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [Qemu-devel] [PATCH 07/17] iotest 055: require blkdebug

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 309 bytes --]

On 2018-04-26 18:19, Roman Kagan wrote:
> This test uses blkdebug extensively so notrun it if blkdebug is
> disabled in QEMU.
> 
> Signed-off-by: Roman Kagan <rkagan@virtuozzo.com>
> ---
>  tests/qemu-iotests/055 | 1 +
>  1 file changed, 1 insertion(+)

Reviewed-by: Max Reitz <mreitz@redhat.com>


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [Qemu-devel] [PATCH 08/17] iotest 056: skip testcases using blkdebug if disabled

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 852 bytes --]

On 2018-04-26 18:19, Roman Kagan wrote:
> Signed-off-by: Roman Kagan <rkagan@virtuozzo.com>
> ---
>  tests/qemu-iotests/056 | 3 +++
>  1 file changed, 3 insertions(+)

TestBeforeWriteNotifier uses blkdebug (and null-co) in its setUp
function.  Maybe you just want to skip the whole test if blkdebug is
disabled.

Then again, I'd argue there are block drivers without which running the
iotests is a bit pointless.  Why not just require blkdebug, null-co/aio,
raw, and file for all of them?

I think the reason why we added the check for quorum support was because
quorum relies on an external library (for hashing), so it isn't trivial
to enable.  But you can always easily whitelist those drivers above if
you want to do testing, so I'm not sure whether it's worth checking
their availability in every test that needs them.

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [Qemu-devel] [PATCH 00/17] iotests: don't choke on disabled drivers

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 3260 bytes --]

On 2018-04-26 18:19, Roman Kagan wrote:
> Some iotests assume availability of certain block drivers, and fail if
> the driver is not supported by QEMU because it was disabled at configure
> time.
> 
> This series tries to address that, by making QEMU report the actual list
> of supported block drivers in response to "-drive format=?", and using
> this information to skip the parts of the io testsuite that can not be
> run in this configuration.
> 
> Roman Kagan (17):
>   block: iterate_format with account of whitelisting
>   iotests: iotests.py: prevent deadlock in subprocess
>   iotests: ask qemu for supported formats
>   iotest 030: skip quorum test setup/teardown too
>   iotest 030: require blkdebug
>   iotest 055: skip unsupported backup target formats
>   iotest 055: require blkdebug
>   iotest 056: skip testcases using blkdebug if disabled
>   iotest 071: notrun if blkdebug or blkverify is disabled
>   iotest 081: notrun if quorum is disabled
>   iotest 087: notrun if null-co is disabled
>   iotest 093: notrun if null-co or null-aio is disabled
>   iotest 099: notrun if blkdebug or blkverify is disabled
>   iotest 124: skip testcases using blkdebug if disabled
>   iotest 139: skip testcases using disabled drivers
>   iotest 147: notrun if nbd is disabled
>   iotest 184: notrun if null-co or throttle is disabled
> 
>  include/block/block.h         |  2 +-
>  block.c                       | 23 ++++++++++++++++++----
>  blockdev.c                    |  4 +++-
>  qemu-img.c                    |  2 +-
>  tests/qemu-iotests/030        |  7 +++++++
>  tests/qemu-iotests/055        | 13 ++++++++++++
>  tests/qemu-iotests/056        |  3 +++
>  tests/qemu-iotests/071        |  1 +
>  tests/qemu-iotests/081        |  1 +
>  tests/qemu-iotests/087        |  1 +
>  tests/qemu-iotests/093        |  1 +
>  tests/qemu-iotests/099        |  1 +
>  tests/qemu-iotests/124        |  5 +++++
>  tests/qemu-iotests/139        |  4 ++++
>  tests/qemu-iotests/147        |  1 +
>  tests/qemu-iotests/184        |  1 +
>  tests/qemu-iotests/common.rc  | 19 ++++++++++++++++++
>  tests/qemu-iotests/iotests.py | 46 ++++++++++++++++++++++++++++++++-----------
>  18 files changed, 117 insertions(+), 18 deletions(-)

I'll stop reviewing this series for now, because there are more iotests
that use drivers outside of their format/protocol combination.

For instance:

$ grep -l null-co ??? | wc -l
15
$ grep -l blkdebug ??? | wc -l
30
$ (grep -l '"raw"' ???; grep -l "'raw'" ???) | wc -l
22

As I've written in my reply to patch 8, I'm not sure whether it's the
right solution to check for the availability of these block drivers in
every single test that needs them.  It makes sense for quorum, because
quorum needs an external library for hashing, so it may not be trivial
to enable.  But it does not seem too useful for other formats that do
not have such a dependency (e.g. null-co, blkdebug, raw).

The thing is that it's OK to whitelist everything for testing, and then
disable some drivers when building a release.  I don't think one needs
to run the iotests with the release version if the whole difference is
whether some drivers have been disabled or not.

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [Qemu-devel] [PATCH 03/17] iotests: ask qemu for supported formats

[Roman Kagan]

On Wed, May 30, 2018 at 02:17:55PM +0200, Max Reitz wrote:
> On 2018-04-26 18:19, Roman Kagan wrote:
> > @@ -550,13 +561,26 @@ def verify_cache_mode(supported_cache_modes=[]):
> >      if supported_cache_modes and (cachemode not in supported_cache_modes):
> >          notrun('not suitable for this cache mode: %s' % cachemode)
> >  
> > +rw_formats = None
> > +
> > +def supports_format(format_name):
> > +    format_message = qemu_pipe('-drive', 'format=?')
> > +    global rw_formats
> > +    if rw_formats is None:
> > +        rw_formats = format_message.splitlines()[0].split(':')[1].split()
> 
> Isn't it sufficient to call qemu_pipe() only if rw_formats is None?

Sure it is, and this was exactly my intention with this global, but I
managed to forget moving the most relevant part under 'if' :$

Thanks for catching,
Roman.

Re: [Qemu-devel] [PATCH 00/17] iotests: don't choke on disabled drivers

[Roman Kagan]

On Wed, May 30, 2018 at 02:35:34PM +0200, Max Reitz wrote:
> On 2018-04-26 18:19, Roman Kagan wrote:
> > Some iotests assume availability of certain block drivers, and fail if
> > the driver is not supported by QEMU because it was disabled at configure
> > time.
> > 
> > This series tries to address that, by making QEMU report the actual list
> > of supported block drivers in response to "-drive format=?", and using
> > this information to skip the parts of the io testsuite that can not be
> > run in this configuration.
> > 
> > Roman Kagan (17):
> >   block: iterate_format with account of whitelisting
> >   iotests: iotests.py: prevent deadlock in subprocess
> >   iotests: ask qemu for supported formats
> >   iotest 030: skip quorum test setup/teardown too
> >   iotest 030: require blkdebug
> >   iotest 055: skip unsupported backup target formats
> >   iotest 055: require blkdebug
> >   iotest 056: skip testcases using blkdebug if disabled
> >   iotest 071: notrun if blkdebug or blkverify is disabled
> >   iotest 081: notrun if quorum is disabled
> >   iotest 087: notrun if null-co is disabled
> >   iotest 093: notrun if null-co or null-aio is disabled
> >   iotest 099: notrun if blkdebug or blkverify is disabled
> >   iotest 124: skip testcases using blkdebug if disabled
> >   iotest 139: skip testcases using disabled drivers
> >   iotest 147: notrun if nbd is disabled
> >   iotest 184: notrun if null-co or throttle is disabled
> > 
> >  include/block/block.h         |  2 +-
> >  block.c                       | 23 ++++++++++++++++++----
> >  blockdev.c                    |  4 +++-
> >  qemu-img.c                    |  2 +-
> >  tests/qemu-iotests/030        |  7 +++++++
> >  tests/qemu-iotests/055        | 13 ++++++++++++
> >  tests/qemu-iotests/056        |  3 +++
> >  tests/qemu-iotests/071        |  1 +
> >  tests/qemu-iotests/081        |  1 +
> >  tests/qemu-iotests/087        |  1 +
> >  tests/qemu-iotests/093        |  1 +
> >  tests/qemu-iotests/099        |  1 +
> >  tests/qemu-iotests/124        |  5 +++++
> >  tests/qemu-iotests/139        |  4 ++++
> >  tests/qemu-iotests/147        |  1 +
> >  tests/qemu-iotests/184        |  1 +
> >  tests/qemu-iotests/common.rc  | 19 ++++++++++++++++++
> >  tests/qemu-iotests/iotests.py | 46 ++++++++++++++++++++++++++++++++-----------
> >  18 files changed, 117 insertions(+), 18 deletions(-)
> 
> I'll stop reviewing this series for now, because there are more iotests
> that use drivers outside of their format/protocol combination.
> 
> For instance:
> 
> $ grep -l null-co ??? | wc -l
> 15
> $ grep -l blkdebug ??? | wc -l
> 30
> $ (grep -l '"raw"' ???; grep -l "'raw'" ???) | wc -l
> 22
> 
> As I've written in my reply to patch 8, I'm not sure whether it's the
> right solution to check for the availability of these block drivers in
> every single test that needs them.  It makes sense for quorum, because
> quorum needs an external library for hashing, so it may not be trivial
> to enable.  But it does not seem too useful for other formats that do
> not have such a dependency (e.g. null-co, blkdebug, raw).
> 
> The thing is that it's OK to whitelist everything for testing, and then
> disable some drivers when building a release.  I don't think one needs
> to run the iotests with the release version if the whole difference is
> whether some drivers have been disabled or not.

This patchset was created when we started to run (quick) iotests as a part
of the package build, which looked like a decent alternative to building
separate CI infrastructure.

If there's no general interest in running iotests against QEMU built in
a release configuration with certain drivers blacklisted, I think we
should be fine handling this locally.

Thanks,
Roman.

Re: [Qemu-devel] [PATCH 00/17] iotests: don't choke on disabled drivers

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 4561 bytes --]

On 2018-05-30 15:47, Roman Kagan wrote:
> On Wed, May 30, 2018 at 02:35:34PM +0200, Max Reitz wrote:
>> On 2018-04-26 18:19, Roman Kagan wrote:
>>> Some iotests assume availability of certain block drivers, and fail if
>>> the driver is not supported by QEMU because it was disabled at configure
>>> time.
>>>
>>> This series tries to address that, by making QEMU report the actual list
>>> of supported block drivers in response to "-drive format=?", and using
>>> this information to skip the parts of the io testsuite that can not be
>>> run in this configuration.
>>>
>>> Roman Kagan (17):
>>>   block: iterate_format with account of whitelisting
>>>   iotests: iotests.py: prevent deadlock in subprocess
>>>   iotests: ask qemu for supported formats
>>>   iotest 030: skip quorum test setup/teardown too
>>>   iotest 030: require blkdebug
>>>   iotest 055: skip unsupported backup target formats
>>>   iotest 055: require blkdebug
>>>   iotest 056: skip testcases using blkdebug if disabled
>>>   iotest 071: notrun if blkdebug or blkverify is disabled
>>>   iotest 081: notrun if quorum is disabled
>>>   iotest 087: notrun if null-co is disabled
>>>   iotest 093: notrun if null-co or null-aio is disabled
>>>   iotest 099: notrun if blkdebug or blkverify is disabled
>>>   iotest 124: skip testcases using blkdebug if disabled
>>>   iotest 139: skip testcases using disabled drivers
>>>   iotest 147: notrun if nbd is disabled
>>>   iotest 184: notrun if null-co or throttle is disabled
>>>
>>>  include/block/block.h         |  2 +-
>>>  block.c                       | 23 ++++++++++++++++++----
>>>  blockdev.c                    |  4 +++-
>>>  qemu-img.c                    |  2 +-
>>>  tests/qemu-iotests/030        |  7 +++++++
>>>  tests/qemu-iotests/055        | 13 ++++++++++++
>>>  tests/qemu-iotests/056        |  3 +++
>>>  tests/qemu-iotests/071        |  1 +
>>>  tests/qemu-iotests/081        |  1 +
>>>  tests/qemu-iotests/087        |  1 +
>>>  tests/qemu-iotests/093        |  1 +
>>>  tests/qemu-iotests/099        |  1 +
>>>  tests/qemu-iotests/124        |  5 +++++
>>>  tests/qemu-iotests/139        |  4 ++++
>>>  tests/qemu-iotests/147        |  1 +
>>>  tests/qemu-iotests/184        |  1 +
>>>  tests/qemu-iotests/common.rc  | 19 ++++++++++++++++++
>>>  tests/qemu-iotests/iotests.py | 46 ++++++++++++++++++++++++++++++++-----------
>>>  18 files changed, 117 insertions(+), 18 deletions(-)
>>
>> I'll stop reviewing this series for now, because there are more iotests
>> that use drivers outside of their format/protocol combination.
>>
>> For instance:
>>
>> $ grep -l null-co ??? | wc -l
>> 15
>> $ grep -l blkdebug ??? | wc -l
>> 30
>> $ (grep -l '"raw"' ???; grep -l "'raw'" ???) | wc -l
>> 22
>>
>> As I've written in my reply to patch 8, I'm not sure whether it's the
>> right solution to check for the availability of these block drivers in
>> every single test that needs them.  It makes sense for quorum, because
>> quorum needs an external library for hashing, so it may not be trivial
>> to enable.  But it does not seem too useful for other formats that do
>> not have such a dependency (e.g. null-co, blkdebug, raw).
>>
>> The thing is that it's OK to whitelist everything for testing, and then
>> disable some drivers when building a release.  I don't think one needs
>> to run the iotests with the release version if the whole difference is
>> whether some drivers have been disabled or not.
> 
> This patchset was created when we started to run (quick) iotests as a part
> of the package build, which looked like a decent alternative to building
> separate CI infrastructure.
> 
> If there's no general interest in running iotests against QEMU built in
> a release configuration with certain drivers blacklisted, I think we
> should be fine handling this locally.

It's not like the majority of drivers uses blkdebug, but still, it seems
at least suboptimal to run the tests if you then disable a big chunk of
them because some drivers were not whitelisted.

If you're OK with a local solution...  OK, but naively I'd think that
it'd be better to just build two versions for packaging: One that's
actually going to be packaged, and one that's used for testing.  (Though
I don't know how well that fits into your process.)

In any case, I'm not fully opposed to this series, but it would need to
be complete and make every test require every block driver that is not
part of its protocol/format combination.

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [Qemu-devel] [PATCH 00/17] iotests: don't choke on disabled drivers

[Roman Kagan]

On Wed, May 30, 2018 at 03:53:52PM +0200, Max Reitz wrote:
> On 2018-05-30 15:47, Roman Kagan wrote:
> > On Wed, May 30, 2018 at 02:35:34PM +0200, Max Reitz wrote:
> >> On 2018-04-26 18:19, Roman Kagan wrote:
> >>> Some iotests assume availability of certain block drivers, and fail if
> >>> the driver is not supported by QEMU because it was disabled at configure
> >>> time.
> >>>
> >>> This series tries to address that, by making QEMU report the actual list
> >>> of supported block drivers in response to "-drive format=?", and using
> >>> this information to skip the parts of the io testsuite that can not be
> >>> run in this configuration.
> >>>
> >>> Roman Kagan (17):
> >>>   block: iterate_format with account of whitelisting
> >>>   iotests: iotests.py: prevent deadlock in subprocess
> >>>   iotests: ask qemu for supported formats
> >>>   iotest 030: skip quorum test setup/teardown too
> >>>   iotest 030: require blkdebug
> >>>   iotest 055: skip unsupported backup target formats
> >>>   iotest 055: require blkdebug
> >>>   iotest 056: skip testcases using blkdebug if disabled
> >>>   iotest 071: notrun if blkdebug or blkverify is disabled
> >>>   iotest 081: notrun if quorum is disabled
> >>>   iotest 087: notrun if null-co is disabled
> >>>   iotest 093: notrun if null-co or null-aio is disabled
> >>>   iotest 099: notrun if blkdebug or blkverify is disabled
> >>>   iotest 124: skip testcases using blkdebug if disabled
> >>>   iotest 139: skip testcases using disabled drivers
> >>>   iotest 147: notrun if nbd is disabled
> >>>   iotest 184: notrun if null-co or throttle is disabled
> >>>
> >>>  include/block/block.h         |  2 +-
> >>>  block.c                       | 23 ++++++++++++++++++----
> >>>  blockdev.c                    |  4 +++-
> >>>  qemu-img.c                    |  2 +-
> >>>  tests/qemu-iotests/030        |  7 +++++++
> >>>  tests/qemu-iotests/055        | 13 ++++++++++++
> >>>  tests/qemu-iotests/056        |  3 +++
> >>>  tests/qemu-iotests/071        |  1 +
> >>>  tests/qemu-iotests/081        |  1 +
> >>>  tests/qemu-iotests/087        |  1 +
> >>>  tests/qemu-iotests/093        |  1 +
> >>>  tests/qemu-iotests/099        |  1 +
> >>>  tests/qemu-iotests/124        |  5 +++++
> >>>  tests/qemu-iotests/139        |  4 ++++
> >>>  tests/qemu-iotests/147        |  1 +
> >>>  tests/qemu-iotests/184        |  1 +
> >>>  tests/qemu-iotests/common.rc  | 19 ++++++++++++++++++
> >>>  tests/qemu-iotests/iotests.py | 46 ++++++++++++++++++++++++++++++++-----------
> >>>  18 files changed, 117 insertions(+), 18 deletions(-)
> >>
> >> I'll stop reviewing this series for now, because there are more iotests
> >> that use drivers outside of their format/protocol combination.
> >>
> >> For instance:
> >>
> >> $ grep -l null-co ??? | wc -l
> >> 15
> >> $ grep -l blkdebug ??? | wc -l
> >> 30
> >> $ (grep -l '"raw"' ???; grep -l "'raw'" ???) | wc -l
> >> 22
> >>
> >> As I've written in my reply to patch 8, I'm not sure whether it's the
> >> right solution to check for the availability of these block drivers in
> >> every single test that needs them.  It makes sense for quorum, because
> >> quorum needs an external library for hashing, so it may not be trivial
> >> to enable.  But it does not seem too useful for other formats that do
> >> not have such a dependency (e.g. null-co, blkdebug, raw).
> >>
> >> The thing is that it's OK to whitelist everything for testing, and then
> >> disable some drivers when building a release.  I don't think one needs
> >> to run the iotests with the release version if the whole difference is
> >> whether some drivers have been disabled or not.
> > 
> > This patchset was created when we started to run (quick) iotests as a part
> > of the package build, which looked like a decent alternative to building
> > separate CI infrastructure.
> > 
> > If there's no general interest in running iotests against QEMU built in
> > a release configuration with certain drivers blacklisted, I think we
> > should be fine handling this locally.
> 
> It's not like the majority of drivers uses blkdebug, but still, it seems
> at least suboptimal to run the tests if you then disable a big chunk of
> them because some drivers were not whitelisted.

Indeed.  In fact, our configuration (based on RedHat's) has those three
(null-co, blkdebug, raw) enabled so the patchset didn't result in
massive test skips.

> If you're OK with a local solution...  OK, but naively I'd think that
> it'd be better to just build two versions for packaging: One that's
> actually going to be packaged, and one that's used for testing.  (Though
> I don't know how well that fits into your process.)

It doesn't: we just added a couple of lines to the %check section of the
RPM .spec

> In any case, I'm not fully opposed to this series, but it would need to
> be complete and make every test require every block driver that is not
> part of its protocol/format combination.

OK this doesn't look like too much of work; I'll see if I can do it
quickly.

Thanks,
Roman.

Re: [Qemu-devel] [PATCH 03/17] iotests: ask qemu for supported formats

[Markus Armbruster]

Roman Kagan <rkagan@virtuozzo.com> writes:

> Add helper functions to query the block drivers actually supported by
> QEMU using "-drive format=?".  This allows to skip certain tests that
> require drivers not built in or whitelisted in QEMU.
>
> Signed-off-by: Roman Kagan <rkagan@virtuozzo.com>
> ---
>  tests/qemu-iotests/common.rc  | 19 +++++++++++++++++++
>  tests/qemu-iotests/iotests.py | 30 +++++++++++++++++++++++++++---
>  2 files changed, 46 insertions(+), 3 deletions(-)
>
> diff --git a/tests/qemu-iotests/common.rc b/tests/qemu-iotests/common.rc
> index 9a65a11026..fe5a4d1cfd 100644
> --- a/tests/qemu-iotests/common.rc
> +++ b/tests/qemu-iotests/common.rc
> @@ -493,5 +493,24 @@ _require_command()
>      [ -x "$c" ] || _notrun "$1 utility required, skipped this test"
>  }
>  
> +# this test requires support for specific formats
> +#
> +_require_format()
> +{
> +    supported_formats=$($QEMU_PROG $QEMU_OPTIONS -drive format=\? 2>&1 | \

Use of '?' to get help is deprecated.  Please use 'format=help', and
update your commit message accordingly.

> +        head -1 | cut -d : -f 2)
> +    for f; do
> +        found=false
> +        for sf in $supported_formats; do
> +            if [ "$f" = "$sf" ]; then
> +                found=true
> +                break
> +            fi
> +        done
> +
> +        $found || _notrun "$QEMU_PROG doesn't support format $f"
> +    done
> +}
> +
>  # make sure this script returns success
>  true
> diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
> index e2abf0cb53..698ef2b2c0 100644
> --- a/tests/qemu-iotests/iotests.py
> +++ b/tests/qemu-iotests/iotests.py
> @@ -119,6 +119,17 @@ def qemu_io(*args):
>          sys.stderr.write('qemu-io received signal %i: %s\n' % (-subp.returncode, ' '.join(args)))
>      return output
>  
> +def qemu_pipe(*args):
> +    '''Run qemu with an option to print something and exit (e.g. a help option),
> +    and return its output'''
> +    args = [qemu_prog] + qemu_opts + list(args)
> +    subp = subprocess.Popen(args, stdout=subprocess.PIPE,
> +                            stderr=subprocess.STDOUT)
> +    output = subp.communicate()[0]
> +    if subp.returncode < 0:
> +        sys.stderr.write('qemu received signal %i: %s\n' % (-subp.returncode, ' '.join(args)))
> +    return output
> +
>  
>  class QemuIoInteractive:
>      def __init__(self, *args):
> @@ -550,13 +561,26 @@ def verify_cache_mode(supported_cache_modes=[]):
>      if supported_cache_modes and (cachemode not in supported_cache_modes):
>          notrun('not suitable for this cache mode: %s' % cachemode)
>  
> +rw_formats = None
> +
> +def supports_format(format_name):
> +    format_message = qemu_pipe('-drive', 'format=?')

Likewise.

> +    global rw_formats
> +    if rw_formats is None:
> +        rw_formats = format_message.splitlines()[0].split(':')[1].split()
> +    return format_name in rw_formats
> +
> +def require_formats(*formats):
> +    for fmt in formats:
> +        if not supports_format(fmt):
> +            notrun('%s does not support format %s' % (qemu_prog, fmt))
> +
>  def supports_quorum():
> -    return 'quorum' in qemu_img_pipe('--help')
> +    return supports_format('quorum')
>  
>  def verify_quorum():
>      '''Skip test suite if quorum support is not available'''
> -    if not supports_quorum():
> -        notrun('quorum support missing')
> +    require_formats('quorum')
>  
>  def main(supported_fmts=[], supported_oses=['linux'], supported_cache_modes=[],
>           unsupported_fmts=[]):

Re: [Qemu-devel] [PATCH 03/17] iotests: ask qemu for supported formats

[Thomas Huth]

On 04.06.2018 09:18, Markus Armbruster wrote:
> Roman Kagan <rkagan@virtuozzo.com> writes:
> 
>> Add helper functions to query the block drivers actually supported by
>> QEMU using "-drive format=?".  This allows to skip certain tests that
>> require drivers not built in or whitelisted in QEMU.
>>
>> Signed-off-by: Roman Kagan <rkagan@virtuozzo.com>
>> ---
>>  tests/qemu-iotests/common.rc  | 19 +++++++++++++++++++
>>  tests/qemu-iotests/iotests.py | 30 +++++++++++++++++++++++++++---
>>  2 files changed, 46 insertions(+), 3 deletions(-)
>>
>> diff --git a/tests/qemu-iotests/common.rc b/tests/qemu-iotests/common.rc
>> index 9a65a11026..fe5a4d1cfd 100644
>> --- a/tests/qemu-iotests/common.rc
>> +++ b/tests/qemu-iotests/common.rc
>> @@ -493,5 +493,24 @@ _require_command()
>>      [ -x "$c" ] || _notrun "$1 utility required, skipped this test"
>>  }
>>  
>> +# this test requires support for specific formats
>> +#
>> +_require_format()
>> +{
>> +    supported_formats=$($QEMU_PROG $QEMU_OPTIONS -drive format=\? 2>&1 | \
> 
> Use of '?' to get help is deprecated.  Please use 'format=help', and
> update your commit message accordingly.

Is it? Where did we document that?

 Thomas

Re: [Qemu-devel] [PATCH 03/17] iotests: ask qemu for supported formats

[Eric Blake]

On 06/04/2018 05:34 AM, Thomas Huth wrote:
> On 04.06.2018 09:18, Markus Armbruster wrote:
>> Roman Kagan <rkagan@virtuozzo.com> writes:
>>
>>> Add helper functions to query the block drivers actually supported by
>>> QEMU using "-drive format=?".  This allows to skip certain tests that
>>> require drivers not built in or whitelisted in QEMU.
>>>

>>> +    supported_formats=$($QEMU_PROG $QEMU_OPTIONS -drive format=\? 2>&1 | \
>>
>> Use of '?' to get help is deprecated.  Please use 'format=help', and
>> update your commit message accordingly.
> 
> Is it? Where did we document that?

Hmm, we haven't documented it yet, but it's been that way since commit 
c8057f95, in Aug 2012, when we added 'help' as the preferred synonym, 
and have tried to avoid mention of '?' in new documentation (as it 
requires additional shell quoting).  I'm guessing we'll probably see a 
patch from you to start an official deprecation window?

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

Re: [Qemu-devel] [PATCH 03/17] iotests: ask qemu for supported formats

[Thomas Huth]

On 05.06.2018 00:40, Eric Blake wrote:
> On 06/04/2018 05:34 AM, Thomas Huth wrote:
>> On 04.06.2018 09:18, Markus Armbruster wrote:
>>> Roman Kagan <rkagan@virtuozzo.com> writes:
>>>
>>>> Add helper functions to query the block drivers actually supported by
>>>> QEMU using "-drive format=?".  This allows to skip certain tests that
>>>> require drivers not built in or whitelisted in QEMU.
>>>>
> 
>>>> +    supported_formats=$($QEMU_PROG $QEMU_OPTIONS -drive format=\?
>>>> 2>&1 | \
>>>
>>> Use of '?' to get help is deprecated.  Please use 'format=help', and
>>> update your commit message accordingly.
>>
>> Is it? Where did we document that?
> 
> Hmm, we haven't documented it yet, but it's been that way since commit
> c8057f95, in Aug 2012, when we added 'help' as the preferred synonym,
> and have tried to avoid mention of '?' in new documentation (as it
> requires additional shell quoting).  I'm guessing we'll probably see a
> patch from you to start an official deprecation window?

I'm using '?' regularly on my own, so don't expect a patch from
my side here ;-)
Anyway, we still use the question mark in our documentation, e.g.:

options

    is a comma separated list of format specific options in a name=value format.
    Use -o ? for an overview of the options supported by the used format or see
    the format descriptions below for details.

or:

-b, --blacklist=list

    Comma-separated list of RPCs to disable (no spaces, ‘?’ to list available RPCs)

So calling it deprecated sounds wrong to me.

 Thomas

Re: [Qemu-devel] [PATCH 03/17] iotests: ask qemu for supported formats

[Markus Armbruster]

Thomas Huth <thuth@redhat.com> writes:

> On 05.06.2018 00:40, Eric Blake wrote:
>> On 06/04/2018 05:34 AM, Thomas Huth wrote:
>>> On 04.06.2018 09:18, Markus Armbruster wrote:
>>>> Roman Kagan <rkagan@virtuozzo.com> writes:
>>>>
>>>>> Add helper functions to query the block drivers actually supported by
>>>>> QEMU using "-drive format=?".  This allows to skip certain tests that
>>>>> require drivers not built in or whitelisted in QEMU.
>>>>>
>> 
>>>>> +    supported_formats=$($QEMU_PROG $QEMU_OPTIONS -drive format=\?
>>>>> 2>&1 | \
>>>>
>>>> Use of '?' to get help is deprecated.  Please use 'format=help', and
>>>> update your commit message accordingly.
>>>
>>> Is it? Where did we document that?
>> 
>> Hmm, we haven't documented it yet, but it's been that way since commit
>> c8057f95, in Aug 2012, when we added 'help' as the preferred synonym,
>> and have tried to avoid mention of '?' in new documentation (as it
>> requires additional shell quoting).  I'm guessing we'll probably see a
>> patch from you to start an official deprecation window?
>
> I'm using '?' regularly on my own, so don't expect a patch from
> my side here ;-)
> Anyway, we still use the question mark in our documentation, e.g.:
>
> options
>
>     is a comma separated list of format specific options in a name=value format.
>     Use -o ? for an overview of the options supported by the used format or see
>     the format descriptions below for details.
>
> or:
>
> -b, --blacklist=list
>
>     Comma-separated list of RPCs to disable (no spaces, ‘?’ to list available RPCs)

These are bugs, and we need to fix them.

> So calling it deprecated sounds wrong to me.

Our intent to deprecate it was pretty clear in commit c8057f95:

    /**
     * is_help_option:
     * @s: string to test
     *
     * Check whether @s is one of the standard strings which indicate
     * that the user is asking for a list of the valid values for a
     * command option like -cpu or -M. The current accepted strings
-->  * are 'help' and '?'. '?' is deprecated (it is a shell wildcard
     * which makes it annoying to use in a reliable way) but provided
     * for backwards compatibility.
     *
     * Returns: true if @s is a request for a list.
     */
    static inline bool is_help_option(const char *s)

Predates today's more formal deprecation policy.  Simpler times.

There's no real need to kill off '?', unless it gets in the way of
steering people towards 'help'.  We should steer them toward 'help',
because '?' is a trap for insufficiently sophisticated users of
shell[*].

Every use of '?' in our source tree works against that goal, and thus
works towards a removal of '?'.



[*] In the wider sense, approximately 99.99% of shell users are
insufficiently sophisticated, including myself.  In the narrower sense
of being prone to fall into the '?' trap, the fraction is lower, but
still significant.

Re: [Qemu-devel] [PATCH 03/17] iotests: ask qemu for supported formats

[Thomas Huth]

On 07.06.2018 08:57, Markus Armbruster wrote:
> Thomas Huth <thuth@redhat.com> writes:
> 
>> On 05.06.2018 00:40, Eric Blake wrote:
>>> On 06/04/2018 05:34 AM, Thomas Huth wrote:
>>>> On 04.06.2018 09:18, Markus Armbruster wrote:
>>>>> Roman Kagan <rkagan@virtuozzo.com> writes:
>>>>>
>>>>>> Add helper functions to query the block drivers actually supported by
>>>>>> QEMU using "-drive format=?".  This allows to skip certain tests that
>>>>>> require drivers not built in or whitelisted in QEMU.
>>>>>>
>>>
>>>>>> +    supported_formats=$($QEMU_PROG $QEMU_OPTIONS -drive format=\?
>>>>>> 2>&1 | \
>>>>>
>>>>> Use of '?' to get help is deprecated.  Please use 'format=help', and
>>>>> update your commit message accordingly.
>>>>
>>>> Is it? Where did we document that?
>>>
>>> Hmm, we haven't documented it yet, but it's been that way since commit
>>> c8057f95, in Aug 2012, when we added 'help' as the preferred synonym,
>>> and have tried to avoid mention of '?' in new documentation (as it
>>> requires additional shell quoting).  I'm guessing we'll probably see a
>>> patch from you to start an official deprecation window?
>>
>> I'm using '?' regularly on my own, so don't expect a patch from
>> my side here ;-)
>> Anyway, we still use the question mark in our documentation, e.g.:
>>
>> options
>>
>>     is a comma separated list of format specific options in a name=value format.
>>     Use -o ? for an overview of the options supported by the used format or see
>>     the format descriptions below for details.
>>
>> or:
>>
>> -b, --blacklist=list
>>
>>     Comma-separated list of RPCs to disable (no spaces, ‘?’ to list available RPCs)
> 
> These are bugs, and we need to fix them.
> 
>> So calling it deprecated sounds wrong to me.
> 
> Our intent to deprecate it was pretty clear in commit c8057f95:
> 
>     /**
>      * is_help_option:
>      * @s: string to test
>      *
>      * Check whether @s is one of the standard strings which indicate
>      * that the user is asking for a list of the valid values for a
>      * command option like -cpu or -M. The current accepted strings
> -->  * are 'help' and '?'. '?' is deprecated (it is a shell wildcard
>      * which makes it annoying to use in a reliable way) but provided
>      * for backwards compatibility.
>      *
>      * Returns: true if @s is a request for a list.
>      */
>     static inline bool is_help_option(const char *s)
> 
> Predates today's more formal deprecation policy.  Simpler times.

Sure, but finding such messages in the sources can be quite cumbersome...

> There's no real need to kill off '?', unless it gets in the way of
> steering people towards 'help'.  We should steer them toward 'help',
> because '?' is a trap for insufficiently sophisticated users of
> shell[*].

... and I agree with your points here.

=> I think we need a second list of deprecated feature (maybe we should
call them "legacy features" instead of "deprecated"?), i.e. a list of
features which we don't recommend for new code / scripts anymore, but
which we do not intend to remove via our official deprecation policy any
time soon. Things like "--enable-kvm" / "-no-kvm" or maybe even "-net"
go into the same category.

If you agree, I can try to come up with a patch (should the list go into
qemu-doc.texi or a separate document in the the documentation folder?).

 Thomas

Re: [Qemu-devel] [PATCH 03/17] iotests: ask qemu for supported formats

[Paolo Bonzini]

On 07/06/2018 09:50, Thomas Huth wrote:
> 
>> There's no real need to kill off '?', unless it gets in the way of
>> steering people towards 'help'.  We should steer them toward 'help',
>> because '?' is a trap for insufficiently sophisticated users of
>> shell[*].
> ... and I agree with your points here.
> 
> => I think we need a second list of deprecated feature (maybe we should
> call them "legacy features" instead of "deprecated"?), i.e. a list of
> features which we don't recommend for new code / scripts anymore, but
> which we do not intend to remove via our official deprecation policy any
> time soon. Things like "--enable-kvm" / "-no-kvm" or maybe even "-net"
> go into the same category.

Yes, "-net" definitely goes there.

> If you agree, I can try to come up with a patch (should the list go into
> qemu-doc.texi or a separate document in the the documentation folder?).

I think it should go in docs/devel.

Paolo

Re: [Qemu-devel] [PATCH 03/17] iotests: ask qemu for supported formats

[Thomas Huth]

On 07.06.2018 13:07, Paolo Bonzini wrote:
> On 07/06/2018 09:50, Thomas Huth wrote:
>>
>>> There's no real need to kill off '?', unless it gets in the way of
>>> steering people towards 'help'.  We should steer them toward 'help',
>>> because '?' is a trap for insufficiently sophisticated users of
>>> shell[*].
>> ... and I agree with your points here.
>>
>> => I think we need a second list of deprecated feature (maybe we should
>> call them "legacy features" instead of "deprecated"?), i.e. a list of
>> features which we don't recommend for new code / scripts anymore, but
>> which we do not intend to remove via our official deprecation policy any
>> time soon. Things like "--enable-kvm" / "-no-kvm" or maybe even "-net"
>> go into the same category.
> 
> Yes, "-net" definitely goes there.
> 
>> If you agree, I can try to come up with a patch (should the list go into
>> qemu-doc.texi or a separate document in the the documentation folder?).
> 
> I think it should go in docs/devel.

I currently rather tend to put it into a new appendix in qemu-doc.texi,
since this is useful information for the normal users, too. Or how shall
we communicate this to the users that the old options that they are used
to are still there, but should not be used for new scripts anymore?

 Thomas

Re: [Qemu-devel] [PATCH 03/17] iotests: ask qemu for supported formats

[Paolo Bonzini]

On 07/06/2018 13:10, Thomas Huth wrote:
> On 07.06.2018 13:07, Paolo Bonzini wrote:
>> On 07/06/2018 09:50, Thomas Huth wrote:
>>>
>>>> There's no real need to kill off '?', unless it gets in the way of
>>>> steering people towards 'help'.  We should steer them toward 'help',
>>>> because '?' is a trap for insufficiently sophisticated users of
>>>> shell[*].
>>> ... and I agree with your points here.
>>>
>>> => I think we need a second list of deprecated feature (maybe we should
>>> call them "legacy features" instead of "deprecated"?), i.e. a list of
>>> features which we don't recommend for new code / scripts anymore, but
>>> which we do not intend to remove via our official deprecation policy any
>>> time soon. Things like "--enable-kvm" / "-no-kvm" or maybe even "-net"
>>> go into the same category.
>>
>> Yes, "-net" definitely goes there.
>>
>>> If you agree, I can try to come up with a patch (should the list go into
>>> qemu-doc.texi or a separate document in the the documentation folder?).
>>
>> I think it should go in docs/devel.
> 
> I currently rather tend to put it into a new appendix in qemu-doc.texi,
> since this is useful information for the normal users, too. Or how shall
> we communicate this to the users that the old options that they are used
> to are still there, but should not be used for new scripts anymore?

I think we should tell them directly in the text for things like "-net".
 The new document would put things together and be mostly about
command-line (anti)patterns.

As to "-enable-kvm", I don't see anything wrong with users using it, or
even with occasionally adding more options like it.  However, we should
warn developers that such simple options should be syntactic sugar for a
structured (i.e. QemuOpts-based) option like "-accel", and that it
should only be done for similarity with existing options.  Basically the
same reason why new options have both "?" and "help", even though "?" is
disliked.

Paolo

[Qemu-devel] -enable-kvm and friens (was: Re: [PATCH 03/17] iotests: ask qemu for supported formats)

[Thomas Huth]

On 07.06.2018 13:18, Paolo Bonzini wrote:
[...]
> As to "-enable-kvm", I don't see anything wrong with users using it, or
> even with occasionally adding more options like it.  However, we should
> warn developers that such simple options should be syntactic sugar for a
> structured (i.e. QemuOpts-based) option like "-accel", and that it
> should only be done for similarity with existing options.
Honestly, in this case I think it's just confusing for the normal users,
and not sugar (anymore). If I'm an unexperienced user who wants to
enable KVM, and I see multiple options that seem to be related, I wonder
whether they do the same or whether there's a difference, and which one
is preferred. And "-accel kvm" is even less to type than "-enable-kvm",
so there is really no advantage for "-enable-kvm" anymore. I think we
should remove "-enable-kvm" and "-enable-hax" from qemu-doc.texi and
only list it in the new legacy chapter / document.

 Thomas

Re: [Qemu-devel] [PATCH 03/17] iotests: ask qemu for supported formats

[Markus Armbruster]

Thomas Huth <thuth@redhat.com> writes:

> On 07.06.2018 08:57, Markus Armbruster wrote:
>> Thomas Huth <thuth@redhat.com> writes:
>> 
>>> On 05.06.2018 00:40, Eric Blake wrote:
>>>> On 06/04/2018 05:34 AM, Thomas Huth wrote:
>>>>> On 04.06.2018 09:18, Markus Armbruster wrote:
>>>>>> Roman Kagan <rkagan@virtuozzo.com> writes:
>>>>>>
>>>>>>> Add helper functions to query the block drivers actually supported by
>>>>>>> QEMU using "-drive format=?".  This allows to skip certain tests that
>>>>>>> require drivers not built in or whitelisted in QEMU.
>>>>>>>
>>>>
>>>>>>> +    supported_formats=$($QEMU_PROG $QEMU_OPTIONS -drive format=\?
>>>>>>> 2>&1 | \
>>>>>>
>>>>>> Use of '?' to get help is deprecated.  Please use 'format=help', and
>>>>>> update your commit message accordingly.
>>>>>
>>>>> Is it? Where did we document that?
>>>>
>>>> Hmm, we haven't documented it yet, but it's been that way since commit
>>>> c8057f95, in Aug 2012, when we added 'help' as the preferred synonym,
>>>> and have tried to avoid mention of '?' in new documentation (as it
>>>> requires additional shell quoting).  I'm guessing we'll probably see a
>>>> patch from you to start an official deprecation window?
>>>
>>> I'm using '?' regularly on my own, so don't expect a patch from
>>> my side here ;-)
>>> Anyway, we still use the question mark in our documentation, e.g.:
>>>
>>> options
>>>
>>>     is a comma separated list of format specific options in a name=value format.
>>>     Use -o ? for an overview of the options supported by the used format or see
>>>     the format descriptions below for details.
>>>
>>> or:
>>>
>>> -b, --blacklist=list
>>>
>>>     Comma-separated list of RPCs to disable (no spaces, ‘?’ to list available RPCs)
>> 
>> These are bugs, and we need to fix them.
>> 
>>> So calling it deprecated sounds wrong to me.
>> 
>> Our intent to deprecate it was pretty clear in commit c8057f95:
>> 
>>     /**
>>      * is_help_option:
>>      * @s: string to test
>>      *
>>      * Check whether @s is one of the standard strings which indicate
>>      * that the user is asking for a list of the valid values for a
>>      * command option like -cpu or -M. The current accepted strings
>> -->  * are 'help' and '?'. '?' is deprecated (it is a shell wildcard
>>      * which makes it annoying to use in a reliable way) but provided
>>      * for backwards compatibility.
>>      *
>>      * Returns: true if @s is a request for a list.
>>      */
>>     static inline bool is_help_option(const char *s)
>> 
>> Predates today's more formal deprecation policy.  Simpler times.
>
> Sure, but finding such messages in the sources can be quite cumbersome...
>
>> There's no real need to kill off '?', unless it gets in the way of
>> steering people towards 'help'.  We should steer them toward 'help',
>> because '?' is a trap for insufficiently sophisticated users of
>> shell[*].
>
> ... and I agree with your points here.
>
> => I think we need a second list of deprecated feature (maybe we should
> call them "legacy features" instead of "deprecated"?), i.e. a list of
> features which we don't recommend for new code / scripts anymore, but
> which we do not intend to remove via our official deprecation policy any
> time soon. Things like "--enable-kvm" / "-no-kvm" or maybe even "-net"
> go into the same category.
>
> If you agree, I can try to come up with a patch (should the list go into
> qemu-doc.texi or a separate document in the the documentation folder?).

I'm not sure it's worthwhile.

The boundary between "deprecated with intent to remove" and "deprecated
without such intent" is going to be a fuzzy one.  Could it be a useful
one anyway?

Formal deprecation is largely for the benefit of people writing software
that interfaces with QEMU.  They really need to know in advance, and
they are well advised to treat either kind of deprecation as "should
move to the replacement interface in an orderly manner".

If you use QEMU manually, it's easier to just keep using stuff until
it's gone, then look for the replacement.

Re: [Qemu-devel] -enable-kvm and friens (was: Re: [PATCH 03/17] iotests: ask qemu for supported formats)

[Paolo Bonzini]

On 07/06/2018 13:27, Thomas Huth wrote:
>> As to "-enable-kvm", I don't see anything wrong with users using it, or
>> even with occasionally adding more options like it.  However, we should
>> warn developers that such simple options should be syntactic sugar for a
>> structured (i.e. QemuOpts-based) option like "-accel", and that it
>> should only be done for similarity with existing options.
> Honestly, in this case I think it's just confusing for the normal users,
> and not sugar (anymore). If I'm an unexperienced user who wants to
> enable KVM, and I see multiple options that seem to be related, I wonder
> whether they do the same or whether there's a difference, and which one
> is preferred. And "-accel kvm" is even less to type than "-enable-kvm",
> so there is really no advantage for "-enable-kvm" anymore. I think we
> should remove "-enable-kvm" and "-enable-hax" from qemu-doc.texi and
> only list it in the new legacy chapter / document.

Well, there's also the issue of distros shipping qemu-kvm binaries.  I
think those should be provided by upstream.  If we do that, then we're
perhaps in a better position to place --enable-kvm under the rug.

Paolo

Re: [Qemu-devel] -enable-kvm and friens

[Thomas Huth]

On 07.06.2018 13:42, Paolo Bonzini wrote:
> On 07/06/2018 13:27, Thomas Huth wrote:
>>> As to "-enable-kvm", I don't see anything wrong with users using it, or
>>> even with occasionally adding more options like it.  However, we should
>>> warn developers that such simple options should be syntactic sugar for a
>>> structured (i.e. QemuOpts-based) option like "-accel", and that it
>>> should only be done for similarity with existing options.
>> Honestly, in this case I think it's just confusing for the normal users,
>> and not sugar (anymore). If I'm an unexperienced user who wants to
>> enable KVM, and I see multiple options that seem to be related, I wonder
>> whether they do the same or whether there's a difference, and which one
>> is preferred. And "-accel kvm" is even less to type than "-enable-kvm",
>> so there is really no advantage for "-enable-kvm" anymore. I think we
>> should remove "-enable-kvm" and "-enable-hax" from qemu-doc.texi and
>> only list it in the new legacy chapter / document.
> 
> Well, there's also the issue of distros shipping qemu-kvm binaries.  I
> think those should be provided by upstream.  If we do that, then we're
> perhaps in a better position to place --enable-kvm under the rug.

You don't need -enable-kvm for those qemu-kvm binaries, only -no-kvm.
And -no-kvm has never been documented in our qemu-doc user documentation
anyway (apart from the fact that it is now mentioned in the deprecation
chapter). So if we'd now mentioned -no-kvm in a "legacy feature" chapter
instead of the deprecation chapter, that would even improve the
situation for downstream qemu-kvms :-)

 Thomas

Re: [Qemu-devel] [PATCH 03/17] iotests: ask qemu for supported formats

[Daniel P. Berrangé]

On Thu, Jun 07, 2018 at 09:50:41AM +0200, Thomas Huth wrote:
> On 07.06.2018 08:57, Markus Armbruster wrote:
> > Thomas Huth <thuth@redhat.com> writes:
> > 
> >> On 05.06.2018 00:40, Eric Blake wrote:
> >>> On 06/04/2018 05:34 AM, Thomas Huth wrote:
> >>>> On 04.06.2018 09:18, Markus Armbruster wrote:
> >>>>> Roman Kagan <rkagan@virtuozzo.com> writes:
> >>>>>
> >>>>>> Add helper functions to query the block drivers actually supported by
> >>>>>> QEMU using "-drive format=?".  This allows to skip certain tests that
> >>>>>> require drivers not built in or whitelisted in QEMU.
> >>>>>>
> >>>
> >>>>>> +    supported_formats=$($QEMU_PROG $QEMU_OPTIONS -drive format=\?
> >>>>>> 2>&1 | \
> >>>>>
> >>>>> Use of '?' to get help is deprecated.  Please use 'format=help', and
> >>>>> update your commit message accordingly.
> >>>>
> >>>> Is it? Where did we document that?
> >>>
> >>> Hmm, we haven't documented it yet, but it's been that way since commit
> >>> c8057f95, in Aug 2012, when we added 'help' as the preferred synonym,
> >>> and have tried to avoid mention of '?' in new documentation (as it
> >>> requires additional shell quoting).  I'm guessing we'll probably see a
> >>> patch from you to start an official deprecation window?
> >>
> >> I'm using '?' regularly on my own, so don't expect a patch from
> >> my side here ;-)
> >> Anyway, we still use the question mark in our documentation, e.g.:
> >>
> >> options
> >>
> >>     is a comma separated list of format specific options in a name=value format.
> >>     Use -o ? for an overview of the options supported by the used format or see
> >>     the format descriptions below for details.
> >>
> >> or:
> >>
> >> -b, --blacklist=list
> >>
> >>     Comma-separated list of RPCs to disable (no spaces, ‘?’ to list available RPCs)
> > 
> > These are bugs, and we need to fix them.
> > 
> >> So calling it deprecated sounds wrong to me.
> > 
> > Our intent to deprecate it was pretty clear in commit c8057f95:
> > 
> >     /**
> >      * is_help_option:
> >      * @s: string to test
> >      *
> >      * Check whether @s is one of the standard strings which indicate
> >      * that the user is asking for a list of the valid values for a
> >      * command option like -cpu or -M. The current accepted strings
> > -->  * are 'help' and '?'. '?' is deprecated (it is a shell wildcard
> >      * which makes it annoying to use in a reliable way) but provided
> >      * for backwards compatibility.
> >      *
> >      * Returns: true if @s is a request for a list.
> >      */
> >     static inline bool is_help_option(const char *s)
> > 
> > Predates today's more formal deprecation policy.  Simpler times.
> 
> Sure, but finding such messages in the sources can be quite cumbersome...
> 
> > There's no real need to kill off '?', unless it gets in the way of
> > steering people towards 'help'.  We should steer them toward 'help',
> > because '?' is a trap for insufficiently sophisticated users of
> > shell[*].
> 
> ... and I agree with your points here.
> 
> => I think we need a second list of deprecated feature (maybe we should
> call them "legacy features" instead of "deprecated"?), i.e. a list of
> features which we don't recommend for new code / scripts anymore, but
> which we do not intend to remove via our official deprecation policy any
> time soon. Things like "--enable-kvm" / "-no-kvm" or maybe even "-net"
> go into the same category.

I don't see much point to be honest. If --enable-kvm works for the user
and we're not intending to removal it, I don't see why they should care
about using something else instead.  The other thing might have a more
flexible syntax, but if they don't need those extra features it really
doesn't matter. IOW, I think it is sufficient to just document all the
options that exist, and when documenting them simply make a note inline
that a particular option is merely  syntax suger for an alternative
option.

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 :|