[Qemu-devel] [PATCH 0/7] qemu-img: Improve option help for amend

[Qemu-devel] [PATCH 0/7] qemu-img: Improve option help for amend

[Max Reitz]

Currently, "qemu-img amend -f $format -o help" prints many things which
it claims to be supported, but most of the time it's wrong.  Usually
that starts with the format already: No format but qcow2 supports option
amendment, so we should never claim that a format supports any options
when it actually does not support amendment in the first place.

It goes on with the options themselves.  The qcow2 driver does not
support amendment of all creation options, so we should not claim it
does.  Actually knowing which formats are supported exactly would be a
bit difficult (this would probably involve adding a field to
QemuOptDesc, and I don't really want to do that), but what we can do
instead is to at least advise the user that the options we print are all
of the creation options and that we might not support amending all of
them.

There is a Bugzilla for this here:
https://bugzilla.redhat.com/show_bug.cgi?id=1537956

On the way to address these issues, there is more to be done, though.
bdrv_amend_options() does not have an Error parameter yet, and there is
no real excuse for that.

Also, "qemu-img create -o help" has its own little issue with formats
that do not support creation:

    $ qemu-img create -f bochs -o help
    Supported options:
    qemu-img: util/qemu-option.c:219:
    qemu_opts_print_help: Assertion `list' failed.
    [1]    24831 abort (core dumped)  qemu-img create -f bochs -o help

Let's fix that, too.


Max Reitz (7):
  qemu-img: Amendment support implies create_opts
  block: Add Error parameter to bdrv_amend_options
  qemu-option: Pull out "Supported options" print
  qemu-img: Add print_amend_option_help()
  qemu-img: Recognize no creation support in -o help
  iotests: Test help option for unsupporting formats
  iotests: Rework 113

 include/block/block.h      |  3 +-
 include/block/block_int.h  |  3 +-
 block.c                    |  8 ++++--
 block/qcow2.c              | 72 ++++++++++++++++++++++++++++++----------------
 qemu-img.c                 | 52 +++++++++++++++++++++++++++++----
 util/qemu-option.c         |  1 -
 tests/qemu-iotests/060.out |  4 +--
 tests/qemu-iotests/061.out |  7 -----
 tests/qemu-iotests/080.out |  4 +--
 tests/qemu-iotests/082     |  9 ++++++
 tests/qemu-iotests/082.out | 53 +++++++++++++++++++++++-----------
 tests/qemu-iotests/112.out |  3 --
 tests/qemu-iotests/113     | 19 ++++++------
 tests/qemu-iotests/113.out |  7 +++--
 14 files changed, 166 insertions(+), 79 deletions(-)

-- 
2.14.3

[Qemu-devel] [PATCH 1/7] qemu-img: Amendment support implies create_opts

[Max Reitz]

Instead of checking whether a driver has a non-NULL create_opts we
should check whether it supports image amendment in the first place.  If
it does, it must have create_opts.

On the other hand, if it does not have create_opts (so it does not
support amendment either), the error message "does not support any
options" is a bit useless.  Stating clearly that the driver has no
amendment support whatsoever is probably better.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 qemu-img.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/qemu-img.c b/qemu-img.c
index 855fa52514..f60b22769e 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -3709,13 +3709,16 @@ static int img_amend(int argc, char **argv)
         goto out;
     }
 
-    if (!bs->drv->create_opts) {
-        error_report("Format driver '%s' does not support any options to amend",
+    if (!bs->drv->bdrv_amend_options) {
+        error_report("Format driver '%s' does not support option amendment",
                      fmt);
         ret = -1;
         goto out;
     }
 
+    /* Every driver supporting amendment must have create_opts */
+    assert(bs->drv->create_opts);
+
     create_opts = qemu_opts_append(create_opts, bs->drv->create_opts);
     opts = qemu_opts_create(create_opts, NULL, 0, &error_abort);
     qemu_opts_do_parse(opts, options, NULL, &err);
-- 
2.14.3

Re: [Qemu-devel] [PATCH 1/7] qemu-img: Amendment support implies create_opts

[Eric Blake]

On 04/21/2018 11:54 AM, Max Reitz wrote:
> Instead of checking whether a driver has a non-NULL create_opts we
> should check whether it supports image amendment in the first place.  If
> it does, it must have create_opts.
> 
> On the other hand, if it does not have create_opts (so it does not
> support amendment either), the error message "does not support any
> options" is a bit useless.  Stating clearly that the driver has no
> amendment support whatsoever is probably better.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>   qemu-img.c | 7 +++++--
>   1 file changed, 5 insertions(+), 2 deletions(-)
> 

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

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

[Qemu-devel] [PATCH 2/7] block: Add Error parameter to bdrv_amend_options

[Max Reitz]

Looking at the qcow2 code that is riddled with error_report() calls,
this is really how it should have been from the start.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 include/block/block.h      |  3 +-
 include/block/block_int.h  |  3 +-
 block.c                    |  8 ++++--
 block/qcow2.c              | 72 ++++++++++++++++++++++++++++++----------------
 qemu-img.c                 |  4 +--
 tests/qemu-iotests/060.out |  4 +--
 tests/qemu-iotests/061.out |  7 -----
 tests/qemu-iotests/080.out |  4 +--
 tests/qemu-iotests/112.out |  3 --
 9 files changed, 64 insertions(+), 44 deletions(-)

diff --git a/include/block/block.h b/include/block/block.h
index cdec3639a3..ba9cfec384 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -336,7 +336,8 @@ int bdrv_check(BlockDriverState *bs, BdrvCheckResult *res, BdrvCheckMode fix);
 typedef void BlockDriverAmendStatusCB(BlockDriverState *bs, int64_t offset,
                                       int64_t total_work_size, void *opaque);
 int bdrv_amend_options(BlockDriverState *bs_new, QemuOpts *opts,
-                       BlockDriverAmendStatusCB *status_cb, void *cb_opaque);
+                       BlockDriverAmendStatusCB *status_cb, void *cb_opaque,
+                       Error **errp);
 
 /* external snapshots */
 bool bdrv_recurse_is_first_non_filter(BlockDriverState *bs,
diff --git a/include/block/block_int.h b/include/block/block_int.h
index c4dd1d4bb8..d913ed1ea1 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -324,7 +324,8 @@ struct BlockDriver {
 
     int (*bdrv_amend_options)(BlockDriverState *bs, QemuOpts *opts,
                               BlockDriverAmendStatusCB *status_cb,
-                              void *cb_opaque);
+                              void *cb_opaque,
+                              Error **errp);
 
     void (*bdrv_debug_event)(BlockDriverState *bs, BlkdebugEvent event);
 
diff --git a/block.c b/block.c
index a2caadf0a0..fefe1109eb 100644
--- a/block.c
+++ b/block.c
@@ -4996,15 +4996,19 @@ void bdrv_remove_aio_context_notifier(BlockDriverState *bs,
 }
 
 int bdrv_amend_options(BlockDriverState *bs, QemuOpts *opts,
-                       BlockDriverAmendStatusCB *status_cb, void *cb_opaque)
+                       BlockDriverAmendStatusCB *status_cb, void *cb_opaque,
+                       Error **errp)
 {
     if (!bs->drv) {
+        error_setg(errp, "Node is ejected");
         return -ENOMEDIUM;
     }
     if (!bs->drv->bdrv_amend_options) {
+        error_setg(errp, "Block driver '%s' does not support option amendment",
+                   bs->drv->format_name);
         return -ENOTSUP;
     }
-    return bs->drv->bdrv_amend_options(bs, opts, status_cb, cb_opaque);
+    return bs->drv->bdrv_amend_options(bs, opts, status_cb, cb_opaque, errp);
 }
 
 /* This function will be called by the bdrv_recurse_is_first_non_filter method
diff --git a/block/qcow2.c b/block/qcow2.c
index 091088e09e..eb86ba0a2a 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -4049,7 +4049,8 @@ static int qcow2_load_vmstate(BlockDriverState *bs, QEMUIOVector *qiov,
  * have to be removed.
  */
 static int qcow2_downgrade(BlockDriverState *bs, int target_version,
-                           BlockDriverAmendStatusCB *status_cb, void *cb_opaque)
+                           BlockDriverAmendStatusCB *status_cb, void *cb_opaque,
+                           Error **errp)
 {
     BDRVQcow2State *s = bs->opaque;
     int current_version = s->qcow_version;
@@ -4058,13 +4059,17 @@ static int qcow2_downgrade(BlockDriverState *bs, int target_version,
     if (target_version == current_version) {
         return 0;
     } else if (target_version > current_version) {
+        error_setg(errp, "Cannot downgrade an image from version %i to %i",
+                   current_version, target_version);
         return -EINVAL;
     } else if (target_version != 2) {
+        error_setg(errp, "Cannot downgrade an image to version %i (only "
+                   "target version 2 is supported)", target_version);
         return -EINVAL;
     }
 
     if (s->refcount_order != 4) {
-        error_report("compat=0.10 requires refcount_bits=16");
+        error_setg(errp, "compat=0.10 requires refcount_bits=16");
         return -ENOTSUP;
     }
 
@@ -4072,6 +4077,8 @@ static int qcow2_downgrade(BlockDriverState *bs, int target_version,
     if (s->incompatible_features & QCOW2_INCOMPAT_DIRTY) {
         ret = qcow2_mark_clean(bs);
         if (ret < 0) {
+            error_setg(errp, "Failed to make the image clean: %s",
+                       strerror(-ret));
             return ret;
         }
     }
@@ -4081,6 +4088,8 @@ static int qcow2_downgrade(BlockDriverState *bs, int target_version,
      * best thing to do anyway */
 
     if (s->incompatible_features) {
+        error_setg(errp, "Cannot downgrade an image with incompatible features "
+                   "%#" PRIx64 " set", s->incompatible_features);
         return -ENOTSUP;
     }
 
@@ -4094,6 +4103,8 @@ static int qcow2_downgrade(BlockDriverState *bs, int target_version,
 
     ret = qcow2_expand_zero_clusters(bs, status_cb, cb_opaque);
     if (ret < 0) {
+        error_setg(errp, "Failed to turn zero into data clusters: %s",
+                   strerror(-ret));
         return ret;
     }
 
@@ -4101,6 +4112,8 @@ static int qcow2_downgrade(BlockDriverState *bs, int target_version,
     ret = qcow2_update_header(bs);
     if (ret < 0) {
         s->qcow_version = current_version;
+        error_setg(errp, "Failed to update the image header: %s",
+                   strerror(-ret));
         return ret;
     }
     return 0;
@@ -4178,7 +4191,8 @@ static void qcow2_amend_helper_cb(BlockDriverState *bs,
 
 static int qcow2_amend_options(BlockDriverState *bs, QemuOpts *opts,
                                BlockDriverAmendStatusCB *status_cb,
-                               void *cb_opaque)
+                               void *cb_opaque,
+                               Error **errp)
 {
     BDRVQcow2State *s = bs->opaque;
     int old_version = s->qcow_version, new_version = old_version;
@@ -4190,7 +4204,6 @@ static int qcow2_amend_options(BlockDriverState *bs, QemuOpts *opts,
     bool encrypt;
     int encformat;
     int refcount_bits = s->refcount_bits;
-    Error *local_err = NULL;
     int ret;
     QemuOptDesc *desc = opts->list->desc;
     Qcow2AmendHelperCBInfo helper_cb_info;
@@ -4211,11 +4224,11 @@ static int qcow2_amend_options(BlockDriverState *bs, QemuOpts *opts,
             } else if (!strcmp(compat, "1.1")) {
                 new_version = 3;
             } else {
-                error_report("Unknown compatibility level %s", compat);
+                error_setg(errp, "Unknown compatibility level %s", compat);
                 return -EINVAL;
             }
         } else if (!strcmp(desc->name, BLOCK_OPT_PREALLOC)) {
-            error_report("Cannot change preallocation mode");
+            error_setg(errp, "Cannot change preallocation mode");
             return -ENOTSUP;
         } else if (!strcmp(desc->name, BLOCK_OPT_SIZE)) {
             new_size = qemu_opt_get_size(opts, BLOCK_OPT_SIZE, 0);
@@ -4228,7 +4241,8 @@ static int qcow2_amend_options(BlockDriverState *bs, QemuOpts *opts,
                                         !!s->crypto);
 
             if (encrypt != !!s->crypto) {
-                error_report("Changing the encryption flag is not supported");
+                error_setg(errp,
+                           "Changing the encryption flag is not supported");
                 return -ENOTSUP;
             }
         } else if (!strcmp(desc->name, BLOCK_OPT_ENCRYPT_FORMAT)) {
@@ -4236,17 +4250,19 @@ static int qcow2_amend_options(BlockDriverState *bs, QemuOpts *opts,
                 qemu_opt_get(opts, BLOCK_OPT_ENCRYPT_FORMAT));
 
             if (encformat != s->crypt_method_header) {
-                error_report("Changing the encryption format is not supported");
+                error_setg(errp,
+                           "Changing the encryption format is not supported");
                 return -ENOTSUP;
             }
         } else if (g_str_has_prefix(desc->name, "encrypt.")) {
-            error_report("Changing the encryption parameters is not supported");
+            error_setg(errp,
+                       "Changing the encryption parameters is not supported");
             return -ENOTSUP;
         } else if (!strcmp(desc->name, BLOCK_OPT_CLUSTER_SIZE)) {
             cluster_size = qemu_opt_get_size(opts, BLOCK_OPT_CLUSTER_SIZE,
                                              cluster_size);
             if (cluster_size != s->cluster_size) {
-                error_report("Changing the cluster size is not supported");
+                error_setg(errp, "Changing the cluster size is not supported");
                 return -ENOTSUP;
             }
         } else if (!strcmp(desc->name, BLOCK_OPT_LAZY_REFCOUNTS)) {
@@ -4259,8 +4275,8 @@ static int qcow2_amend_options(BlockDriverState *bs, QemuOpts *opts,
             if (refcount_bits <= 0 || refcount_bits > 64 ||
                 !is_power_of_2(refcount_bits))
             {
-                error_report("Refcount width must be a power of two and may "
-                             "not exceed 64 bits");
+                error_setg(errp, "Refcount width must be a power of two and "
+                           "may not exceed 64 bits");
                 return -EINVAL;
             }
         } else {
@@ -4285,6 +4301,8 @@ static int qcow2_amend_options(BlockDriverState *bs, QemuOpts *opts,
         ret = qcow2_update_header(bs);
         if (ret < 0) {
             s->qcow_version = old_version;
+            error_setg(errp, "Failed to update the image header: %s",
+                       strerror(-ret));
             return ret;
         }
     }
@@ -4293,18 +4311,17 @@ static int qcow2_amend_options(BlockDriverState *bs, QemuOpts *opts,
         int refcount_order = ctz32(refcount_bits);
 
         if (new_version < 3 && refcount_bits != 16) {
-            error_report("Different refcount widths than 16 bits require "
-                         "compatibility level 1.1 or above (use compat=1.1 or "
-                         "greater)");
+            error_setg(errp, "Different refcount widths than 16 bits require "
+                       "compatibility level 1.1 or above (use compat=1.1 or "
+                       "greater)");
             return -EINVAL;
         }
 
         helper_cb_info.current_operation = QCOW2_CHANGING_REFCOUNT_ORDER;
         ret = qcow2_change_refcount_order(bs, refcount_order,
                                           &qcow2_amend_helper_cb,
-                                          &helper_cb_info, &local_err);
+                                          &helper_cb_info, errp);
         if (ret < 0) {
-            error_report_err(local_err);
             return ret;
         }
     }
@@ -4314,6 +4331,8 @@ static int qcow2_amend_options(BlockDriverState *bs, QemuOpts *opts,
                     backing_file ?: s->image_backing_file,
                     backing_format ?: s->image_backing_format);
         if (ret < 0) {
+            error_setg(errp, "Failed to change the backing file: %s",
+                       strerror(-ret));
             return ret;
         }
     }
@@ -4321,14 +4340,17 @@ static int qcow2_amend_options(BlockDriverState *bs, QemuOpts *opts,
     if (s->use_lazy_refcounts != lazy_refcounts) {
         if (lazy_refcounts) {
             if (new_version < 3) {
-                error_report("Lazy refcounts only supported with compatibility "
-                             "level 1.1 and above (use compat=1.1 or greater)");
+                error_setg(errp, "Lazy refcounts only supported with "
+                           "compatibility level 1.1 and above (use compat=1.1 "
+                           "or greater)");
                 return -EINVAL;
             }
             s->compatible_features |= QCOW2_COMPAT_LAZY_REFCOUNTS;
             ret = qcow2_update_header(bs);
             if (ret < 0) {
                 s->compatible_features &= ~QCOW2_COMPAT_LAZY_REFCOUNTS;
+                error_setg(errp, "Failed to update the image header: %s",
+                           strerror(-ret));
                 return ret;
             }
             s->use_lazy_refcounts = true;
@@ -4336,6 +4358,8 @@ static int qcow2_amend_options(BlockDriverState *bs, QemuOpts *opts,
             /* make image clean first */
             ret = qcow2_mark_clean(bs);
             if (ret < 0) {
+                error_setg(errp, "Failed to make the image clean: %s",
+                           strerror(-ret));
                 return ret;
             }
             /* now disallow lazy refcounts */
@@ -4343,6 +4367,8 @@ static int qcow2_amend_options(BlockDriverState *bs, QemuOpts *opts,
             ret = qcow2_update_header(bs);
             if (ret < 0) {
                 s->compatible_features |= QCOW2_COMPAT_LAZY_REFCOUNTS;
+                error_setg(errp, "Failed to update the image header: %s",
+                           strerror(-ret));
                 return ret;
             }
             s->use_lazy_refcounts = false;
@@ -4351,17 +4377,15 @@ static int qcow2_amend_options(BlockDriverState *bs, QemuOpts *opts,
 
     if (new_size) {
         BlockBackend *blk = blk_new(BLK_PERM_RESIZE, BLK_PERM_ALL);
-        ret = blk_insert_bs(blk, bs, &local_err);
+        ret = blk_insert_bs(blk, bs, errp);
         if (ret < 0) {
-            error_report_err(local_err);
             blk_unref(blk);
             return ret;
         }
 
-        ret = blk_truncate(blk, new_size, PREALLOC_MODE_OFF, &local_err);
+        ret = blk_truncate(blk, new_size, PREALLOC_MODE_OFF, errp);
         blk_unref(blk);
         if (ret < 0) {
-            error_report_err(local_err);
             return ret;
         }
     }
@@ -4370,7 +4394,7 @@ static int qcow2_amend_options(BlockDriverState *bs, QemuOpts *opts,
     if (new_version < old_version) {
         helper_cb_info.current_operation = QCOW2_DOWNGRADING;
         ret = qcow2_downgrade(bs, new_version, &qcow2_amend_helper_cb,
-                              &helper_cb_info);
+                              &helper_cb_info, errp);
         if (ret < 0) {
             return ret;
         }
diff --git a/qemu-img.c b/qemu-img.c
index f60b22769e..375fe852e0 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -3730,10 +3730,10 @@ static int img_amend(int argc, char **argv)
 
     /* In case the driver does not call amend_status_cb() */
     qemu_progress_print(0.f, 0);
-    ret = bdrv_amend_options(bs, opts, &amend_status_cb, NULL);
+    ret = bdrv_amend_options(bs, opts, &amend_status_cb, NULL, &err);
     qemu_progress_print(100.f, 0);
     if (ret < 0) {
-        error_report("Error while amending options: %s", strerror(-ret));
+        error_report_err(err);
         goto out;
     }
 
diff --git a/tests/qemu-iotests/060.out b/tests/qemu-iotests/060.out
index 25d5c3938b..5f4264cff6 100644
--- a/tests/qemu-iotests/060.out
+++ b/tests/qemu-iotests/060.out
@@ -129,7 +129,7 @@ Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
 wrote 65536/65536 bytes at offset 0
 64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
 qcow2: Marking image as corrupt: L2 table offset 0x42a00 unaligned (L1 index: 0); further corruption events will be suppressed
-qemu-img: Error while amending options: Input/output error
+qemu-img: Failed to turn zero into data clusters: Input/output error
 
 === Testing unaligned L2 entry ===
 
@@ -145,7 +145,7 @@ Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
 wrote 65536/65536 bytes at offset 0
 64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
 qcow2: Marking image as corrupt: Cluster allocation offset 0x52a00 unaligned (L2 offset: 0x40000, L2 index: 0); further corruption events will be suppressed
-qemu-img: Error while amending options: Input/output error
+qemu-img: Failed to turn zero into data clusters: Input/output error
 
 === Testing unaligned reftable entry ===
 
diff --git a/tests/qemu-iotests/061.out b/tests/qemu-iotests/061.out
index e857ef9a7d..183f7dd690 100644
--- a/tests/qemu-iotests/061.out
+++ b/tests/qemu-iotests/061.out
@@ -358,18 +358,12 @@ No errors were found on the image.
 
 Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
 qemu-img: Lazy refcounts only supported with compatibility level 1.1 and above (use compat=1.1 or greater)
-qemu-img: Error while amending options: Invalid argument
 qemu-img: Lazy refcounts only supported with compatibility level 1.1 and above (use compat=1.1 or greater)
-qemu-img: Error while amending options: Invalid argument
 qemu-img: Unknown compatibility level 0.42
-qemu-img: Error while amending options: Invalid argument
 qemu-img: Invalid parameter 'foo'
 qemu-img: Changing the cluster size is not supported
-qemu-img: Error while amending options: Operation not supported
 qemu-img: Changing the encryption flag is not supported
-qemu-img: Error while amending options: Operation not supported
 qemu-img: Cannot change preallocation mode
-qemu-img: Error while amending options: Operation not supported
 
 === Testing correct handling of unset value ===
 
@@ -377,7 +371,6 @@ Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
 Should work:
 Should not work:
 qemu-img: Changing the cluster size is not supported
-qemu-img: Error while amending options: Operation not supported
 
 === Testing zero expansion on inactive clusters ===
 
diff --git a/tests/qemu-iotests/080.out b/tests/qemu-iotests/080.out
index 4e0f7f7b92..281c7e0d1d 100644
--- a/tests/qemu-iotests/080.out
+++ b/tests/qemu-iotests/080.out
@@ -65,7 +65,7 @@ wrote 512/512 bytes at offset 0
 512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
 qemu-img: Failed to load snapshot: Snapshot L1 table offset invalid
 qemu-img: Snapshot L1 table offset invalid
-qemu-img: Error while amending options: Invalid argument
+qemu-img: Failed to turn zero into data clusters: Invalid argument
 Failed to flush the refcount block cache: Invalid argument
 write failed: Invalid argument
 qemu-img: Snapshot L1 table offset invalid
@@ -88,7 +88,7 @@ wrote 512/512 bytes at offset 0
 512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
 qemu-img: Failed to load snapshot: Snapshot L1 table too large
 qemu-img: Snapshot L1 table too large
-qemu-img: Error while amending options: File too large
+qemu-img: Failed to turn zero into data clusters: File too large
 Failed to flush the refcount block cache: File too large
 write failed: File too large
 qemu-img: Snapshot L1 table too large
diff --git a/tests/qemu-iotests/112.out b/tests/qemu-iotests/112.out
index 86f041075d..a23aa1be75 100644
--- a/tests/qemu-iotests/112.out
+++ b/tests/qemu-iotests/112.out
@@ -99,13 +99,11 @@ refcount bits: 64
 === Amend to compat=0.10 ===
 
 qemu-img: compat=0.10 requires refcount_bits=16
-qemu-img: Error while amending options: Operation not supported
 refcount bits: 64
 No errors were found on the image.
 refcount bits: 16
 refcount bits: 16
 qemu-img: Different refcount widths than 16 bits require compatibility level 1.1 or above (use compat=1.1 or greater)
-qemu-img: Error while amending options: Invalid argument
 refcount bits: 16
 
 === Amend with snapshot ===
@@ -113,7 +111,6 @@ refcount bits: 16
 wrote 16777216/16777216 bytes at offset 0
 16 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
 qemu-img: Cannot decrease refcount entry width to 1 bits: Cluster at offset 0x50000 has a refcount of 2
-qemu-img: Error while amending options: Invalid argument
 No errors were found on the image.
 refcount bits: 16
 No errors were found on the image.
-- 
2.14.3

Re: [Qemu-devel] [Qemu-block] [PATCH 2/7] block: Add Error parameter to bdrv_amend_options

[John Snow]

On 04/21/2018 12:54 PM, Max Reitz wrote:
> Looking at the qcow2 code that is riddled with error_report() calls,
> this is really how it should have been from the start.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  include/block/block.h      |  3 +-
>  include/block/block_int.h  |  3 +-
>  block.c                    |  8 ++++--
>  block/qcow2.c              | 72 ++++++++++++++++++++++++++++++----------------
>  qemu-img.c                 |  4 +--
>  tests/qemu-iotests/060.out |  4 +--
>  tests/qemu-iotests/061.out |  7 -----
>  tests/qemu-iotests/080.out |  4 +--
>  tests/qemu-iotests/112.out |  3 --
>  9 files changed, 64 insertions(+), 44 deletions(-)
> 
> diff --git a/include/block/block.h b/include/block/block.h
> index cdec3639a3..ba9cfec384 100644
> --- a/include/block/block.h
> +++ b/include/block/block.h
> @@ -336,7 +336,8 @@ int bdrv_check(BlockDriverState *bs, BdrvCheckResult *res, BdrvCheckMode fix);
>  typedef void BlockDriverAmendStatusCB(BlockDriverState *bs, int64_t offset,
>                                        int64_t total_work_size, void *opaque);
>  int bdrv_amend_options(BlockDriverState *bs_new, QemuOpts *opts,
> -                       BlockDriverAmendStatusCB *status_cb, void *cb_opaque);
> +                       BlockDriverAmendStatusCB *status_cb, void *cb_opaque,
> +                       Error **errp);
>  
>  /* external snapshots */
>  bool bdrv_recurse_is_first_non_filter(BlockDriverState *bs,
> diff --git a/include/block/block_int.h b/include/block/block_int.h
> index c4dd1d4bb8..d913ed1ea1 100644
> --- a/include/block/block_int.h
> +++ b/include/block/block_int.h
> @@ -324,7 +324,8 @@ struct BlockDriver {
>  
>      int (*bdrv_amend_options)(BlockDriverState *bs, QemuOpts *opts,
>                                BlockDriverAmendStatusCB *status_cb,
> -                              void *cb_opaque);
> +                              void *cb_opaque,
> +                              Error **errp);
>  
>      void (*bdrv_debug_event)(BlockDriverState *bs, BlkdebugEvent event);
>  
> diff --git a/block.c b/block.c
> index a2caadf0a0..fefe1109eb 100644
> --- a/block.c
> +++ b/block.c
> @@ -4996,15 +4996,19 @@ void bdrv_remove_aio_context_notifier(BlockDriverState *bs,
>  }
>  
>  int bdrv_amend_options(BlockDriverState *bs, QemuOpts *opts,
> -                       BlockDriverAmendStatusCB *status_cb, void *cb_opaque)
> +                       BlockDriverAmendStatusCB *status_cb, void *cb_opaque,
> +                       Error **errp)
>  {
>      if (!bs->drv) {
> +        error_setg(errp, "Node is ejected");
>          return -ENOMEDIUM;
>      }
>      if (!bs->drv->bdrv_amend_options) {
> +        error_setg(errp, "Block driver '%s' does not support option amendment",
> +                   bs->drv->format_name);
>          return -ENOTSUP;
>      }
> -    return bs->drv->bdrv_amend_options(bs, opts, status_cb, cb_opaque);
> +    return bs->drv->bdrv_amend_options(bs, opts, status_cb, cb_opaque, errp);
>  }
>  
>  /* This function will be called by the bdrv_recurse_is_first_non_filter method
> diff --git a/block/qcow2.c b/block/qcow2.c
> index 091088e09e..eb86ba0a2a 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -4049,7 +4049,8 @@ static int qcow2_load_vmstate(BlockDriverState *bs, QEMUIOVector *qiov,
>   * have to be removed.
>   */
>  static int qcow2_downgrade(BlockDriverState *bs, int target_version,
> -                           BlockDriverAmendStatusCB *status_cb, void *cb_opaque)
> +                           BlockDriverAmendStatusCB *status_cb, void *cb_opaque,
> +                           Error **errp)
>  {
>      BDRVQcow2State *s = bs->opaque;
>      int current_version = s->qcow_version;
> @@ -4058,13 +4059,17 @@ static int qcow2_downgrade(BlockDriverState *bs, int target_version,
>      if (target_version == current_version) {
>          return 0;
>      } else if (target_version > current_version) {
> +        error_setg(errp, "Cannot downgrade an image from version %i to %i",
> +                   current_version, target_version);
>          return -EINVAL;
>      } else if (target_version != 2) {
> +        error_setg(errp, "Cannot downgrade an image to version %i (only "
> +                   "target version 2 is supported)", target_version);
>          return -EINVAL;
>      }
>  
>      if (s->refcount_order != 4) {
> -        error_report("compat=0.10 requires refcount_bits=16");
> +        error_setg(errp, "compat=0.10 requires refcount_bits=16");
>          return -ENOTSUP;
>      }
>  
> @@ -4072,6 +4077,8 @@ static int qcow2_downgrade(BlockDriverState *bs, int target_version,
>      if (s->incompatible_features & QCOW2_INCOMPAT_DIRTY) {
>          ret = qcow2_mark_clean(bs);
>          if (ret < 0) {
> +            error_setg(errp, "Failed to make the image clean: %s",
> +                       strerror(-ret));
>              return ret;
>          }
>      }
> @@ -4081,6 +4088,8 @@ static int qcow2_downgrade(BlockDriverState *bs, int target_version,
>       * best thing to do anyway */
>  
>      if (s->incompatible_features) {
> +        error_setg(errp, "Cannot downgrade an image with incompatible features "
> +                   "%#" PRIx64 " set", s->incompatible_features);
>          return -ENOTSUP;
>      }
>  
> @@ -4094,6 +4103,8 @@ static int qcow2_downgrade(BlockDriverState *bs, int target_version,
>  
>      ret = qcow2_expand_zero_clusters(bs, status_cb, cb_opaque);
>      if (ret < 0) {
> +        error_setg(errp, "Failed to turn zero into data clusters: %s",
> +                   strerror(-ret));
>          return ret;
>      }
>  
> @@ -4101,6 +4112,8 @@ static int qcow2_downgrade(BlockDriverState *bs, int target_version,
>      ret = qcow2_update_header(bs);
>      if (ret < 0) {
>          s->qcow_version = current_version;
> +        error_setg(errp, "Failed to update the image header: %s",
> +                   strerror(-ret));
>          return ret;
>      }
>      return 0;
> @@ -4178,7 +4191,8 @@ static void qcow2_amend_helper_cb(BlockDriverState *bs,
>  
>  static int qcow2_amend_options(BlockDriverState *bs, QemuOpts *opts,
>                                 BlockDriverAmendStatusCB *status_cb,
> -                               void *cb_opaque)
> +                               void *cb_opaque,
> +                               Error **errp)
>  {
>      BDRVQcow2State *s = bs->opaque;
>      int old_version = s->qcow_version, new_version = old_version;
> @@ -4190,7 +4204,6 @@ static int qcow2_amend_options(BlockDriverState *bs, QemuOpts *opts,
>      bool encrypt;
>      int encformat;
>      int refcount_bits = s->refcount_bits;
> -    Error *local_err = NULL;
>      int ret;
>      QemuOptDesc *desc = opts->list->desc;
>      Qcow2AmendHelperCBInfo helper_cb_info;
> @@ -4211,11 +4224,11 @@ static int qcow2_amend_options(BlockDriverState *bs, QemuOpts *opts,
>              } else if (!strcmp(compat, "1.1")) {
>                  new_version = 3;
>              } else {
> -                error_report("Unknown compatibility level %s", compat);
> +                error_setg(errp, "Unknown compatibility level %s", compat);
>                  return -EINVAL;
>              }
>          } else if (!strcmp(desc->name, BLOCK_OPT_PREALLOC)) {
> -            error_report("Cannot change preallocation mode");
> +            error_setg(errp, "Cannot change preallocation mode");
>              return -ENOTSUP;
>          } else if (!strcmp(desc->name, BLOCK_OPT_SIZE)) {
>              new_size = qemu_opt_get_size(opts, BLOCK_OPT_SIZE, 0);
> @@ -4228,7 +4241,8 @@ static int qcow2_amend_options(BlockDriverState *bs, QemuOpts *opts,
>                                          !!s->crypto);
>  
>              if (encrypt != !!s->crypto) {
> -                error_report("Changing the encryption flag is not supported");
> +                error_setg(errp,
> +                           "Changing the encryption flag is not supported");
>                  return -ENOTSUP;
>              }
>          } else if (!strcmp(desc->name, BLOCK_OPT_ENCRYPT_FORMAT)) {
> @@ -4236,17 +4250,19 @@ static int qcow2_amend_options(BlockDriverState *bs, QemuOpts *opts,
>                  qemu_opt_get(opts, BLOCK_OPT_ENCRYPT_FORMAT));
>  
>              if (encformat != s->crypt_method_header) {
> -                error_report("Changing the encryption format is not supported");
> +                error_setg(errp,
> +                           "Changing the encryption format is not supported");
>                  return -ENOTSUP;
>              }
>          } else if (g_str_has_prefix(desc->name, "encrypt.")) {
> -            error_report("Changing the encryption parameters is not supported");
> +            error_setg(errp,
> +                       "Changing the encryption parameters is not supported");
>              return -ENOTSUP;
>          } else if (!strcmp(desc->name, BLOCK_OPT_CLUSTER_SIZE)) {
>              cluster_size = qemu_opt_get_size(opts, BLOCK_OPT_CLUSTER_SIZE,
>                                               cluster_size);
>              if (cluster_size != s->cluster_size) {
> -                error_report("Changing the cluster size is not supported");
> +                error_setg(errp, "Changing the cluster size is not supported");
>                  return -ENOTSUP;
>              }
>          } else if (!strcmp(desc->name, BLOCK_OPT_LAZY_REFCOUNTS)) {
> @@ -4259,8 +4275,8 @@ static int qcow2_amend_options(BlockDriverState *bs, QemuOpts *opts,
>              if (refcount_bits <= 0 || refcount_bits > 64 ||
>                  !is_power_of_2(refcount_bits))
>              {
> -                error_report("Refcount width must be a power of two and may "
> -                             "not exceed 64 bits");
> +                error_setg(errp, "Refcount width must be a power of two and "
> +                           "may not exceed 64 bits");
>                  return -EINVAL;
>              }
>          } else {
> @@ -4285,6 +4301,8 @@ static int qcow2_amend_options(BlockDriverState *bs, QemuOpts *opts,
>          ret = qcow2_update_header(bs);
>          if (ret < 0) {
>              s->qcow_version = old_version;
> +            error_setg(errp, "Failed to update the image header: %s",
> +                       strerror(-ret));
>              return ret;
>          }
>      }
> @@ -4293,18 +4311,17 @@ static int qcow2_amend_options(BlockDriverState *bs, QemuOpts *opts,
>          int refcount_order = ctz32(refcount_bits);
>  
>          if (new_version < 3 && refcount_bits != 16) {
> -            error_report("Different refcount widths than 16 bits require "
> -                         "compatibility level 1.1 or above (use compat=1.1 or "
> -                         "greater)");
> +            error_setg(errp, "Different refcount widths than 16 bits require "
> +                       "compatibility level 1.1 or above (use compat=1.1 or "
> +                       "greater)");
>              return -EINVAL;
>          }
>  
>          helper_cb_info.current_operation = QCOW2_CHANGING_REFCOUNT_ORDER;
>          ret = qcow2_change_refcount_order(bs, refcount_order,
>                                            &qcow2_amend_helper_cb,
> -                                          &helper_cb_info, &local_err);
> +                                          &helper_cb_info, errp);
>          if (ret < 0) {
> -            error_report_err(local_err);
>              return ret;
>          }
>      }
> @@ -4314,6 +4331,8 @@ static int qcow2_amend_options(BlockDriverState *bs, QemuOpts *opts,
>                      backing_file ?: s->image_backing_file,
>                      backing_format ?: s->image_backing_format);
>          if (ret < 0) {
> +            error_setg(errp, "Failed to change the backing file: %s",
> +                       strerror(-ret));
>              return ret;
>          }
>      }
> @@ -4321,14 +4340,17 @@ static int qcow2_amend_options(BlockDriverState *bs, QemuOpts *opts,
>      if (s->use_lazy_refcounts != lazy_refcounts) {
>          if (lazy_refcounts) {
>              if (new_version < 3) {
> -                error_report("Lazy refcounts only supported with compatibility "
> -                             "level 1.1 and above (use compat=1.1 or greater)");
> +                error_setg(errp, "Lazy refcounts only supported with "
> +                           "compatibility level 1.1 and above (use compat=1.1 "
> +                           "or greater)");
>                  return -EINVAL;
>              }
>              s->compatible_features |= QCOW2_COMPAT_LAZY_REFCOUNTS;
>              ret = qcow2_update_header(bs);
>              if (ret < 0) {
>                  s->compatible_features &= ~QCOW2_COMPAT_LAZY_REFCOUNTS;
> +                error_setg(errp, "Failed to update the image header: %s",
> +                           strerror(-ret));
>                  return ret;
>              }
>              s->use_lazy_refcounts = true;
> @@ -4336,6 +4358,8 @@ static int qcow2_amend_options(BlockDriverState *bs, QemuOpts *opts,
>              /* make image clean first */
>              ret = qcow2_mark_clean(bs);
>              if (ret < 0) {
> +                error_setg(errp, "Failed to make the image clean: %s",
> +                           strerror(-ret));
>                  return ret;
>              }
>              /* now disallow lazy refcounts */
> @@ -4343,6 +4367,8 @@ static int qcow2_amend_options(BlockDriverState *bs, QemuOpts *opts,
>              ret = qcow2_update_header(bs);
>              if (ret < 0) {
>                  s->compatible_features |= QCOW2_COMPAT_LAZY_REFCOUNTS;
> +                error_setg(errp, "Failed to update the image header: %s",
> +                           strerror(-ret));
>                  return ret;
>              }
>              s->use_lazy_refcounts = false;
> @@ -4351,17 +4377,15 @@ static int qcow2_amend_options(BlockDriverState *bs, QemuOpts *opts,
>  
>      if (new_size) {
>          BlockBackend *blk = blk_new(BLK_PERM_RESIZE, BLK_PERM_ALL);
> -        ret = blk_insert_bs(blk, bs, &local_err);
> +        ret = blk_insert_bs(blk, bs, errp);
>          if (ret < 0) {
> -            error_report_err(local_err);
>              blk_unref(blk);
>              return ret;
>          }
>  
> -        ret = blk_truncate(blk, new_size, PREALLOC_MODE_OFF, &local_err);
> +        ret = blk_truncate(blk, new_size, PREALLOC_MODE_OFF, errp);
>          blk_unref(blk);
>          if (ret < 0) {
> -            error_report_err(local_err);
>              return ret;
>          }
>      }
> @@ -4370,7 +4394,7 @@ static int qcow2_amend_options(BlockDriverState *bs, QemuOpts *opts,
>      if (new_version < old_version) {
>          helper_cb_info.current_operation = QCOW2_DOWNGRADING;
>          ret = qcow2_downgrade(bs, new_version, &qcow2_amend_helper_cb,
> -                              &helper_cb_info);
> +                              &helper_cb_info, errp);
>          if (ret < 0) {
>              return ret;
>          }
> diff --git a/qemu-img.c b/qemu-img.c
> index f60b22769e..375fe852e0 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -3730,10 +3730,10 @@ static int img_amend(int argc, char **argv)
>  
>      /* In case the driver does not call amend_status_cb() */
>      qemu_progress_print(0.f, 0);
> -    ret = bdrv_amend_options(bs, opts, &amend_status_cb, NULL);
> +    ret = bdrv_amend_options(bs, opts, &amend_status_cb, NULL, &err);
>      qemu_progress_print(100.f, 0);
>      if (ret < 0) {
> -        error_report("Error while amending options: %s", strerror(-ret));
> +        error_report_err(err);
>          goto out;
>      }
>  
> diff --git a/tests/qemu-iotests/060.out b/tests/qemu-iotests/060.out
> index 25d5c3938b..5f4264cff6 100644
> --- a/tests/qemu-iotests/060.out
> +++ b/tests/qemu-iotests/060.out
> @@ -129,7 +129,7 @@ Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
>  wrote 65536/65536 bytes at offset 0
>  64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
>  qcow2: Marking image as corrupt: L2 table offset 0x42a00 unaligned (L1 index: 0); further corruption events will be suppressed
> -qemu-img: Error while amending options: Input/output error
> +qemu-img: Failed to turn zero into data clusters: Input/output error
>  
>  === Testing unaligned L2 entry ===
>  
> @@ -145,7 +145,7 @@ Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
>  wrote 65536/65536 bytes at offset 0
>  64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
>  qcow2: Marking image as corrupt: Cluster allocation offset 0x52a00 unaligned (L2 offset: 0x40000, L2 index: 0); further corruption events will be suppressed
> -qemu-img: Error while amending options: Input/output error
> +qemu-img: Failed to turn zero into data clusters: Input/output error
>  
>  === Testing unaligned reftable entry ===
>  
> diff --git a/tests/qemu-iotests/061.out b/tests/qemu-iotests/061.out
> index e857ef9a7d..183f7dd690 100644
> --- a/tests/qemu-iotests/061.out
> +++ b/tests/qemu-iotests/061.out
> @@ -358,18 +358,12 @@ No errors were found on the image.
>  
>  Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
>  qemu-img: Lazy refcounts only supported with compatibility level 1.1 and above (use compat=1.1 or greater)
> -qemu-img: Error while amending options: Invalid argument
>  qemu-img: Lazy refcounts only supported with compatibility level 1.1 and above (use compat=1.1 or greater)
> -qemu-img: Error while amending options: Invalid argument
>  qemu-img: Unknown compatibility level 0.42
> -qemu-img: Error while amending options: Invalid argument
>  qemu-img: Invalid parameter 'foo'
>  qemu-img: Changing the cluster size is not supported
> -qemu-img: Error while amending options: Operation not supported
>  qemu-img: Changing the encryption flag is not supported
> -qemu-img: Error while amending options: Operation not supported
>  qemu-img: Cannot change preallocation mode
> -qemu-img: Error while amending options: Operation not supported
>  
>  === Testing correct handling of unset value ===
>  
> @@ -377,7 +371,6 @@ Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
>  Should work:
>  Should not work:
>  qemu-img: Changing the cluster size is not supported
> -qemu-img: Error while amending options: Operation not supported
>  
>  === Testing zero expansion on inactive clusters ===
>  
> diff --git a/tests/qemu-iotests/080.out b/tests/qemu-iotests/080.out
> index 4e0f7f7b92..281c7e0d1d 100644
> --- a/tests/qemu-iotests/080.out
> +++ b/tests/qemu-iotests/080.out
> @@ -65,7 +65,7 @@ wrote 512/512 bytes at offset 0
>  512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
>  qemu-img: Failed to load snapshot: Snapshot L1 table offset invalid
>  qemu-img: Snapshot L1 table offset invalid
> -qemu-img: Error while amending options: Invalid argument
> +qemu-img: Failed to turn zero into data clusters: Invalid argument
>  Failed to flush the refcount block cache: Invalid argument
>  write failed: Invalid argument
>  qemu-img: Snapshot L1 table offset invalid
> @@ -88,7 +88,7 @@ wrote 512/512 bytes at offset 0
>  512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
>  qemu-img: Failed to load snapshot: Snapshot L1 table too large
>  qemu-img: Snapshot L1 table too large
> -qemu-img: Error while amending options: File too large
> +qemu-img: Failed to turn zero into data clusters: File too large
>  Failed to flush the refcount block cache: File too large
>  write failed: File too large
>  qemu-img: Snapshot L1 table too large
> diff --git a/tests/qemu-iotests/112.out b/tests/qemu-iotests/112.out
> index 86f041075d..a23aa1be75 100644
> --- a/tests/qemu-iotests/112.out
> +++ b/tests/qemu-iotests/112.out
> @@ -99,13 +99,11 @@ refcount bits: 64
>  === Amend to compat=0.10 ===
>  
>  qemu-img: compat=0.10 requires refcount_bits=16
> -qemu-img: Error while amending options: Operation not supported
>  refcount bits: 64
>  No errors were found on the image.
>  refcount bits: 16
>  refcount bits: 16
>  qemu-img: Different refcount widths than 16 bits require compatibility level 1.1 or above (use compat=1.1 or greater)
> -qemu-img: Error while amending options: Invalid argument
>  refcount bits: 16
>  
>  === Amend with snapshot ===
> @@ -113,7 +111,6 @@ refcount bits: 16
>  wrote 16777216/16777216 bytes at offset 0
>  16 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
>  qemu-img: Cannot decrease refcount entry width to 1 bits: Cluster at offset 0x50000 has a refcount of 2
> -qemu-img: Error while amending options: Invalid argument
>  No errors were found on the image.
>  refcount bits: 16
>  No errors were found on the image.
> 

1,2: Reviewed-by: John Snow <jsnow@redhat.com>

[I'll get to the rest.]

Re: [Qemu-devel] [PATCH 2/7] block: Add Error parameter to bdrv_amend_options

[Eric Blake]

On 04/21/2018 11:54 AM, Max Reitz wrote:
> Looking at the qcow2 code that is riddled with error_report() calls,
> this is really how it should have been from the start.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---

> +++ b/block/qcow2.c
> @@ -4049,7 +4049,8 @@ static int qcow2_load_vmstate(BlockDriverState *bs, QEMUIOVector *qiov,
>    * have to be removed.
>    */
>   static int qcow2_downgrade(BlockDriverState *bs, int target_version,
> -                           BlockDriverAmendStatusCB *status_cb, void *cb_opaque)
> +                           BlockDriverAmendStatusCB *status_cb, void *cb_opaque,
> +                           Error **errp)
>   {
>       BDRVQcow2State *s = bs->opaque;
>       int current_version = s->qcow_version;
> @@ -4058,13 +4059,17 @@ static int qcow2_downgrade(BlockDriverState *bs, int target_version,
>       if (target_version == current_version) {
>           return 0;
>       } else if (target_version > current_version) {
> +        error_setg(errp, "Cannot downgrade an image from version %i to %i",

%i is unusual compared to %d, but as they do the same, I won't make you 
change it, if we keep it.  That said, this function is static; and the 
lone caller qcow2_amend_options is already doing sanity checks, right? 
(Yes - it does qemu_opt_get(opts, BLOCK_OPT_COMPAT_LEVEL), and then 
validates that the user's information is in range, which already filters 
out everything but v2 and v3; then _this_ function is called exactly for 
3 downto 2).  So this should be an assert() instead of an error message.


> +                   current_version, target_version);
>           return -EINVAL;
>       } else if (target_version != 2) {
> +        error_setg(errp, "Cannot downgrade an image to version %i (only "
> +                   "target version 2 is supported)", target_version);

And again.

> @@ -4072,6 +4077,8 @@ static int qcow2_downgrade(BlockDriverState *bs, int target_version,
>       if (s->incompatible_features & QCOW2_INCOMPAT_DIRTY) {
>           ret = qcow2_mark_clean(bs);
>           if (ret < 0) {
> +            error_setg(errp, "Failed to make the image clean: %s",
> +                       strerror(-ret));

error_setg_errno(), please.


> @@ -4094,6 +4103,8 @@ static int qcow2_downgrade(BlockDriverState *bs, int target_version,
>   
>       ret = qcow2_expand_zero_clusters(bs, status_cb, cb_opaque);
>       if (ret < 0) {
> +        error_setg(errp, "Failed to turn zero into data clusters: %s",
> +                   strerror(-ret));

and again

>           return ret;
>       }
>   
> @@ -4101,6 +4112,8 @@ static int qcow2_downgrade(BlockDriverState *bs, int target_version,
>       ret = qcow2_update_header(bs);
>       if (ret < 0) {
>           s->qcow_version = current_version;
> +        error_setg(errp, "Failed to update the image header: %s",
> +                   strerror(-ret));

etc.  I'll quit pointing it out, but there are more.

> @@ -4293,18 +4311,17 @@ static int qcow2_amend_options(BlockDriverState *bs, QemuOpts *opts,
>           int refcount_order = ctz32(refcount_bits);
>   
>           if (new_version < 3 && refcount_bits != 16) {
> -            error_report("Different refcount widths than 16 bits require "
> -                         "compatibility level 1.1 or above (use compat=1.1 or "
> -                         "greater)");
> +            error_setg(errp, "Different refcount widths than 16 bits require "
> +                       "compatibility level 1.1 or above (use compat=1.1 or "
> +                       "greater)");

Not your fault, but that reads poorly.  Better would be:

Refcount widths other than 16 bits require compatibility level 1.1 or above

> +++ b/qemu-img.c
> @@ -3730,10 +3730,10 @@ static int img_amend(int argc, char **argv)
>   
>       /* In case the driver does not call amend_status_cb() */
>       qemu_progress_print(0.f, 0);
> -    ret = bdrv_amend_options(bs, opts, &amend_status_cb, NULL);
> +    ret = bdrv_amend_options(bs, opts, &amend_status_cb, NULL, &err);
>       qemu_progress_print(100.f, 0);
>       if (ret < 0) {
> -        error_report("Error while amending options: %s", strerror(-ret));
> +        error_report_err(err);

Definitely nicer, but does change various outputs; glad you fixed 
iotests to match.

Overall a nice patch, looking forward to v2.

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

Re: [Qemu-devel] [PATCH 2/7] block: Add Error parameter to bdrv_amend_options

[Max Reitz]

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

On 2018-05-02 19:52, Eric Blake wrote:
> On 04/21/2018 11:54 AM, Max Reitz wrote:
>> Looking at the qcow2 code that is riddled with error_report() calls,
>> this is really how it should have been from the start.
>>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> ---
> 
>> +++ b/block/qcow2.c
>> @@ -4049,7 +4049,8 @@ static int qcow2_load_vmstate(BlockDriverState
>> *bs, QEMUIOVector *qiov,
>>    * have to be removed.
>>    */
>>   static int qcow2_downgrade(BlockDriverState *bs, int target_version,
>> -                           BlockDriverAmendStatusCB *status_cb, void
>> *cb_opaque)
>> +                           BlockDriverAmendStatusCB *status_cb, void
>> *cb_opaque,
>> +                           Error **errp)
>>   {
>>       BDRVQcow2State *s = bs->opaque;
>>       int current_version = s->qcow_version;
>> @@ -4058,13 +4059,17 @@ static int qcow2_downgrade(BlockDriverState
>> *bs, int target_version,
>>       if (target_version == current_version) {
>>           return 0;
>>       } else if (target_version > current_version) {
>> +        error_setg(errp, "Cannot downgrade an image from version %i
>> to %i",
> 
> %i is unusual compared to %d, but as they do the same, I won't make you
> change it, if we keep it.  That said, this function is static; and the
> lone caller qcow2_amend_options is already doing sanity checks, right?
> (Yes - it does qemu_opt_get(opts, BLOCK_OPT_COMPAT_LEVEL), and then
> validates that the user's information is in range, which already filters
> out everything but v2 and v3; then _this_ function is called exactly for
> 3 downto 2).  So this should be an assert() instead of an error message.

OK, I'll change it to an assertion.

>> +                   current_version, target_version);
>>           return -EINVAL;
>>       } else if (target_version != 2) {
>> +        error_setg(errp, "Cannot downgrade an image to version %i
>> (only "
>> +                   "target version 2 is supported)", target_version);
> 
> And again.
> 
>> @@ -4072,6 +4077,8 @@ static int qcow2_downgrade(BlockDriverState *bs,
>> int target_version,
>>       if (s->incompatible_features & QCOW2_INCOMPAT_DIRTY) {
>>           ret = qcow2_mark_clean(bs);
>>           if (ret < 0) {
>> +            error_setg(errp, "Failed to make the image clean: %s",
>> +                       strerror(-ret));
> 
> error_setg_errno(), please.

Right, forgot we had that...

>> @@ -4094,6 +4103,8 @@ static int qcow2_downgrade(BlockDriverState *bs,
>> int target_version,
>>         ret = qcow2_expand_zero_clusters(bs, status_cb, cb_opaque);
>>       if (ret < 0) {
>> +        error_setg(errp, "Failed to turn zero into data clusters: %s",
>> +                   strerror(-ret));
> 
> and again
> 
>>           return ret;
>>       }
>>   @@ -4101,6 +4112,8 @@ static int qcow2_downgrade(BlockDriverState
>> *bs, int target_version,
>>       ret = qcow2_update_header(bs);
>>       if (ret < 0) {
>>           s->qcow_version = current_version;
>> +        error_setg(errp, "Failed to update the image header: %s",
>> +                   strerror(-ret));
> 
> etc.  I'll quit pointing it out, but there are more.
> 
>> @@ -4293,18 +4311,17 @@ static int
>> qcow2_amend_options(BlockDriverState *bs, QemuOpts *opts,
>>           int refcount_order = ctz32(refcount_bits);
>>             if (new_version < 3 && refcount_bits != 16) {
>> -            error_report("Different refcount widths than 16 bits
>> require "
>> -                         "compatibility level 1.1 or above (use
>> compat=1.1 or "
>> -                         "greater)");
>> +            error_setg(errp, "Different refcount widths than 16 bits
>> require "
>> +                       "compatibility level 1.1 or above (use
>> compat=1.1 or "
>> +                       "greater)");
> 
> Not your fault, but that reads poorly.  Better would be:
> 
> Refcount widths other than 16 bits require compatibility level 1.1 or above

Sure it's my fault, I wrote this in the first place. :-)

Max

>> +++ b/qemu-img.c
>> @@ -3730,10 +3730,10 @@ static int img_amend(int argc, char **argv)
>>         /* In case the driver does not call amend_status_cb() */
>>       qemu_progress_print(0.f, 0);
>> -    ret = bdrv_amend_options(bs, opts, &amend_status_cb, NULL);
>> +    ret = bdrv_amend_options(bs, opts, &amend_status_cb, NULL, &err);
>>       qemu_progress_print(100.f, 0);
>>       if (ret < 0) {
>> -        error_report("Error while amending options: %s",
>> strerror(-ret));
>> +        error_report_err(err);
> 
> Definitely nicer, but does change various outputs; glad you fixed
> iotests to match.
> 
> Overall a nice patch, looking forward to v2.
> 



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

[Qemu-devel] [PATCH 3/7] qemu-option: Pull out "Supported options" print

[Max Reitz]

It really is up to the caller to decide what this list of options means.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 qemu-img.c         | 1 +
 util/qemu-option.c | 1 -
 2 files changed, 1 insertion(+), 1 deletion(-)

diff --git a/qemu-img.c b/qemu-img.c
index 375fe852e0..6dd8e95bb2 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -261,6 +261,7 @@ static int print_block_option_help(const char *filename, const char *fmt)
         create_opts = qemu_opts_append(create_opts, proto_drv->create_opts);
     }
 
+    printf("Supported options:\n");
     qemu_opts_print_help(create_opts);
     qemu_opts_free(create_opts);
     return 0;
diff --git a/util/qemu-option.c b/util/qemu-option.c
index d0756fda58..95e6cf4e49 100644
--- a/util/qemu-option.c
+++ b/util/qemu-option.c
@@ -218,7 +218,6 @@ void qemu_opts_print_help(QemuOptsList *list)
 
     assert(list);
     desc = list->desc;
-    printf("Supported options:\n");
     while (desc && desc->name) {
         printf("%-16s %s\n", desc->name,
                desc->help ? desc->help : "No description available");
-- 
2.14.3

Re: [Qemu-devel] [PATCH 3/7] qemu-option: Pull out "Supported options" print

[Eric Blake]

On 04/21/2018 11:54 AM, Max Reitz wrote:
> It really is up to the caller to decide what this list of options means.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>   qemu-img.c         | 1 +
>   util/qemu-option.c | 1 -
>   2 files changed, 1 insertion(+), 1 deletion(-)
> 

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

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

[Qemu-devel] [PATCH 4/7] qemu-img: Add print_amend_option_help()

[Max Reitz]

The more generic print_block_option_help() function is not really
suitable for qemu-img amend, for a couple of reasons:
(1) We do not need to append the protocol-level options, as amendment
    happens only on one node and does not descend downwards to its
    children.
(2) print_block_option_help() says those options are "supported".  For
    option amendment, we do not really know that.  So this new function
    explicitly says that those options are the creation options, and not
    all of them may be supported.
(3) If the driver does not support option amendment, we should not print
    anything (except for an error message that amendment is not
    supported).

Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1537956
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 qemu-img.c                 | 30 ++++++++++++++++++++++++++++--
 tests/qemu-iotests/082.out | 44 +++++++++++++++++++++++++++-----------------
 2 files changed, 55 insertions(+), 19 deletions(-)

diff --git a/qemu-img.c b/qemu-img.c
index 6dd8e95bb2..45e243cc80 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -3578,6 +3578,32 @@ static void amend_status_cb(BlockDriverState *bs,
     qemu_progress_print(100.f * offset / total_work_size, 0);
 }
 
+static int print_amend_option_help(const char *format)
+{
+    BlockDriver *drv;
+
+    /* Find driver and parse its options */
+    drv = bdrv_find_format(format);
+    if (!drv) {
+        error_report("Unknown file format '%s'", format);
+        return 1;
+    }
+
+    if (!drv->bdrv_amend_options) {
+        error_report("Format driver '%s' does not support option amendment",
+                     format);
+        return 1;
+    }
+
+    /* Every driver supporting amendment must have create_opts */
+    assert(drv->create_opts);
+
+    printf("Creation options for '%s':\n", format);
+    qemu_opts_print_help(drv->create_opts);
+    printf("\nNote that not all of these options may be amendable.\n");
+    return 0;
+}
+
 static int img_amend(int argc, char **argv)
 {
     Error *err = NULL;
@@ -3677,7 +3703,7 @@ static int img_amend(int argc, char **argv)
     if (fmt && has_help_option(options)) {
         /* If a format is explicitly specified (and possibly no filename is
          * given), print option help here */
-        ret = print_block_option_help(filename, fmt);
+        ret = print_amend_option_help(fmt);
         goto out;
     }
 
@@ -3706,7 +3732,7 @@ static int img_amend(int argc, char **argv)
 
     if (has_help_option(options)) {
         /* If the format was auto-detected, print option help here */
-        ret = print_block_option_help(filename, fmt);
+        ret = print_amend_option_help(fmt);
         goto out;
     }
 
diff --git a/tests/qemu-iotests/082.out b/tests/qemu-iotests/082.out
index 1527fbe1b7..4e52dce8d6 100644
--- a/tests/qemu-iotests/082.out
+++ b/tests/qemu-iotests/082.out
@@ -546,7 +546,7 @@ cluster_size: 65536
 === amend: help for -o ===
 
 Testing: amend -f qcow2 -o help TEST_DIR/t.qcow2
-Supported options:
+Creation options for 'qcow2':
 size             Virtual disk size
 compat           Compatibility level (0.10 or 1.1)
 backing_file     File name of a base image
@@ -564,10 +564,11 @@ cluster_size     qcow2 cluster size
 preallocation    Preallocation mode (allowed values: off, metadata, falloc, full)
 lazy_refcounts   Postpone refcount updates
 refcount_bits    Width of a reference count entry in bits
-nocow            Turn off copy-on-write (valid only on btrfs)
+
+Note that not all of these options may be amendable.
 
 Testing: amend -f qcow2 -o ? TEST_DIR/t.qcow2
-Supported options:
+Creation options for 'qcow2':
 size             Virtual disk size
 compat           Compatibility level (0.10 or 1.1)
 backing_file     File name of a base image
@@ -585,10 +586,11 @@ cluster_size     qcow2 cluster size
 preallocation    Preallocation mode (allowed values: off, metadata, falloc, full)
 lazy_refcounts   Postpone refcount updates
 refcount_bits    Width of a reference count entry in bits
-nocow            Turn off copy-on-write (valid only on btrfs)
+
+Note that not all of these options may be amendable.
 
 Testing: amend -f qcow2 -o cluster_size=4k,help TEST_DIR/t.qcow2
-Supported options:
+Creation options for 'qcow2':
 size             Virtual disk size
 compat           Compatibility level (0.10 or 1.1)
 backing_file     File name of a base image
@@ -606,10 +608,11 @@ cluster_size     qcow2 cluster size
 preallocation    Preallocation mode (allowed values: off, metadata, falloc, full)
 lazy_refcounts   Postpone refcount updates
 refcount_bits    Width of a reference count entry in bits
-nocow            Turn off copy-on-write (valid only on btrfs)
+
+Note that not all of these options may be amendable.
 
 Testing: amend -f qcow2 -o cluster_size=4k,? TEST_DIR/t.qcow2
-Supported options:
+Creation options for 'qcow2':
 size             Virtual disk size
 compat           Compatibility level (0.10 or 1.1)
 backing_file     File name of a base image
@@ -627,10 +630,11 @@ cluster_size     qcow2 cluster size
 preallocation    Preallocation mode (allowed values: off, metadata, falloc, full)
 lazy_refcounts   Postpone refcount updates
 refcount_bits    Width of a reference count entry in bits
-nocow            Turn off copy-on-write (valid only on btrfs)
+
+Note that not all of these options may be amendable.
 
 Testing: amend -f qcow2 -o help,cluster_size=4k TEST_DIR/t.qcow2
-Supported options:
+Creation options for 'qcow2':
 size             Virtual disk size
 compat           Compatibility level (0.10 or 1.1)
 backing_file     File name of a base image
@@ -648,10 +652,11 @@ cluster_size     qcow2 cluster size
 preallocation    Preallocation mode (allowed values: off, metadata, falloc, full)
 lazy_refcounts   Postpone refcount updates
 refcount_bits    Width of a reference count entry in bits
-nocow            Turn off copy-on-write (valid only on btrfs)
+
+Note that not all of these options may be amendable.
 
 Testing: amend -f qcow2 -o ?,cluster_size=4k TEST_DIR/t.qcow2
-Supported options:
+Creation options for 'qcow2':
 size             Virtual disk size
 compat           Compatibility level (0.10 or 1.1)
 backing_file     File name of a base image
@@ -669,10 +674,11 @@ cluster_size     qcow2 cluster size
 preallocation    Preallocation mode (allowed values: off, metadata, falloc, full)
 lazy_refcounts   Postpone refcount updates
 refcount_bits    Width of a reference count entry in bits
-nocow            Turn off copy-on-write (valid only on btrfs)
+
+Note that not all of these options may be amendable.
 
 Testing: amend -f qcow2 -o cluster_size=4k -o help TEST_DIR/t.qcow2
-Supported options:
+Creation options for 'qcow2':
 size             Virtual disk size
 compat           Compatibility level (0.10 or 1.1)
 backing_file     File name of a base image
@@ -690,10 +696,11 @@ cluster_size     qcow2 cluster size
 preallocation    Preallocation mode (allowed values: off, metadata, falloc, full)
 lazy_refcounts   Postpone refcount updates
 refcount_bits    Width of a reference count entry in bits
-nocow            Turn off copy-on-write (valid only on btrfs)
+
+Note that not all of these options may be amendable.
 
 Testing: amend -f qcow2 -o cluster_size=4k -o ? TEST_DIR/t.qcow2
-Supported options:
+Creation options for 'qcow2':
 size             Virtual disk size
 compat           Compatibility level (0.10 or 1.1)
 backing_file     File name of a base image
@@ -711,7 +718,8 @@ cluster_size     qcow2 cluster size
 preallocation    Preallocation mode (allowed values: off, metadata, falloc, full)
 lazy_refcounts   Postpone refcount updates
 refcount_bits    Width of a reference count entry in bits
-nocow            Turn off copy-on-write (valid only on btrfs)
+
+Note that not all of these options may be amendable.
 
 Testing: amend -f qcow2 -o backing_file=TEST_DIR/t.qcow2,,help TEST_DIR/t.qcow2
 
@@ -731,7 +739,7 @@ Testing: amend -f qcow2 -o backing_file=TEST_DIR/t.qcow2 -o ,, -o help TEST_DIR/
 qemu-img: Invalid option list: ,,
 
 Testing: amend -f qcow2 -o help
-Supported options:
+Creation options for 'qcow2':
 size             Virtual disk size
 compat           Compatibility level (0.10 or 1.1)
 backing_file     File name of a base image
@@ -750,6 +758,8 @@ preallocation    Preallocation mode (allowed values: off, metadata, falloc, full
 lazy_refcounts   Postpone refcount updates
 refcount_bits    Width of a reference count entry in bits
 
+Note that not all of these options may be amendable.
+
 Testing: convert -o help
 Supported options:
 size             Virtual disk size
-- 
2.14.3

Re: [Qemu-devel] [PATCH 4/7] qemu-img: Add print_amend_option_help()

[Eric Blake]

On 04/21/2018 11:54 AM, Max Reitz wrote:
> The more generic print_block_option_help() function is not really
> suitable for qemu-img amend, for a couple of reasons:
> (1) We do not need to append the protocol-level options, as amendment
>      happens only on one node and does not descend downwards to its
>      children.
> (2) print_block_option_help() says those options are "supported".  For
>      option amendment, we do not really know that.  So this new function
>      explicitly says that those options are the creation options, and not
>      all of them may be supported.
> (3) If the driver does not support option amendment, we should not print
>      anything (except for an error message that amendment is not
>      supported).
> 
> Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1537956
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>   qemu-img.c                 | 30 ++++++++++++++++++++++++++++--
>   tests/qemu-iotests/082.out | 44 +++++++++++++++++++++++++++-----------------
>   2 files changed, 55 insertions(+), 19 deletions(-)
> 

> diff --git a/qemu-img.c b/qemu-img.c
> index 6dd8e95bb2..45e243cc80 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -3578,6 +3578,32 @@ static void amend_status_cb(BlockDriverState *bs,
>       qemu_progress_print(100.f * offset / total_work_size, 0);
>   }
>   
> +static int print_amend_option_help(const char *format)
> +{
> +    BlockDriver *drv;
> +
> +    /* Find driver and parse its options */
> +    drv = bdrv_find_format(format);
> +    if (!drv) {
> +        error_report("Unknown file format '%s'", format);
> +        return 1;
> +    }

Not your fault; I'd love it if this file consistently used EXIT_FAILURE 
instead of 1 (since that's a bit more obvious why we are returning a 
positive value instead of our more usual negative-on-error - we expect 
the caller to feed our return value to exit()), but that's a separate 
cleanup.

But the new output is definitely nicer.

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

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

[Qemu-devel] [PATCH 5/7] qemu-img: Recognize no creation support in -o help

[Max Reitz]

The only users of print_block_option_help() are qemu-img create and
qemu-img convert for the output image, so this function is always used
for image creation (it used to be used for amendment also, but that is
no longer the case).

So if image creation is not supported by either the format or the
protocol, there is no need to print any option description, because the
user cannot create an image like this anyway.

This also fixes an assertion failure:

    $ qemu-img create -f bochs -o help
    Supported options:
    qemu-img: util/qemu-option.c:219:
    qemu_opts_print_help: Assertion `list' failed.
    [1]    24831 abort (core dumped)  qemu-img create -f bochs -o help

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 qemu-img.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/qemu-img.c b/qemu-img.c
index 45e243cc80..29efd80c35 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -250,6 +250,11 @@ static int print_block_option_help(const char *filename, const char *fmt)
         return 1;
     }
 
