* [Qemu-devel] [PATCH 0/7] qemu-img: Improve option help for amend
@ 2018-04-21 16:54 Max Reitz
2018-04-21 16:54 ` [Qemu-devel] [PATCH 1/7] qemu-img: Amendment support implies create_opts Max Reitz
` (6 more replies)
0 siblings, 7 replies; 24+ messages in thread
From: Max Reitz @ 2018-04-21 16:54 UTC (permalink / raw)
To: qemu-block; +Cc: qemu-devel, Max Reitz, Kevin Wolf
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
^ permalink raw reply [flat|nested] 24+ messages in thread
* [Qemu-devel] [PATCH 1/7] qemu-img: Amendment support implies create_opts
2018-04-21 16:54 [Qemu-devel] [PATCH 0/7] qemu-img: Improve option help for amend Max Reitz
@ 2018-04-21 16:54 ` Max Reitz
2018-05-02 17:35 ` Eric Blake
2018-04-21 16:54 ` [Qemu-devel] [PATCH 2/7] block: Add Error parameter to bdrv_amend_options Max Reitz
` (5 subsequent siblings)
6 siblings, 1 reply; 24+ messages in thread
From: Max Reitz @ 2018-04-21 16:54 UTC (permalink / raw)
To: qemu-block; +Cc: qemu-devel, Max Reitz, 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>
---
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
^ permalink raw reply related [flat|nested] 24+ messages in thread
* [Qemu-devel] [PATCH 2/7] block: Add Error parameter to bdrv_amend_options
2018-04-21 16:54 [Qemu-devel] [PATCH 0/7] qemu-img: Improve option help for amend Max Reitz
2018-04-21 16:54 ` [Qemu-devel] [PATCH 1/7] qemu-img: Amendment support implies create_opts Max Reitz
@ 2018-04-21 16:54 ` Max Reitz
2018-05-01 22:49 ` [Qemu-devel] [Qemu-block] " John Snow
2018-05-02 17:52 ` [Qemu-devel] " Eric Blake
2018-04-21 16:54 ` [Qemu-devel] [PATCH 3/7] qemu-option: Pull out "Supported options" print Max Reitz
` (4 subsequent siblings)
6 siblings, 2 replies; 24+ messages in thread
From: Max Reitz @ 2018-04-21 16:54 UTC (permalink / raw)
To: qemu-block; +Cc: qemu-devel, Max Reitz, 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.
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
^ permalink raw reply related [flat|nested] 24+ messages in thread
* [Qemu-devel] [PATCH 3/7] qemu-option: Pull out "Supported options" print
2018-04-21 16:54 [Qemu-devel] [PATCH 0/7] qemu-img: Improve option help for amend Max Reitz
2018-04-21 16:54 ` [Qemu-devel] [PATCH 1/7] qemu-img: Amendment support implies create_opts Max Reitz
2018-04-21 16:54 ` [Qemu-devel] [PATCH 2/7] block: Add Error parameter to bdrv_amend_options Max Reitz
@ 2018-04-21 16:54 ` Max Reitz
2018-05-02 18:09 ` Eric Blake
2018-04-21 16:54 ` [Qemu-devel] [PATCH 4/7] qemu-img: Add print_amend_option_help() Max Reitz
` (3 subsequent siblings)
6 siblings, 1 reply; 24+ messages in thread
From: Max Reitz @ 2018-04-21 16:54 UTC (permalink / raw)
To: qemu-block; +Cc: qemu-devel, Max Reitz, 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>
---
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
^ permalink raw reply related [flat|nested] 24+ messages in thread
* [Qemu-devel] [PATCH 4/7] qemu-img: Add print_amend_option_help()
2018-04-21 16:54 [Qemu-devel] [PATCH 0/7] qemu-img: Improve option help for amend Max Reitz
` (2 preceding siblings ...)
2018-04-21 16:54 ` [Qemu-devel] [PATCH 3/7] qemu-option: Pull out "Supported options" print Max Reitz
@ 2018-04-21 16:54 ` Max Reitz
2018-05-02 18:13 ` Eric Blake
2018-04-21 16:54 ` [Qemu-devel] [PATCH 5/7] qemu-img: Recognize no creation support in -o help Max Reitz
` (2 subsequent siblings)
6 siblings, 1 reply; 24+ messages in thread
From: Max Reitz @ 2018-04-21 16:54 UTC (permalink / raw)
To: qemu-block; +Cc: qemu-devel, Max Reitz, 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>
---
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
^ permalink raw reply related [flat|nested] 24+ messages in thread
* [Qemu-devel] [PATCH 5/7] qemu-img: Recognize no creation support in -o help
2018-04-21 16:54 [Qemu-devel] [PATCH 0/7] qemu-img: Improve option help for amend Max Reitz
` (3 preceding siblings ...)
2018-04-21 16:54 ` [Qemu-devel] [PATCH 4/7] qemu-img: Add print_amend_option_help() Max Reitz
@ 2018-04-21 16:54 ` Max Reitz
2018-05-02 17:40 ` [Qemu-devel] [Qemu-block] " John Snow
2018-05-02 18:15 ` [Qemu-devel] " Eric Blake
2018-04-21 16:54 ` [Qemu-devel] [PATCH 6/7] iotests: Test help option for unsupporting formats Max Reitz
2018-04-21 16:54 ` [Qemu-devel] [PATCH 7/7] iotests: Rework 113 Max Reitz
6 siblings, 2 replies; 24+ messages in thread
From: Max Reitz @ 2018-04-21 16:54 UTC (permalink / raw)
To: qemu-block; +Cc: qemu-devel, Max Reitz, 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>
---
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
^ permalink raw reply related [flat|nested] 24+ messages in thread
* [Qemu-devel] [PATCH 6/7] iotests: Test help option for unsupporting formats
2018-04-21 16:54 [Qemu-devel] [PATCH 0/7] qemu-img: Improve option help for amend Max Reitz
` (4 preceding siblings ...)
2018-04-21 16:54 ` [Qemu-devel] [PATCH 5/7] qemu-img: Recognize no creation support in -o help Max Reitz
@ 2018-04-21 16:54 ` Max Reitz
2018-05-02 18:00 ` [Qemu-devel] [Qemu-block] " John Snow
2018-05-02 18:17 ` [Qemu-devel] " Eric Blake
2018-04-21 16:54 ` [Qemu-devel] [PATCH 7/7] iotests: Rework 113 Max Reitz
6 siblings, 2 replies; 24+ messages in thread
From: Max Reitz @ 2018-04-21 16:54 UTC (permalink / raw)
To: qemu-block; +Cc: qemu-devel, Max Reitz, 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>
---
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] 24+ messages in thread
* [Qemu-devel] [PATCH 7/7] iotests: Rework 113
2018-04-21 16:54 [Qemu-devel] [PATCH 0/7] qemu-img: Improve option help for amend Max Reitz
` (5 preceding siblings ...)
2018-04-21 16:54 ` [Qemu-devel] [PATCH 6/7] iotests: Test help option for unsupporting formats Max Reitz
@ 2018-04-21 16:54 ` Max Reitz
2018-05-02 18:03 ` [Qemu-devel] [Qemu-block] " John Snow
2018-05-02 18:24 ` [Qemu-devel] " Eric Blake
6 siblings, 2 replies; 24+ messages in thread
From: Max Reitz @ 2018-04-21 16:54 UTC (permalink / raw)
To: qemu-block; +Cc: qemu-devel, Max Reitz, 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>
---
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] 24+ messages in thread
* Re: [Qemu-devel] [Qemu-block] [PATCH 2/7] block: Add Error parameter to bdrv_amend_options
2018-04-21 16:54 ` [Qemu-devel] [PATCH 2/7] block: Add Error parameter to bdrv_amend_options Max Reitz
@ 2018-05-01 22:49 ` John Snow
2018-05-02 17:52 ` [Qemu-devel] " Eric Blake
1 sibling, 0 replies; 24+ messages in thread
From: John Snow @ 2018-05-01 22:49 UTC (permalink / raw)
To: Max Reitz, qemu-block; +Cc: Kevin Wolf, qemu-devel
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.]
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [Qemu-devel] [PATCH 1/7] qemu-img: Amendment support implies create_opts
2018-04-21 16:54 ` [Qemu-devel] [PATCH 1/7] qemu-img: Amendment support implies create_opts Max Reitz
@ 2018-05-02 17:35 ` Eric Blake
0 siblings, 0 replies; 24+ messages in thread
From: Eric Blake @ 2018-05-02 17:35 UTC (permalink / raw)
To: Max Reitz, qemu-block; +Cc: Kevin Wolf, qemu-devel
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
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [Qemu-devel] [Qemu-block] [PATCH 5/7] qemu-img: Recognize no creation support in -o help
2018-04-21 16:54 ` [Qemu-devel] [PATCH 5/7] qemu-img: Recognize no creation support in -o help Max Reitz
@ 2018-05-02 17:40 ` John Snow
2018-05-02 18:15 ` [Qemu-devel] " Eric Blake
1 sibling, 0 replies; 24+ messages in thread
From: John Snow @ 2018-05-02 17:40 UTC (permalink / raw)
To: Max Reitz, qemu-block; +Cc: Kevin Wolf, qemu-devel
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>
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [Qemu-devel] [PATCH 2/7] block: Add Error parameter to bdrv_amend_options
2018-04-21 16:54 ` [Qemu-devel] [PATCH 2/7] block: Add Error parameter to bdrv_amend_options Max Reitz
2018-05-01 22:49 ` [Qemu-devel] [Qemu-block] " John Snow
@ 2018-05-02 17:52 ` Eric Blake
2018-05-02 18:08 ` Max Reitz
1 sibling, 1 reply; 24+ messages in thread
From: Eric Blake @ 2018-05-02 17:52 UTC (permalink / raw)
To: Max Reitz, qemu-block; +Cc: Kevin Wolf, qemu-devel
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
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [Qemu-devel] [Qemu-block] [PATCH 6/7] iotests: Test help option for unsupporting formats
2018-04-21 16:54 ` [Qemu-devel] [PATCH 6/7] iotests: Test help option for unsupporting formats Max Reitz
@ 2018-05-02 18:00 ` John Snow
2018-05-02 18:17 ` [Qemu-devel] " Eric Blake
1 sibling, 0 replies; 24+ messages in thread
From: John Snow @ 2018-05-02 18:00 UTC (permalink / raw)
To: Max Reitz, qemu-block; +Cc: Kevin Wolf, qemu-devel
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>
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [Qemu-devel] [Qemu-block] [PATCH 7/7] iotests: Rework 113
2018-04-21 16:54 ` [Qemu-devel] [PATCH 7/7] iotests: Rework 113 Max Reitz
@ 2018-05-02 18:03 ` John Snow
2018-05-02 18:13 ` Max Reitz
2018-05-02 18:24 ` [Qemu-devel] " Eric Blake
1 sibling, 1 reply; 24+ messages in thread
From: John Snow @ 2018-05-02 18:03 UTC (permalink / raw)
To: Max Reitz, qemu-block; +Cc: Kevin Wolf, qemu-devel
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>
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [Qemu-devel] [PATCH 2/7] block: Add Error parameter to bdrv_amend_options
2018-05-02 17:52 ` [Qemu-devel] " Eric Blake
@ 2018-05-02 18:08 ` Max Reitz
0 siblings, 0 replies; 24+ messages in thread
From: Max Reitz @ 2018-05-02 18:08 UTC (permalink / raw)
To: Eric Blake, qemu-block; +Cc: Kevin Wolf, qemu-devel
[-- 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 --]
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [Qemu-devel] [PATCH 3/7] qemu-option: Pull out "Supported options" print
2018-04-21 16:54 ` [Qemu-devel] [PATCH 3/7] qemu-option: Pull out "Supported options" print Max Reitz
@ 2018-05-02 18:09 ` Eric Blake
0 siblings, 0 replies; 24+ messages in thread
From: Eric Blake @ 2018-05-02 18:09 UTC (permalink / raw)
To: Max Reitz, qemu-block; +Cc: Kevin Wolf, qemu-devel
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
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [Qemu-devel] [PATCH 4/7] qemu-img: Add print_amend_option_help()
2018-04-21 16:54 ` [Qemu-devel] [PATCH 4/7] qemu-img: Add print_amend_option_help() Max Reitz
@ 2018-05-02 18:13 ` Eric Blake
0 siblings, 0 replies; 24+ messages in thread
From: Eric Blake @ 2018-05-02 18:13 UTC (permalink / raw)
To: Max Reitz, qemu-block; +Cc: Kevin Wolf, qemu-devel
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
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [Qemu-devel] [Qemu-block] [PATCH 7/7] iotests: Rework 113
2018-05-02 18:03 ` [Qemu-devel] [Qemu-block] " John Snow
@ 2018-05-02 18:13 ` Max Reitz
2018-05-02 18:18 ` John Snow
0 siblings, 1 reply; 24+ messages in thread
From: Max Reitz @ 2018-05-02 18:13 UTC (permalink / raw)
To: John Snow, qemu-block; +Cc: Kevin Wolf, qemu-devel
[-- 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 --]
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [Qemu-devel] [PATCH 5/7] qemu-img: Recognize no creation support in -o help
2018-04-21 16:54 ` [Qemu-devel] [PATCH 5/7] qemu-img: Recognize no creation support in -o help Max Reitz
2018-05-02 17:40 ` [Qemu-devel] [Qemu-block] " John Snow
@ 2018-05-02 18:15 ` Eric Blake
1 sibling, 0 replies; 24+ messages in thread
From: Eric Blake @ 2018-05-02 18:15 UTC (permalink / raw)
To: Max Reitz, qemu-block; +Cc: Kevin Wolf, qemu-devel
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
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [Qemu-devel] [PATCH 6/7] iotests: Test help option for unsupporting formats
2018-04-21 16:54 ` [Qemu-devel] [PATCH 6/7] iotests: Test help option for unsupporting formats Max Reitz
2018-05-02 18:00 ` [Qemu-devel] [Qemu-block] " John Snow
@ 2018-05-02 18:17 ` Eric Blake
1 sibling, 0 replies; 24+ messages in thread
From: Eric Blake @ 2018-05-02 18:17 UTC (permalink / raw)
To: Max Reitz, qemu-block; +Cc: Kevin Wolf, qemu-devel
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
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [Qemu-devel] [Qemu-block] [PATCH 7/7] iotests: Rework 113
2018-05-02 18:13 ` Max Reitz
@ 2018-05-02 18:18 ` John Snow
2018-05-02 18:48 ` Max Reitz
0 siblings, 1 reply; 24+ messages in thread
From: John Snow @ 2018-05-02 18:18 UTC (permalink / raw)
To: Max Reitz, qemu-block; +Cc: Kevin Wolf, qemu-devel
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
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [Qemu-devel] [PATCH 7/7] iotests: Rework 113
2018-04-21 16:54 ` [Qemu-devel] [PATCH 7/7] iotests: Rework 113 Max Reitz
2018-05-02 18:03 ` [Qemu-devel] [Qemu-block] " John Snow
@ 2018-05-02 18:24 ` Eric Blake
2018-05-02 18:35 ` Max Reitz
1 sibling, 1 reply; 24+ messages in thread
From: Eric Blake @ 2018-05-02 18:24 UTC (permalink / raw)
To: Max Reitz, qemu-block; +Cc: Kevin Wolf, qemu-devel
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
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [Qemu-devel] [PATCH 7/7] iotests: Rework 113
2018-05-02 18:24 ` [Qemu-devel] " Eric Blake
@ 2018-05-02 18:35 ` Max Reitz
0 siblings, 0 replies; 24+ messages in thread
From: Max Reitz @ 2018-05-02 18:35 UTC (permalink / raw)
To: Eric Blake, qemu-block; +Cc: Kevin Wolf, qemu-devel
[-- 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 --]
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [Qemu-devel] [Qemu-block] [PATCH 7/7] iotests: Rework 113
2018-05-02 18:18 ` John Snow
@ 2018-05-02 18:48 ` Max Reitz
0 siblings, 0 replies; 24+ messages in thread
From: Max Reitz @ 2018-05-02 18:48 UTC (permalink / raw)
To: John Snow, qemu-block; +Cc: Kevin Wolf, qemu-devel
[-- 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 --]
^ permalink raw reply [flat|nested] 24+ messages in thread
end of thread, other threads:[~2018-05-02 18:49 UTC | newest]
Thread overview: 24+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-04-21 16:54 [Qemu-devel] [PATCH 0/7] qemu-img: Improve option help for amend Max Reitz
2018-04-21 16:54 ` [Qemu-devel] [PATCH 1/7] qemu-img: Amendment support implies create_opts Max Reitz
2018-05-02 17:35 ` Eric Blake
2018-04-21 16:54 ` [Qemu-devel] [PATCH 2/7] block: Add Error parameter to bdrv_amend_options Max Reitz
2018-05-01 22:49 ` [Qemu-devel] [Qemu-block] " John Snow
2018-05-02 17:52 ` [Qemu-devel] " Eric Blake
2018-05-02 18:08 ` Max Reitz
2018-04-21 16:54 ` [Qemu-devel] [PATCH 3/7] qemu-option: Pull out "Supported options" print Max Reitz
2018-05-02 18:09 ` Eric Blake
2018-04-21 16:54 ` [Qemu-devel] [PATCH 4/7] qemu-img: Add print_amend_option_help() Max Reitz
2018-05-02 18:13 ` Eric Blake
2018-04-21 16:54 ` [Qemu-devel] [PATCH 5/7] qemu-img: Recognize no creation support in -o help Max Reitz
2018-05-02 17:40 ` [Qemu-devel] [Qemu-block] " John Snow
2018-05-02 18:15 ` [Qemu-devel] " Eric Blake
2018-04-21 16:54 ` [Qemu-devel] [PATCH 6/7] iotests: Test help option for unsupporting formats Max Reitz
2018-05-02 18:00 ` [Qemu-devel] [Qemu-block] " John Snow
2018-05-02 18:17 ` [Qemu-devel] " Eric Blake
2018-04-21 16:54 ` [Qemu-devel] [PATCH 7/7] iotests: Rework 113 Max Reitz
2018-05-02 18:03 ` [Qemu-devel] [Qemu-block] " John Snow
2018-05-02 18:13 ` Max Reitz
2018-05-02 18:18 ` John Snow
2018-05-02 18:48 ` Max Reitz
2018-05-02 18:24 ` [Qemu-devel] " Eric Blake
2018-05-02 18:35 ` 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.