All of lore.kernel.org
 help / color / mirror / Atom feed
* [Qemu-devel] [PATCH v2 0/7] qemu-img: Improve option help for amend
@ 2018-05-09 21:00 Max Reitz
  2018-05-09 21:00 ` [Qemu-devel] [PATCH v2 1/7] qemu-img: Amendment support implies create_opts Max Reitz
                   ` (7 more replies)
  0 siblings, 8 replies; 11+ messages in thread
From: Max Reitz @ 2018-05-09 21:00 UTC (permalink / raw)
  To: qemu-block; +Cc: qemu-devel, Max Reitz, John Snow, Eric Blake, Kevin Wolf

[Unchanged text from v1 ahead]

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.


v2:
- Patch 2: [Eric]
  - Use error_setg_errno() where possible
  - Use assertions instead of error messages where that makes sense
  - Rephrase an error message since we're already touching it


git-backport-diff against v1:

Key:
[----] : patches are identical
[####] : number of functional differences between upstream/downstream patch
[down] : patch is downstream-only
The flags [FC] indicate (F)unctional and (C)ontextual differences, respectively

001/7:[----] [--] 'qemu-img: Amendment support implies create_opts'
002/7:[0042] [FC] 'block: Add Error parameter to bdrv_amend_options'
003/7:[----] [--] 'qemu-option: Pull out "Supported options" print'
004/7:[----] [--] 'qemu-img: Add print_amend_option_help()'
005/7:[----] [--] 'qemu-img: Recognize no creation support in -o help'
006/7:[----] [--] 'iotests: Test help option for unsupporting formats'
007/7:[----] [--] 'iotests: Rework 113'


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, 159 insertions(+), 86 deletions(-)

-- 
2.14.3

^ permalink raw reply	[flat|nested] 11+ messages in thread

* [Qemu-devel] [PATCH v2 1/7] qemu-img: Amendment support implies create_opts
  2018-05-09 21:00 [Qemu-devel] [PATCH v2 0/7] qemu-img: Improve option help for amend Max Reitz
@ 2018-05-09 21:00 ` Max Reitz
  2018-05-09 21:00 ` [Qemu-devel] [PATCH v2 2/7] block: Add Error parameter to bdrv_amend_options Max Reitz
                   ` (6 subsequent siblings)
  7 siblings, 0 replies; 11+ messages in thread
From: Max Reitz @ 2018-05-09 21:00 UTC (permalink / raw)
  To: qemu-block; +Cc: qemu-devel, Max Reitz, John Snow, Eric Blake, Kevin Wolf

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>
Reviewed-by: John Snow <jsnow@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 qemu-img.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/qemu-img.c b/qemu-img.c
index 62b29e7feb..f38aa51166 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -3740,13 +3740,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

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [Qemu-devel] [PATCH v2 2/7] block: Add Error parameter to bdrv_amend_options
  2018-05-09 21:00 [Qemu-devel] [PATCH v2 0/7] qemu-img: Improve option help for amend Max Reitz
  2018-05-09 21:00 ` [Qemu-devel] [PATCH v2 1/7] qemu-img: Amendment support implies create_opts Max Reitz
@ 2018-05-09 21:00 ` Max Reitz
  2018-05-09 21:20   ` Eric Blake
  2018-05-09 22:57   ` John Snow
  2018-05-09 21:00 ` [Qemu-devel] [PATCH v2 3/7] qemu-option: Pull out "Supported options" print Max Reitz
                   ` (5 subsequent siblings)
  7 siblings, 2 replies; 11+ messages in thread
From: Max Reitz @ 2018-05-09 21:00 UTC (permalink / raw)
  To: qemu-block; +Cc: qemu-devel, Max Reitz, John Snow, Eric Blake, Kevin Wolf

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

Along the way, turn the target_version/current_version comparisons at
the beginning of qcow2_downgrade() into assertions (the caller has to
make sure these conditions are met), and rephrase the error message on
using compat=1.1 to get refcount widths other than 16 bits.

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, 57 insertions(+), 51 deletions(-)

diff --git a/include/block/block.h b/include/block/block.h
index 3894edda9d..30f2a3364b 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -343,7 +343,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 76b589da57..92b0807c9a 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -322,7 +322,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 676e57f562..1a73c6a476 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 6d532470a8..9106512f97 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -4049,22 +4049,21 @@ 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;
     int ret;
 
-    if (target_version == current_version) {
-        return 0;
-    } else if (target_version > current_version) {
-        return -EINVAL;
-    } else if (target_version != 2) {
-        return -EINVAL;
-    }
+    /* This is qcow2_downgrade(), not qcow2_upgrade() */
+    assert(target_version < current_version);
+
+    /* There are no other versions (now) that you can downgrade to */
+    assert(target_version == 2);
 
     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 +4071,7 @@ 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_errno(errp, -ret, "Failed to make the image clean");
             return ret;
         }
     }
@@ -4081,6 +4081,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 +4096,7 @@ static int qcow2_downgrade(BlockDriverState *bs, int target_version,
 
     ret = qcow2_expand_zero_clusters(bs, status_cb, cb_opaque);
     if (ret < 0) {
+        error_setg_errno(errp, -ret, "Failed to turn zero into data clusters");
         return ret;
     }
 
@@ -4101,6 +4104,7 @@ static int qcow2_downgrade(BlockDriverState *bs, int target_version,
     ret = qcow2_update_header(bs);
     if (ret < 0) {
         s->qcow_version = current_version;
+        error_setg_errno(errp, -ret, "Failed to update the image header");
         return ret;
     }
     return 0;
@@ -4178,7 +4182,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 +4195,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 +4215,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 +4232,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 +4241,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 +4266,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 +4292,7 @@ static int qcow2_amend_options(BlockDriverState *bs, QemuOpts *opts,
         ret = qcow2_update_header(bs);
         if (ret < 0) {
             s->qcow_version = old_version;
+            error_setg_errno(errp, -ret, "Failed to update the image header");
             return ret;
         }
     }
@@ -4293,18 +4301,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, "Refcount widths other 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 +4321,7 @@ 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_errno(errp, -ret, "Failed to change the backing file");
             return ret;
         }
     }
@@ -4321,14 +4329,16 @@ 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_errno(errp, -ret, "Failed to update the image header");
                 return ret;
             }
             s->use_lazy_refcounts = true;
@@ -4336,6 +4346,7 @@ static int qcow2_amend_options(BlockDriverState *bs, QemuOpts *opts,
             /* make image clean first */
             ret = qcow2_mark_clean(bs);
             if (ret < 0) {
+                error_setg_errno(errp, -ret, "Failed to make the image clean");
                 return ret;
             }
             /* now disallow lazy refcounts */
@@ -4343,6 +4354,7 @@ 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_errno(errp, -ret, "Failed to update the image header");
                 return ret;
             }
             s->use_lazy_refcounts = false;
@@ -4351,17 +4363,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 +4380,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 f38aa51166..347cf2ed9f 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -3761,10 +3761,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

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [Qemu-devel] [PATCH v2 3/7] qemu-option: Pull out "Supported options" print
  2018-05-09 21:00 [Qemu-devel] [PATCH v2 0/7] qemu-img: Improve option help for amend Max Reitz
  2018-05-09 21:00 ` [Qemu-devel] [PATCH v2 1/7] qemu-img: Amendment support implies create_opts Max Reitz
  2018-05-09 21:00 ` [Qemu-devel] [PATCH v2 2/7] block: Add Error parameter to bdrv_amend_options Max Reitz
@ 2018-05-09 21:00 ` Max Reitz
  2018-05-09 21:00 ` [Qemu-devel] [PATCH v2 4/7] qemu-img: Add print_amend_option_help() Max Reitz
                   ` (4 subsequent siblings)
  7 siblings, 0 replies; 11+ messages in thread
From: Max Reitz @ 2018-05-09 21:00 UTC (permalink / raw)
  To: qemu-block; +Cc: qemu-devel, Max Reitz, John Snow, Eric Blake, Kevin Wolf

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

Signed-off-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: John Snow <jsnow@redhat.com>
Reviewed-by: Eric Blake <eblake@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 347cf2ed9f..c84d047872 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

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [Qemu-devel] [PATCH v2 4/7] qemu-img: Add print_amend_option_help()
  2018-05-09 21:00 [Qemu-devel] [PATCH v2 0/7] qemu-img: Improve option help for amend Max Reitz
                   ` (2 preceding siblings ...)
  2018-05-09 21:00 ` [Qemu-devel] [PATCH v2 3/7] qemu-option: Pull out "Supported options" print Max Reitz
@ 2018-05-09 21:00 ` Max Reitz
  2018-05-09 21:00 ` [Qemu-devel] [PATCH v2 5/7] qemu-img: Recognize no creation support in -o help Max Reitz
                   ` (3 subsequent siblings)
  7 siblings, 0 replies; 11+ messages in thread
From: Max Reitz @ 2018-05-09 21:00 UTC (permalink / raw)
  To: qemu-block; +Cc: qemu-devel, Max Reitz, John Snow, Eric Blake, Kevin Wolf

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>
Reviewed-by: John Snow <jsnow@redhat.com>
Reviewed-by: Eric Blake <eblake@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 c84d047872..a1ae718a52 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -3609,6 +3609,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;
@@ -3708,7 +3734,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;
     }
 
@@ -3737,7 +3763,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

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [Qemu-devel] [PATCH v2 5/7] qemu-img: Recognize no creation support in -o help
  2018-05-09 21:00 [Qemu-devel] [PATCH v2 0/7] qemu-img: Improve option help for amend Max Reitz
                   ` (3 preceding siblings ...)
  2018-05-09 21:00 ` [Qemu-devel] [PATCH v2 4/7] qemu-img: Add print_amend_option_help() Max Reitz
@ 2018-05-09 21:00 ` Max Reitz
  2018-05-09 21:00 ` [Qemu-devel] [PATCH v2 6/7] iotests: Test help option for unsupporting formats Max Reitz
                   ` (2 subsequent siblings)
  7 siblings, 0 replies; 11+ messages in thread
From: Max Reitz @ 2018-05-09 21:00 UTC (permalink / raw)
  To: qemu-block; +Cc: qemu-devel, Max Reitz, John Snow, Eric Blake, Kevin Wolf

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>
Reviewed-by: John Snow <jsnow@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 qemu-img.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/qemu-img.c b/qemu-img.c
index a1ae718a52..81ad1e040c 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

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [Qemu-devel] [PATCH v2 6/7] iotests: Test help option for unsupporting formats
  2018-05-09 21:00 [Qemu-devel] [PATCH v2 0/7] qemu-img: Improve option help for amend Max Reitz
                   ` (4 preceding siblings ...)
  2018-05-09 21:00 ` [Qemu-devel] [PATCH v2 5/7] qemu-img: Recognize no creation support in -o help Max Reitz
@ 2018-05-09 21:00 ` Max Reitz
  2018-05-09 21:00 ` [Qemu-devel] [PATCH v2 7/7] iotests: Rework 113 Max Reitz
  2018-06-01 11:06 ` [Qemu-devel] [PATCH v2 0/7] qemu-img: Improve option help for amend Max Reitz
  7 siblings, 0 replies; 11+ messages in thread
From: Max Reitz @ 2018-05-09 21:00 UTC (permalink / raw)
  To: qemu-block; +Cc: qemu-devel, Max Reitz, John Snow, Eric Blake, Kevin Wolf

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>
Reviewed-by: Eric Blake <eblake@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

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [Qemu-devel] [PATCH v2 7/7] iotests: Rework 113
  2018-05-09 21:00 [Qemu-devel] [PATCH v2 0/7] qemu-img: Improve option help for amend Max Reitz
                   ` (5 preceding siblings ...)
  2018-05-09 21:00 ` [Qemu-devel] [PATCH v2 6/7] iotests: Test help option for unsupporting formats Max Reitz
@ 2018-05-09 21:00 ` Max Reitz
  2018-06-01 11:06 ` [Qemu-devel] [PATCH v2 0/7] qemu-img: Improve option help for amend Max Reitz
  7 siblings, 0 replies; 11+ messages in thread
From: Max Reitz @ 2018-05-09 21:00 UTC (permalink / raw)
  To: qemu-block; +Cc: qemu-devel, Max Reitz, John Snow, Eric Blake, Kevin Wolf

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>
Reviewed-by: John Snow <jsnow@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

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [Qemu-devel] [PATCH v2 2/7] block: Add Error parameter to bdrv_amend_options
  2018-05-09 21:00 ` [Qemu-devel] [PATCH v2 2/7] block: Add Error parameter to bdrv_amend_options Max Reitz
@ 2018-05-09 21:20   ` Eric Blake
  2018-05-09 22:57   ` John Snow
  1 sibling, 0 replies; 11+ messages in thread
From: Eric Blake @ 2018-05-09 21:20 UTC (permalink / raw)
  To: Max Reitz, qemu-block; +Cc: qemu-devel, John Snow, Kevin Wolf

On 05/09/2018 04:00 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.
> 
> Along the way, turn the target_version/current_version comparisons at
> the beginning of qcow2_downgrade() into assertions (the caller has to
> make sure these conditions are met), and rephrase the error message on
> using compat=1.1 to get refcount widths other than 16 bits.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---

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

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [Qemu-devel] [PATCH v2 2/7] block: Add Error parameter to bdrv_amend_options
  2018-05-09 21:00 ` [Qemu-devel] [PATCH v2 2/7] block: Add Error parameter to bdrv_amend_options Max Reitz
  2018-05-09 21:20   ` Eric Blake
@ 2018-05-09 22:57   ` John Snow
  1 sibling, 0 replies; 11+ messages in thread
From: John Snow @ 2018-05-09 22:57 UTC (permalink / raw)
  To: Max Reitz, qemu-block; +Cc: Kevin Wolf, qemu-devel



On 05/09/2018 05:00 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.
> 
> Along the way, turn the target_version/current_version comparisons at
> the beginning of qcow2_downgrade() into assertions (the caller has to
> make sure these conditions are met), and rephrase the error message on
> using compat=1.1 to get refcount widths other than 16 bits.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>

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

thx@u

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [Qemu-devel] [PATCH v2 0/7] qemu-img: Improve option help for amend
  2018-05-09 21:00 [Qemu-devel] [PATCH v2 0/7] qemu-img: Improve option help for amend Max Reitz
                   ` (6 preceding siblings ...)
  2018-05-09 21:00 ` [Qemu-devel] [PATCH v2 7/7] iotests: Rework 113 Max Reitz
@ 2018-06-01 11:06 ` Max Reitz
  7 siblings, 0 replies; 11+ messages in thread
From: Max Reitz @ 2018-06-01 11:06 UTC (permalink / raw)
  To: qemu-block; +Cc: qemu-devel, John Snow, Eric Blake, Kevin Wolf

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

On 2018-05-09 23:00, Max Reitz wrote:
> [Unchanged text from v1 ahead]
> 
> 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.
> 
> 
> v2:
> - Patch 2: [Eric]
>   - Use error_setg_errno() where possible
>   - Use assertions instead of error messages where that makes sense
>   - Rephrase an error message since we're already touching it
> 
> 
> git-backport-diff against v1:
> 
> Key:
> [----] : patches are identical
> [####] : number of functional differences between upstream/downstream patch
> [down] : patch is downstream-only
> The flags [FC] indicate (F)unctional and (C)ontextual differences, respectively
> 
> 001/7:[----] [--] 'qemu-img: Amendment support implies create_opts'
> 002/7:[0042] [FC] 'block: Add Error parameter to bdrv_amend_options'
> 003/7:[----] [--] 'qemu-option: Pull out "Supported options" print'
> 004/7:[----] [--] 'qemu-img: Add print_amend_option_help()'
> 005/7:[----] [--] 'qemu-img: Recognize no creation support in -o help'
> 006/7:[----] [--] 'iotests: Test help option for unsupporting formats'
> 007/7:[----] [--] 'iotests: Rework 113'
> 
> 
> 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, 159 insertions(+), 86 deletions(-)

Applied to my block branch.

Max


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

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2018-06-01 11:06 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-05-09 21:00 [Qemu-devel] [PATCH v2 0/7] qemu-img: Improve option help for amend Max Reitz
2018-05-09 21:00 ` [Qemu-devel] [PATCH v2 1/7] qemu-img: Amendment support implies create_opts Max Reitz
2018-05-09 21:00 ` [Qemu-devel] [PATCH v2 2/7] block: Add Error parameter to bdrv_amend_options Max Reitz
2018-05-09 21:20   ` Eric Blake
2018-05-09 22:57   ` John Snow
2018-05-09 21:00 ` [Qemu-devel] [PATCH v2 3/7] qemu-option: Pull out "Supported options" print Max Reitz
2018-05-09 21:00 ` [Qemu-devel] [PATCH v2 4/7] qemu-img: Add print_amend_option_help() Max Reitz
2018-05-09 21:00 ` [Qemu-devel] [PATCH v2 5/7] qemu-img: Recognize no creation support in -o help Max Reitz
2018-05-09 21:00 ` [Qemu-devel] [PATCH v2 6/7] iotests: Test help option for unsupporting formats Max Reitz
2018-05-09 21:00 ` [Qemu-devel] [PATCH v2 7/7] iotests: Rework 113 Max Reitz
2018-06-01 11:06 ` [Qemu-devel] [PATCH v2 0/7] qemu-img: Improve option help for amend Max Reitz

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.