+    if (!drv->create_opts) {
+        error_report("Format driver '%s' does not support image creation", fmt);
+        return 1;
+    }
+
     create_opts = qemu_opts_append(create_opts, drv->create_opts);
     if (filename) {
         proto_drv = bdrv_find_protocol(filename, true, &local_err);
@@ -258,6 +263,11 @@ static int print_block_option_help(const char *filename, const char *fmt)
             qemu_opts_free(create_opts);
             return 1;
         }
+        if (!proto_drv->create_opts) {
+            error_report("Protocal driver '%s' does not support image creation",
+                         proto_drv->format_name);
+            return 1;
+        }
         create_opts = qemu_opts_append(create_opts, proto_drv->create_opts);
     }
 
-- 
2.14.3

Re: [Qemu-devel] [Qemu-block] [PATCH 5/7] qemu-img: Recognize no creation support in -o help

[John Snow]

On 04/21/2018 12:54 PM, Max Reitz wrote:
> The only users of print_block_option_help() are qemu-img create and
> qemu-img convert for the output image, so this function is always used
> for image creation (it used to be used for amendment also, but that is
> no longer the case).
> 
> So if image creation is not supported by either the format or the
> protocol, there is no need to print any option description, because the
> user cannot create an image like this anyway.
> 
> This also fixes an assertion failure:
> 
>     $ qemu-img create -f bochs -o help
>     Supported options:
>     qemu-img: util/qemu-option.c:219:
>     qemu_opts_print_help: Assertion `list' failed.
>     [1]    24831 abort (core dumped)  qemu-img create -f bochs -o help
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>

This is great.

3-5: Reviewed-by: John Snow <jsnow@redhat.com>

Re: [Qemu-devel] [PATCH 5/7] qemu-img: Recognize no creation support in -o help

[Eric Blake]

On 04/21/2018 11:54 AM, Max Reitz wrote:
> The only users of print_block_option_help() are qemu-img create and
> qemu-img convert for the output image, so this function is always used
> for image creation (it used to be used for amendment also, but that is
> no longer the case).
> 
> So if image creation is not supported by either the format or the
> protocol, there is no need to print any option description, because the
> user cannot create an image like this anyway.
> 
> This also fixes an assertion failure:
> 
>      $ qemu-img create -f bochs -o help
>      Supported options:
>      qemu-img: util/qemu-option.c:219:
>      qemu_opts_print_help: Assertion `list' failed.
>      [1]    24831 abort (core dumped)  qemu-img create -f bochs -o help
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>   qemu-img.c | 10 ++++++++++
>   1 file changed, 10 insertions(+)
> 
Reviewed-by: Eric Blake <eblake@redhat.com>

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

