[PATCH] block: Error out on image creation with conflicting size options

[PATCH] block: Error out on image creation with conflicting size options

[Kevin Wolf]

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

Re: [PATCH] block: Error out on image creation with conflicting size options

[Eric Blake]

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

Re: [PATCH] block: Error out on image creation with conflicting size options

[Eric Blake]

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

Re: [PATCH] block: Error out on image creation with conflicting size options

[Kevin Wolf]

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

Re: [PATCH] block: Error out on image creation with conflicting size options

[Eric Blake]

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

Re: [PATCH] block: Error out on image creation with conflicting size options

[Markus Armbruster]

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