block.c | 10 ++++++++-- tests/qemu-iotests/049 | 5 +++++ tests/qemu-iotests/049.out | 5 +++++ 3 files changed, 18 insertions(+), 2 deletions(-)
If both the create options (qemu-img create -o ...) and the size
parameter were given, the size parameter was silently ignored. Instead,
make specifying two sizes an error.
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
block.c | 10 ++++++++--
tests/qemu-iotests/049 | 5 +++++
tests/qemu-iotests/049.out | 5 +++++
3 files changed, 18 insertions(+), 2 deletions(-)
diff --git a/block.c b/block.c
index 473eb6eeaa..73029fad64 100644
--- a/block.c
+++ b/block.c
@@ -5751,12 +5751,11 @@ void bdrv_img_create(const char *filename, const char *fmt,
return;
}
+ /* Create parameter list */
create_opts = qemu_opts_append(create_opts, drv->create_opts);
create_opts = qemu_opts_append(create_opts, proto_drv->create_opts);
- /* Create parameter list with default values */
opts = qemu_opts_create(create_opts, NULL, 0, &error_abort);
- qemu_opt_set_number(opts, BLOCK_OPT_SIZE, img_size, &error_abort);
/* Parse -o options */
if (options) {
@@ -5766,6 +5765,13 @@ void bdrv_img_create(const char *filename, const char *fmt,
}
}
+ if (!qemu_opt_get(opts, BLOCK_OPT_SIZE)) {
+ qemu_opt_set_number(opts, BLOCK_OPT_SIZE, img_size, &error_abort);
+ } else if (img_size != UINT64_C(-1)) {
+ error_setg(errp, "The image size must be specified only once");
+ goto out;
+ }
+
if (base_filename) {
qemu_opt_set(opts, BLOCK_OPT_BACKING_FILE, base_filename, &local_err);
if (local_err) {
diff --git a/tests/qemu-iotests/049 b/tests/qemu-iotests/049
index c100d30ed0..051a1c79e0 100755
--- a/tests/qemu-iotests/049
+++ b/tests/qemu-iotests/049
@@ -78,6 +78,11 @@ for s in $sizes; do
test_qemu_img create -f $IMGFMT -o size=$s "$TEST_IMG"
done
+echo "== 4. Specify size twice (-o and traditional parameter) =="
+echo
+
+test_qemu_img create -f $IMGFMT -o size=10M "$TEST_IMG" 20M
+
echo "== Check correct interpretation of suffixes for cluster size =="
echo
sizes="1024 1024b 1k 1K 1M "
diff --git a/tests/qemu-iotests/049.out b/tests/qemu-iotests/049.out
index 6b505408dd..affa55b341 100644
--- a/tests/qemu-iotests/049.out
+++ b/tests/qemu-iotests/049.out
@@ -121,6 +121,11 @@ qemu-img: TEST_DIR/t.qcow2: Parameter 'size' expects a non-negative number below
Optional suffix k, M, G, T, P or E means kilo-, mega-, giga-, tera-, peta-
and exabytes, respectively.
+== 4. Specify size twice (-o and traditional parameter) ==
+
+qemu-img create -f qcow2 -o size=10M TEST_DIR/t.qcow2 20M
+qemu-img: TEST_DIR/t.qcow2: The image size must be specified only once
+
== Check correct interpretation of suffixes for cluster size ==
qemu-img create -f qcow2 -o cluster_size=1024 TEST_DIR/t.qcow2 64M
--
2.20.1
On 11/26/19 9:48 AM, Kevin Wolf wrote: > If both the create options (qemu-img create -o ...) and the size > parameter were given, the size parameter was silently ignored. Instead, > make specifying two sizes an error. > > Signed-off-by: Kevin Wolf <kwolf@redhat.com> > --- > block.c | 10 ++++++++-- > tests/qemu-iotests/049 | 5 +++++ > tests/qemu-iotests/049.out | 5 +++++ > 3 files changed, 18 insertions(+), 2 deletions(-) Longtime bug, so not essential for 4.2-rc3. But small enough that you could sneak it in if you have other patches to queue and don't want to wait for 5.0. Reviewed-by: Eric Blake <eblake@redhat.com> -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
On 11/26/19 9:54 AM, Eric Blake wrote: > On 11/26/19 9:48 AM, Kevin Wolf wrote: >> If both the create options (qemu-img create -o ...) and the size >> parameter were given, the size parameter was silently ignored. Instead, >> make specifying two sizes an error. >> >> Signed-off-by: Kevin Wolf <kwolf@redhat.com> >> --- >> block.c | 10 ++++++++-- >> tests/qemu-iotests/049 | 5 +++++ >> tests/qemu-iotests/049.out | 5 +++++ >> 3 files changed, 18 insertions(+), 2 deletions(-) > > Longtime bug, so not essential for 4.2-rc3. But small enough that you > could sneak it in if you have other patches to queue and don't want to > wait for 5.0. > > Reviewed-by: Eric Blake <eblake@redhat.com> Actually, your patch fails to diagnose: $ qemu-img create -o size=1m,size=2m -f qcow2 x.qcow2 Formatting 'x.qcow2', fmt=qcow2 size=2097152 cluster_size=65536 lazy_refcounts=off refcount_bits=16 so you may want to enhance this patch to also catch the case of -o size used more than once. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
Am 26.11.2019 um 16:56 hat Eric Blake geschrieben: > On 11/26/19 9:54 AM, Eric Blake wrote: > > On 11/26/19 9:48 AM, Kevin Wolf wrote: > > > If both the create options (qemu-img create -o ...) and the size > > > parameter were given, the size parameter was silently ignored. Instead, > > > make specifying two sizes an error. > > > > > > Signed-off-by: Kevin Wolf <kwolf@redhat.com> > > > --- > > > block.c | 10 ++++++++-- > > > tests/qemu-iotests/049 | 5 +++++ > > > tests/qemu-iotests/049.out | 5 +++++ > > > 3 files changed, 18 insertions(+), 2 deletions(-) > > > > Longtime bug, so not essential for 4.2-rc3. But small enough that you > > could sneak it in if you have other patches to queue and don't want to > > wait for 5.0. > > > > Reviewed-by: Eric Blake <eblake@redhat.com> > > Actually, your patch fails to diagnose: > > $ qemu-img create -o size=1m,size=2m -f qcow2 x.qcow2 > Formatting 'x.qcow2', fmt=qcow2 size=2097152 cluster_size=65536 > lazy_refcounts=off refcount_bits=16 > > so you may want to enhance this patch to also catch the case of -o size used > more than once. Hm... Isn't this something that QemuOpts should already catch? Or do we have callers that actually expect the same option specified multiple times? Somehow I'm almost sure that Markus will know an example... But anyway, I figure the same problem exists for almost all options. Kevin
On 11/26/19 10:26 AM, Kevin Wolf wrote: >> Actually, your patch fails to diagnose: >> >> $ qemu-img create -o size=1m,size=2m -f qcow2 x.qcow2 >> Formatting 'x.qcow2', fmt=qcow2 size=2097152 cluster_size=65536 >> lazy_refcounts=off refcount_bits=16 >> >> so you may want to enhance this patch to also catch the case of -o size used >> more than once. > > Hm... Isn't this something that QemuOpts should already catch? Or do we > have callers that actually expect the same option specified multiple > times? QemuOpts is horrible. It allows duplication, and leaves it up to the client what to do about it. Some clients use the duplication to collect multiple arguments (such as specifying more than one cpu), some callers treat duplication by honoring only the FIRST option specified (and ignoring later uses - I find this ugly), and yet other callers treat duplication by honoring only the LAST option specified. > > Somehow I'm almost sure that Markus will know an example... Probably of all three (ab)uses of QemuOpt duplication. > > But anyway, I figure the same problem exists for almost all options. > > Kevin > -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
Eric Blake <eblake@redhat.com> writes: > On 11/26/19 10:26 AM, Kevin Wolf wrote: > >>> Actually, your patch fails to diagnose: >>> >>> $ qemu-img create -o size=1m,size=2m -f qcow2 x.qcow2 >>> Formatting 'x.qcow2', fmt=qcow2 size=2097152 cluster_size=65536 >>> lazy_refcounts=off refcount_bits=16 >>> >>> so you may want to enhance this patch to also catch the case of -o size used >>> more than once. Please don't. Let me explain. >> Hm... Isn't this something that QemuOpts should already catch? Or do we >> have callers that actually expect the same option specified multiple >> times? > > QemuOpts is horrible. It allows duplication, and leaves it up to the > client what to do about it. Some clients use the duplication to > collect multiple arguments (such as specifying more than one cpu), > some callers treat duplication by honoring only the FIRST option > specified (and ignoring later uses - I find this ugly), and yet other > callers treat duplication by honoring only the LAST option specified. QemuOpts has always permitted multiple keys, last one wins. key=1,key=2,key=3 means key=3. Permits things like overriding settings read from a configuration file on the command line. The straightforward way to use QemuOpts lets the last one win: qemu_opt_set() & friends insert at the end of the tail queue, qemu_opt_find() searches backwards for a match. Its storing of duplicates may have been just an implementation artifact initially, but then some "clever" uses were invented. These work by iterating over all QemuOpts parameters with qemu_opt_foreach(), or over all parameters of a certain name with qemu_opt_iter_init() and qemu_opt_iter_next(). >> >> Somehow I'm almost sure that Markus will know an example... > > Probably of all three (ab)uses of QemuOpt duplication. What the iterating ones all do is anybody's guess. A common use is collecting all values. This presses key repetition into list service: key=1,key=2,key=3 is interpreted like key: [1, 2, 3]. Abusing iteration to honor the first one instead of the last one would be disgusting. I'm not aware of such a user, but I'm also not betting my own money on absence of such abuse. Needless to say, the peculiarities of QemuOpts are all ABI by now. Doesn't mean we cannot change them, only that changes need to be deliberate and careful. >> But anyway, I figure the same problem exists for almost all options. It's a feature, not a problem :)
© 2016 - 2024 Red Hat, Inc.