[Qemu-devel] [PATCH 6/7] iotests: Test help option for unsupporting formats

[Max Reitz]

This adds test cases to 082 for qemu-img create/convert/amend "-o help"
on formats that do not support creation or amendment, respectively.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 tests/qemu-iotests/082     | 9 +++++++++
 tests/qemu-iotests/082.out | 9 +++++++++
 2 files changed, 18 insertions(+)

diff --git a/tests/qemu-iotests/082 b/tests/qemu-iotests/082
index d5c83d45ed..a872f771a6 100755
--- a/tests/qemu-iotests/082
+++ b/tests/qemu-iotests/082
@@ -97,6 +97,9 @@ run_qemu_img create -f $IMGFMT -o backing_file="$TEST_IMG" -o ,, -o help "$TEST_
 run_qemu_img create -f $IMGFMT -o help
 run_qemu_img create -o help
 
+# Try help option for a format that does not support creation
+run_qemu_img create -f bochs -o help
+
 echo
 echo === convert: Options specified more than once ===
 
@@ -151,6 +154,9 @@ run_qemu_img convert -O $IMGFMT -o backing_file="$TEST_IMG" -o ,, -o help "$TEST
 run_qemu_img convert -O $IMGFMT -o help
 run_qemu_img convert -o help
 
+# Try help option for a format that does not support creation
+run_qemu_img convert -O bochs -o help
+
 echo
 echo === amend: Options specified more than once ===
 
