[for-2.10,3/3] qemu-img/convert: Move bs_n > 1 && -B check down
diff mbox

Message ID 20170413203401.3213-4-mreitz@redhat.com
State New
Headers show

Commit Message

Max Reitz April 13, 2017, 8:34 p.m. UTC
It does not make much sense to use a backing image for the target when
you concatenate multiple images (because then there is no correspondence
between the source images' backing files and the target's); but it was
still possible to give one by using -o backing_file=X instead of -B X.

Fix this by moving the check.

(Also, change the error message because -B is not the only way to
 specify the backing file, evidently.)

Cc: qemu-stable <qemu-stable@nongnu.org>
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 qemu-img.c                 | 14 +++++++-------
 tests/qemu-iotests/122.out |  4 ++--
 2 files changed, 9 insertions(+), 9 deletions(-)

Comments

Eric Blake April 13, 2017, 9:17 p.m. UTC | #1
On 04/13/2017 03:34 PM, Max Reitz wrote:
> It does not make much sense to use a backing image for the target when
> you concatenate multiple images (because then there is no correspondence
> between the source images' backing files and the target's); but it was
> still possible to give one by using -o backing_file=X instead of -B X.
> 
> Fix this by moving the check.
> 
> (Also, change the error message because -B is not the only way to
>  specify the backing file, evidently.)

For that matter, 'create -B' is intentionally? undocumented in the
qemu-img --help and man page.  (Personally, I don't know why we don't
want it documented - is it because it it not a universal option because
not all formats support backing files?)

> 
> Cc: qemu-stable <qemu-stable@nongnu.org>
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  qemu-img.c                 | 14 +++++++-------
>  tests/qemu-iotests/122.out |  4 ++--
>  2 files changed, 9 insertions(+), 9 deletions(-)
> 

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

Having a backing file for the destination may still make sense for some
convoluted workflow, but you can get that with a followup qemu-img rebase.
Max Reitz April 19, 2017, 12:38 p.m. UTC | #2
On 13.04.2017 23:17, Eric Blake wrote:
> On 04/13/2017 03:34 PM, Max Reitz wrote:
>> It does not make much sense to use a backing image for the target when
>> you concatenate multiple images (because then there is no correspondence
>> between the source images' backing files and the target's); but it was
>> still possible to give one by using -o backing_file=X instead of -B X.
>>
>> Fix this by moving the check.
>>
>> (Also, change the error message because -B is not the only way to
>>  specify the backing file, evidently.)
> 
> For that matter, 'create -B' is intentionally? undocumented in the
> qemu-img --help and man page.  (Personally, I don't know why we don't
> want it documented - is it because it it not a universal option because
> not all formats support backing files?)

(I suppose you mean convert instead of create)

Well, the man page at least documents that you can use backing_file for
the destination and what it will do. So documenting would probably be
just "Oh, you can also use -B, by the way."

I won't promise to write a patch to fix that, but I do hope I will. :-)

>> Cc: qemu-stable <qemu-stable@nongnu.org>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> ---
>>  qemu-img.c                 | 14 +++++++-------
>>  tests/qemu-iotests/122.out |  4 ++--
>>  2 files changed, 9 insertions(+), 9 deletions(-)
>>
> 
> Reviewed-by: Eric Blake <eblake@redhat.com>

Thanks, as always!

Max

> Having a backing file for the destination may still make sense for some
> convoluted workflow, but you can get that with a followup qemu-img rebase.

Patch
diff mbox

diff --git a/qemu-img.c b/qemu-img.c
index 70ffb07447..cfd1986efd 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -2113,13 +2113,6 @@  static int img_convert(int argc, char **argv)
     }
 
 
-    if (bs_n > 1 && out_baseimg) {
-        error_report("-B makes no sense when concatenating multiple input "
-                     "images");
-        ret = -1;
-        goto out;
-    }
-
     src_flags = 0;
     ret = bdrv_parse_cache_mode(src_cache, &src_flags, &src_writethrough);
     if (ret < 0) {
@@ -2229,6 +2222,13 @@  static int img_convert(int argc, char **argv)
         out_baseimg = out_baseimg_param;
     }
 
+    if (bs_n > 1 && out_baseimg) {
+        error_report("Having a backing file for the target makes no sense when "
+                     "concatenating multiple input images");
+        ret = -1;
+        goto out;
+    }
+
     /* Check if compression is supported */
     if (compress) {
         bool encryption =
diff --git a/tests/qemu-iotests/122.out b/tests/qemu-iotests/122.out
index 98814de5d6..9317d801ad 100644
--- a/tests/qemu-iotests/122.out
+++ b/tests/qemu-iotests/122.out
@@ -61,8 +61,8 @@  read 65536/65536 bytes at offset 4194304
 64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
 read 65536/65536 bytes at offset 8388608
 64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
-qemu-img: -B makes no sense when concatenating multiple input images
-qemu-img: -B makes no sense when concatenating multiple input images
+qemu-img: Having a backing file for the target makes no sense when concatenating multiple input images
+qemu-img: Having a backing file for the target makes no sense when concatenating multiple input images
 
 === Compression with misaligned allocations and image sizes ===