@@ -201,6 +207,9 @@ run_qemu_img amend -f $IMGFMT -o backing_file="$TEST_IMG" -o ,, -o help "$TEST_I
 run_qemu_img amend -f $IMGFMT -o help
 run_qemu_img convert -o help
 
+# Try help option for a format that does not support amendment
+run_qemu_img amend -f bochs -o help
+
 # success, all done
 echo "*** done"
 rm -f $seq.full
diff --git a/tests/qemu-iotests/082.out b/tests/qemu-iotests/082.out
index 4e52dce8d6..60ef87c276 100644
--- a/tests/qemu-iotests/082.out
+++ b/tests/qemu-iotests/082.out
@@ -249,6 +249,9 @@ Testing: create -o help
 Supported options:
 size             Virtual disk size
 
+Testing: create -f bochs -o help
+qemu-img: Format driver 'bochs' does not support image creation
+
 === convert: Options specified more than once ===
 
 Testing: create -f qcow2 TEST_DIR/t.qcow2 128M
@@ -502,6 +505,9 @@ Testing: convert -o help
 Supported options:
 size             Virtual disk size
 
+Testing: convert -O bochs -o help
+qemu-img: Format driver 'bochs' does not support image creation
+
 === amend: Options specified more than once ===
 
 Testing: amend -f foo -f qcow2 -o lazy_refcounts=on TEST_DIR/t.qcow2
@@ -763,4 +769,7 @@ Note that not all of these options may be amendable.
 Testing: convert -o help
 Supported options:
 size             Virtual disk size
+
+Testing: amend -f bochs -o help
+qemu-img: Format driver 'bochs' does not support option amendment
 *** done
-- 
2.14.3

Re: [Qemu-devel] [Qemu-block] [PATCH 6/7] iotests: Test help option for unsupporting formats

[John Snow]

On 04/21/2018 12:54 PM, Max Reitz wrote:
> This adds test cases to 082 for qemu-img create/convert/amend "-o help"
> on formats that do not support creation or amendment, respectively.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>

Reviewed-by: John Snow <jsnow@redhat.com>

Re: [Qemu-devel] [PATCH 6/7] iotests: Test help option for unsupporting formats

[Eric Blake]

On 04/21/2018 11:54 AM, Max Reitz wrote:
> This adds test cases to 082 for qemu-img create/convert/amend "-o help"
> on formats that do not support creation or amendment, respectively.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>   tests/qemu-iotests/082     | 9 +++++++++
>   tests/qemu-iotests/082.out | 9 +++++++++
>   2 files changed, 18 insertions(+)
> 
> diff --git a/tests/qemu-iotests/082 b/tests/qemu-iotests/082
> index d5c83d45ed..a872f771a6 100755
> --- a/tests/qemu-iotests/082
> +++ b/tests/qemu-iotests/082
> @@ -97,6 +97,9 @@ run_qemu_img create -f $IMGFMT -o backing_file="$TEST_IMG" -o ,, -o help "$TEST_
>   run_qemu_img create -f $IMGFMT -o help
>   run_qemu_img create -o help
>   
> +# Try help option for a format that does not support creation
> +run_qemu_img create -f bochs -o help

If we ever get brave enough to add creation support for bochs, hopefully 
we figure out what iotests need tweaking at that point (realistically, I 
don't see that ever happening).

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

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

[Qemu-devel] [PATCH 7/7] iotests: Rework 113

[Max Reitz]

This test case has been broken since 398e6ad014df261d (roughly half a
year).  qemu-img amend requires its output image to be R/W, so it opens
it as such; the node is then turned into an read-only node automatically
which is now accompanied by a warning, however.  This warning has not
been part of the reference output.

For one thing, this warning shows that we cannot keep the test case as
it is.  We would need a format that has no create_opts but that does
have write support -- we do not have such a format, though.

Another thing is that qemu now actually checks whether an image format
supports amendment instead of whether it has create_opts (since the
former always implies the latter).  So we can now use any format that
does not support amendment (even if it supports creation) and thus test
the same code path.

The reason nobody has noticed the breakage until now of course is the
fact that nobody runs the iotests for nbd+bochs.  There actually was
never any reason to set the protocol to "nbd" but because that was
technically correct; functionally it made no difference.  So that is the
first thing we are going to change: Make the protocol "file" instead so
that people might actually notice breakage here.

Secondly, now that bochs no longer works for the amend test case, we
have to change the format there anyway.  Set let us just bend the truth
a bit, declare this test a raw test.  In fact, that does not even
concern the bochs test cases, other than the output now reading 'bochs'
instead of 'IMGFMT'.

So with this test now being a raw test, we can rework the amend test
case to use raw instead.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 tests/qemu-iotests/113     | 19 +++++++++----------
 tests/qemu-iotests/113.out |  7 ++++---
 2 files changed, 13 insertions(+), 13 deletions(-)

diff --git a/tests/qemu-iotests/113 b/tests/qemu-iotests/113
index 19b68b2727..4e09810905 100755
--- a/tests/qemu-iotests/113
+++ b/tests/qemu-iotests/113
@@ -38,16 +38,17 @@ trap "_cleanup; exit \$status" 0 1 2 3 15
 . ./common.rc
 . ./common.filter
 
-# We can only test one format here because we need its sample file
-_supported_fmt bochs
-_supported_proto nbd
+# Some of these test cases use bochs, but others do use raw, so this
+# is only half a lie.
+_supported_fmt raw
+_supported_proto file
 _supported_os Linux
 
 echo
 echo '=== Unsupported image creation in qemu-img create ==='
 echo
 
-$QEMU_IMG create -f $IMGFMT nbd://example.com 2>&1 64M | _filter_imgfmt
+$QEMU_IMG create -f bochs nbd://example.com 2>&1 64M
 
 echo
 echo '=== Unsupported image creation in qemu-img convert ==='
@@ -56,17 +57,15 @@ echo
 # We could use any input image format here, but this is a bochs test, so just
 # use the bochs image
 _use_sample_img empty.bochs.bz2
-$QEMU_IMG convert -f $IMGFMT -O $IMGFMT "$TEST_IMG" nbd://example.com 2>&1 \
-    | _filter_imgfmt
+$QEMU_IMG convert -f bochs -O bochs "$TEST_IMG" nbd://example.com
 
 echo
 echo '=== Unsupported format in qemu-img amend ==='
 echo
 
-# The protocol does not matter here
-_use_sample_img empty.bochs.bz2
-$QEMU_IMG amend -f $IMGFMT -o foo=bar "$TEST_IMG" 2>&1 | _filter_imgfmt
-
+TEST_IMG="$TEST_DIR/t.$IMGFMT"
+_make_test_img 1M
+$QEMU_IMG amend -f $IMGFMT -o size=2M "$TEST_IMG" 2>&1 | _filter_imgfmt
 
 # success, all done
 echo
diff --git a/tests/qemu-iotests/113.out b/tests/qemu-iotests/113.out
index 00bdfd6887..3557e2bbf0 100644
--- a/tests/qemu-iotests/113.out
+++ b/tests/qemu-iotests/113.out
@@ -2,14 +2,15 @@ QA output created by 113
 
 === Unsupported image creation in qemu-img create ===
 
-qemu-img: nbd://example.com: Format driver 'IMGFMT' does not support image creation
+qemu-img: nbd://example.com: Format driver 'bochs' does not support image creation
 
 === Unsupported image creation in qemu-img convert ===
 
-qemu-img: Format driver 'IMGFMT' does not support image creation
+qemu-img: Format driver 'bochs' does not support image creation
 
 === Unsupported format in qemu-img amend ===
 
-qemu-img: Format driver 'IMGFMT' does not support any options to amend
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1048576
+qemu-img: Format driver 'IMGFMT' does not support option amendment
 
 *** done
-- 
2.14.3

Re: [Qemu-devel] [Qemu-block] [PATCH 7/7] iotests: Rework 113

[John Snow]

On 04/21/2018 12:54 PM, Max Reitz wrote:
> This test case has been broken since 398e6ad014df261d (roughly half a
> year).  qemu-img amend requires its output image to be R/W, so it opens
> it as such; the node is then turned into an read-only node automatically
> which is now accompanied by a warning, however.  This warning has not
> been part of the reference output.
> 
> For one thing, this warning shows that we cannot keep the test case as
> it is.  We would need a format that has no create_opts but that does
> have write support -- we do not have such a format, though.
> 
> Another thing is that qemu now actually checks whether an image format
> supports amendment instead of whether it has create_opts (since the
> former always implies the latter).  So we can now use any format that
> does not support amendment (even if it supports creation) and thus test
> the same code path.
> 
> The reason nobody has noticed the breakage until now of course is the
> fact that nobody runs the iotests for nbd+bochs.  There actually was
> never any reason to set the protocol to "nbd" but because that was
> technically correct; functionally it made no difference.  So that is the
> first thing we are going to change: Make the protocol "file" instead so
> that people might actually notice breakage here.
> 
> Secondly, now that bochs no longer works for the amend test case, we
> have to change the format there anyway.  Set let us just bend the truth
> a bit, declare this test a raw test.  In fact, that does not even
> concern the bochs test cases, other than the output now reading 'bochs'
> instead of 'IMGFMT'.
> 
> So with this test now being a raw test, we can rework the amend test
> case to use raw instead.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>

Well, it passes... Not sure if I'm wild about the format change, it
sounds like a failure of our CI more than something that needed to
change in the test, but... shrug.

Tested-by: John Snow <jsnow@redhat.com>

Re: [Qemu-devel] [Qemu-block] [PATCH 7/7] iotests: Rework 113

[Max Reitz]

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

On 2018-05-02 20:03, John Snow wrote:
> 
> 
> On 04/21/2018 12:54 PM, Max Reitz wrote:
>> This test case has been broken since 398e6ad014df261d (roughly half a
>> year).  qemu-img amend requires its output image to be R/W, so it opens
>> it as such; the node is then turned into an read-only node automatically
>> which is now accompanied by a warning, however.  This warning has not
>> been part of the reference output.
>>
>> For one thing, this warning shows that we cannot keep the test case as
>> it is.  We would need a format that has no create_opts but that does
>> have write support -- we do not have such a format, though.
>>
>> Another thing is that qemu now actually checks whether an image format
>> supports amendment instead of whether it has create_opts (since the
>> former always implies the latter).  So we can now use any format that
>> does not support amendment (even if it supports creation) and thus test
>> the same code path.
>>
>> The reason nobody has noticed the breakage until now of course is the
>> fact that nobody runs the iotests for nbd+bochs.  There actually was
>> never any reason to set the protocol to "nbd" but because that was
>> technically correct; functionally it made no difference.  So that is the
>> first thing we are going to change: Make the protocol "file" instead so
>> that people might actually notice breakage here.
>>
>> Secondly, now that bochs no longer works for the amend test case, we
>> have to change the format there anyway.  Set let us just bend the truth

I suppose s/Set/So/

>> a bit, declare this test a raw test.  In fact, that does not even
>> concern the bochs test cases, other than the output now reading 'bochs'
>> instead of 'IMGFMT'.
>>
>> So with this test now being a raw test, we can rework the amend test
>> case to use raw instead.
>>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
> 
> Well, it passes... Not sure if I'm wild about the format change, it
> sounds like a failure of our CI more than something that needed to
> change in the test, but... shrug.

I think it's not really an issue in the CI.  Testing all possible
combinations of protocols and formats seems too much to me.

I think it really is an issue in our test suite.  There are many tests
that work only with a single combination (numerous file+qcow2 tests, for
instance), so having our great test matrix capability is completely
useless for them.

Maybe tests should be able to offer a preferred format+protocol
(+options) combination?  Then, when you run check without any such
arguments, it would just run each test in its preferred mode.

Max


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

Re: [Qemu-devel] [Qemu-block] [PATCH 7/7] iotests: Rework 113

[John Snow]

On 05/02/2018 02:13 PM, Max Reitz wrote:
> On 2018-05-02 20:03, John Snow wrote:
>>
>>
>> On 04/21/2018 12:54 PM, Max Reitz wrote:
>>> This test case has been broken since 398e6ad014df261d (roughly half a
>>> year).  qemu-img amend requires its output image to be R/W, so it opens
>>> it as such; the node is then turned into an read-only node automatically
>>> which is now accompanied by a warning, however.  This warning has not
>>> been part of the reference output.
>>>
>>> For one thing, this warning shows that we cannot keep the test case as
>>> it is.  We would need a format that has no create_opts but that does
>>> have write support -- we do not have such a format, though.
>>>
>>> Another thing is that qemu now actually checks whether an image format
>>> supports amendment instead of whether it has create_opts (since the
>>> former always implies the latter).  So we can now use any format that
>>> does not support amendment (even if it supports creation) and thus test
>>> the same code path.
>>>
>>> The reason nobody has noticed the breakage until now of course is the
>>> fact that nobody runs the iotests for nbd+bochs.  There actually was
>>> never any reason to set the protocol to "nbd" but because that was
>>> technically correct; functionally it made no difference.  So that is the
>>> first thing we are going to change: Make the protocol "file" instead so
>>> that people might actually notice breakage here.
>>>
>>> Secondly, now that bochs no longer works for the amend test case, we
>>> have to change the format there anyway.  Set let us just bend the truth
> 
> I suppose s/Set/So/
> 
>>> a bit, declare this test a raw test.  In fact, that does not even
>>> concern the bochs test cases, other than the output now reading 'bochs'
>>> instead of 'IMGFMT'.
>>>
>>> So with this test now being a raw test, we can rework the amend test
>>> case to use raw instead.
>>>
>>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>>
>> Well, it passes... Not sure if I'm wild about the format change, it
>> sounds like a failure of our CI more than something that needed to
>> change in the test, but... shrug.
> 
> I think it's not really an issue in the CI.  Testing all possible
> combinations of protocols and formats seems too much to me.
> 
> I think it really is an issue in our test suite.  There are many tests
> that work only with a single combination (numerous file+qcow2 tests, for
> instance), so having our great test matrix capability is completely
> useless for them.
> 
> Maybe tests should be able to offer a preferred format+protocol
> (+options) combination?  Then, when you run check without any such
> arguments, it would just run each test in its preferred mode.
> 
> Max
> 

"You're not wrong" -- iotests presents an impossible matrix. You're not
going to fix our infrastructural problems with this one patch, which
looks like a hack in contrast to how the rest of iotests is presently
structured.

I don't oppose it, though. Just voicing my full-throated ambivalence.
There's definitely a discussion or two to be had about what
instrumenting iotests looks like and how we can ensure we're getting
proper coverage. The current solution *is* bad.

--js

Re: [Qemu-devel] [Qemu-block] [PATCH 7/7] iotests: Rework 113

[Max Reitz]

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

On 2018-05-02 20:18, John Snow wrote:
> 
> 
> On 05/02/2018 02:13 PM, Max Reitz wrote:
>> On 2018-05-02 20:03, John Snow wrote:
>>>
>>>
>>> On 04/21/2018 12:54 PM, Max Reitz wrote:
>>>> This test case has been broken since 398e6ad014df261d (roughly half a
>>>> year).  qemu-img amend requires its output image to be R/W, so it opens
>>>> it as such; the node is then turned into an read-only node automatically
>>>> which is now accompanied by a warning, however.  This warning has not
>>>> been part of the reference output.
>>>>
>>>> For one thing, this warning shows that we cannot keep the test case as
>>>> it is.  We would need a format that has no create_opts but that does
>>>> have write support -- we do not have such a format, though.
>>>>
>>>> Another thing is that qemu now actually checks whether an image format
>>>> supports amendment instead of whether it has create_opts (since the
>>>> former always implies the latter).  So we can now use any format that
>>>> does not support amendment (even if it supports creation) and thus test
>>>> the same code path.
>>>>
>>>> The reason nobody has noticed the breakage until now of course is the
>>>> fact that nobody runs the iotests for nbd+bochs.  There actually was
>>>> never any reason to set the protocol to "nbd" but because that was
>>>> technically correct; functionally it made no difference.  So that is the
>>>> first thing we are going to change: Make the protocol "file" instead so
>>>> that people might actually notice breakage here.
>>>>
>>>> Secondly, now that bochs no longer works for the amend test case, we
>>>> have to change the format there anyway.  Set let us just bend the truth
>>
>> I suppose s/Set/So/
>>
>>>> a bit, declare this test a raw test.  In fact, that does not even
>>>> concern the bochs test cases, other than the output now reading 'bochs'
>>>> instead of 'IMGFMT'.
>>>>
>>>> So with this test now being a raw test, we can rework the amend test
>>>> case to use raw instead.
>>>>
>>>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>>>
>>> Well, it passes... Not sure if I'm wild about the format change, it
>>> sounds like a failure of our CI more than something that needed to
>>> change in the test, but... shrug.
>>
>> I think it's not really an issue in the CI.  Testing all possible
>> combinations of protocols and formats seems too much to me.
>>
>> I think it really is an issue in our test suite.  There are many tests
>> that work only with a single combination (numerous file+qcow2 tests, for
>> instance), so having our great test matrix capability is completely
>> useless for them.
>>
>> Maybe tests should be able to offer a preferred format+protocol
>> (+options) combination?  Then, when you run check without any such
>> arguments, it would just run each test in its preferred mode.
>>
>> Max
>>
> 
> "You're not wrong" -- iotests presents an impossible matrix. You're not
> going to fix our infrastructural problems with this one patch, which
> looks like a hack in contrast to how the rest of iotests is presently
> structured.
> 
> I don't oppose it, though. Just voicing my full-throated ambivalence.
> There's definitely a discussion or two to be had about what
> instrumenting iotests looks like and how we can ensure we're getting
> proper coverage. The current solution *is* bad.

I do completely agree with you.  It's a hack.  That's why my
justification in the commit message is rather long.

And indeed I was thinking about what we could do to the iotests to get
around this.  I thought of maybe fixing some iotests to a specific
format+protocol combination and then always run them with that,
regardless of what the user specified -- but that would mean running
them always, which is stupid because it would take too much time for
every combination you want to test.

I didn't think of adding a "default" mode to the check script, though,
which would allow you to run that fixed combination only then.  (Though
you may want to be able to run such tests with different combinations,
too, though -- the "default" mode just ensures the test is run at all).

So my solution was to just hack around this with a questionable
(although not fully wrong) explanation; and to see which tests I
personally didn't run so far (and then added the proper combinations to
my test environment so they would run (except for some, e.g. the ones
requiring root)).  Then I decided that would be enough for me right now...

Max


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

Re: [Qemu-devel] [PATCH 7/7] iotests: Rework 113

[Eric Blake]

On 04/21/2018 11:54 AM, Max Reitz wrote:
> This test case has been broken since 398e6ad014df261d (roughly half a
> year).  qemu-img amend requires its output image to be R/W, so it opens
> it as such; the node is then turned into an read-only node automatically
> which is now accompanied by a warning, however.  This warning has not
> been part of the reference output.
> 

> The reason nobody has noticed the breakage until now of course is the
> fact that nobody runs the iotests for nbd+bochs.  There actually was
> never any reason to set the protocol to "nbd" but because that was
> technically correct; functionally it made no difference.  So that is the
> first thing we are going to change: Make the protocol "file" instead so
> that people might actually notice breakage here.

There's probably several tests that fail with an explicit pairing of nbd 
as protocol + explicit format (I'm guilty of usually testing either the 
format './check -qcow2' or the protocol './check -nbd', and not 
combining the two).

> So with this test now being a raw test, we can rework the amend test
> case to use raw instead.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>   tests/qemu-iotests/113     | 19 +++++++++----------
>   tests/qemu-iotests/113.out |  7 ++++---
>   2 files changed, 13 insertions(+), 13 deletions(-)
> 
> diff --git a/tests/qemu-iotests/113 b/tests/qemu-iotests/113
> index 19b68b2727..4e09810905 100755
> --- a/tests/qemu-iotests/113
> +++ b/tests/qemu-iotests/113
> @@ -38,16 +38,17 @@ trap "_cleanup; exit \$status" 0 1 2 3 15
>   . ./common.rc
>   . ./common.filter
>   
> -# We can only test one format here because we need its sample file
> -_supported_fmt bochs
> -_supported_proto nbd
> +# Some of these test cases use bochs, but others do use raw, so this
> +# is only half a lie.
> +_supported_fmt raw
> +_supported_proto file

Naive question - can we pass a list to _supported_fmt/_supported_proto, 
in which case the test runs under both elements of the list?  That is, 
I'm wondering if:

_supported_fmt raw bochs
_supported_proto file nbd

gives us any additional coverage (theoretically 4 times the testing 
possibilities - but if the rest of the test hard-codes a particular 
format or protocol then it might not actually execute a different 
combination).

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

Re: [Qemu-devel] [PATCH 7/7] iotests: Rework 113

[Max Reitz]

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

On 2018-05-02 20:24, Eric Blake wrote:
> On 04/21/2018 11:54 AM, Max Reitz wrote:
>> This test case has been broken since 398e6ad014df261d (roughly half a
>> year).  qemu-img amend requires its output image to be R/W, so it opens
>> it as such; the node is then turned into an read-only node automatically
>> which is now accompanied by a warning, however.  This warning has not
>> been part of the reference output.
>>
> 
>> The reason nobody has noticed the breakage until now of course is the
>> fact that nobody runs the iotests for nbd+bochs.  There actually was
>> never any reason to set the protocol to "nbd" but because that was
>> technically correct; functionally it made no difference.  So that is the
>> first thing we are going to change: Make the protocol "file" instead so
>> that people might actually notice breakage here.
> 
> There's probably several tests that fail with an explicit pairing of nbd
> as protocol + explicit format (I'm guilty of usually testing either the
> format './check -qcow2' or the protocol './check -nbd', and not
> combining the two).
> 
>> So with this test now being a raw test, we can rework the amend test
>> case to use raw instead.
>>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> ---
>>   tests/qemu-iotests/113     | 19 +++++++++----------
>>   tests/qemu-iotests/113.out |  7 ++++---
>>   2 files changed, 13 insertions(+), 13 deletions(-)
>>
>> diff --git a/tests/qemu-iotests/113 b/tests/qemu-iotests/113
>> index 19b68b2727..4e09810905 100755
>> --- a/tests/qemu-iotests/113
>> +++ b/tests/qemu-iotests/113
>> @@ -38,16 +38,17 @@ trap "_cleanup; exit \$status" 0 1 2 3 15
>>   . ./common.rc
>>   . ./common.filter
>>   -# We can only test one format here because we need its sample file
>> -_supported_fmt bochs
>> -_supported_proto nbd
>> +# Some of these test cases use bochs, but others do use raw, so this
>> +# is only half a lie.
>> +_supported_fmt raw
>> +_supported_proto file
> 
> Naive question - can we pass a list to _supported_fmt/_supported_proto,
> in which case the test runs under both elements of the list?  That is,
> I'm wondering if:
> 
> _supported_fmt raw bochs
> _supported_proto file nbd
> 
> gives us any additional coverage (theoretically 4 times the testing
> possibilities - but if the rest of the test hard-codes a particular
> format or protocol then it might not actually execute a different
> combination).

Well, anyone who's going to run any of these combinations is probably
going to run raw+file as well.  You could argue that if someone changes
something about NBD, they might just run raw+nbd; but then again, this
test doesn't really test nbd at all.  In fact, it just tests qemu-img
(we simply need a format that does not support image creation, but it's
not like we actually do anything with that format).  Same for bochs.

(And I think anyone who wants to test qemu-img is indeed going to run
the raw+file tests.)

But see my reply to John, I think the best way would be if tests like
these could be just run in "default" mode without forcing some
format/protocol combination on them.

Max


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