[Qemu-devel] [PATCH v2 0/3] qemu-img/convert: Some small fixes

[Qemu-devel] [PATCH v2 0/3] qemu-img/convert: Some small fixes

[Max Reitz]

This series fixes some small issues in qemu-img's convert subcommand.


v2:
- Not CC'd to qemu-stable: This is mostly a rebase onto block-next, and
  the patches that make a difference will not appear in qemu-stable.
  Therefore, if this is merged, v1 will be good for qemu-stable.
- Dropped the old patch 1, it became superfluous
- (New) patch 2: Rebased onto block-next
  - %s/bs_n/s.src_num/g
  - The block that's removed no longer sets ret to -1; but we still have
    to do this in the block that's added (and don't set it to -EINVAL
    there because all the places around it use -1 as well and -EINVAL
    would thus look weird).
- (New) patch 3: Added, as reported by Eric


git-backport-diff against v1:

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

001/3:[----] [-C] 'qemu-img/convert: Use @opts for one thing only'
002/3:[0007] [FC] 'qemu-img/convert: Move bs_n > 1 && -B check down'
003/3:[down] 'qemu-img: Document backing options'


Max Reitz (3):
  qemu-img/convert: Use @opts for one thing only
  qemu-img/convert: Move bs_n > 1 && -B check down
  qemu-img: Document backing options

 qemu-img.c                 | 23 +++++++++++++----------
 qemu-img-cmds.hx           |  8 ++++----
 qemu-img.texi              |  4 ++--
 tests/qemu-iotests/122.out |  4 ++--
 4 files changed, 21 insertions(+), 18 deletions(-)

-- 
2.12.2

[Qemu-devel] [PATCH v2 1/3] qemu-img/convert: Use @opts for one thing only

[Max Reitz]

After storing the creation options for the new image into @opts, we
fetch some things for our own information, like the backing file name,
or whether to use encryption or preallocation.

With the -n parameter, there will not be any creation options; this is
not too bad because this just means that querying a NULL @opts will
always return the default value.

However, we also use @opts for the --object options. Therefore, @opts is
not necessarily NULL if -n was specified; instead, it may contain those
options. In practice, this probably does not cause any problems because
there most likely is no object that supports any of the parameters we
query here, but this is neither something we should rely on nor does
this variable reuse make the code very nice to read.

Therefore, just use an own variable for the --object options.

Signed-off-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 qemu-img.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/qemu-img.c b/qemu-img.c
index b2c4dc9613..ae6f27f899 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -2049,13 +2049,15 @@ static int img_convert(int argc, char **argv)
         case 'W':
             s.wr_in_order = false;
             break;
-        case OPTION_OBJECT:
-            opts = qemu_opts_parse_noisily(&qemu_object_opts,
-                                           optarg, true);
-            if (!opts) {
+        case OPTION_OBJECT: {
+            QemuOpts *object_opts;
+            object_opts = qemu_opts_parse_noisily(&qemu_object_opts,
+                                                  optarg, true);
+            if (!object_opts) {
                 goto fail_getopt;
             }
             break;
+        }
         case OPTION_IMAGE_OPTS:
             image_opts = true;
             break;
-- 
2.12.2

[Qemu-devel] [PATCH v2 2/3] qemu-img/convert: Move bs_n > 1 && -B check down

[Max Reitz]

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

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

diff --git a/qemu-img.c b/qemu-img.c
index ae6f27f899..9eb82830f7 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -2089,12 +2089,6 @@ static int img_convert(int argc, char **argv)
     }
 
 
-    if (s.src_num > 1 && out_baseimg) {
-        error_report("-B makes no sense when concatenating multiple input "
-                     "images");
-        goto fail_getopt;
-    }
-
     /* ret is still -EINVAL until here */
     ret = bdrv_parse_cache_mode(src_cache, &src_flags, &src_writethrough);
     if (ret < 0) {
@@ -2208,6 +2202,13 @@ static int img_convert(int argc, char **argv)
     }
     s.target_has_backing = (bool) out_baseimg;
 
+    if (s.src_num > 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 (s.compressed) {
         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 ===
 
-- 
2.12.2

[Qemu-devel] [PATCH v2 3/3] qemu-img: Document backing options

[Max Reitz]

The create and convert subcommands have shorthands to set the
backing_file and, in the case of create, the backing_fmt options for the
new image. However, they have not been documented so far, which is
remedied by this patch.

Reported-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 qemu-img-cmds.hx | 8 ++++----
 qemu-img.texi    | 4 ++--
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/qemu-img-cmds.hx b/qemu-img-cmds.hx
index 8ac78222af..bf4ce59019 100644
--- a/qemu-img-cmds.hx
+++ b/qemu-img-cmds.hx
@@ -22,9 +22,9 @@ STEXI
 ETEXI
 
 DEF("create", img_create,
-    "create [-q] [--object objectdef] [-f fmt] [-o options] filename [size]")
+    "create [-q] [--object objectdef] [-f fmt] [-b backing_file] [-F backing_fmt] [-o options] filename [size]")
 STEXI
-@item create [--object @var{objectdef}] [-q] [-f @var{fmt}] [-o @var{options}] @var{filename} [@var{size}]
+@item create [--object @var{objectdef}] [-q] [-f @var{fmt}] [-b @var{backing_file}] [-F @var{backing_fmt}] [-o @var{options}] @var{filename} [@var{size}]
 ETEXI
 
 DEF("commit", img_commit,
@@ -40,9 +40,9 @@ STEXI
 ETEXI
 
 DEF("convert", img_convert,
-    "convert [--object objectdef] [--image-opts] [-c] [-p] [-q] [-n] [-f fmt] [-t cache] [-T src_cache] [-O output_fmt] [-o options] [-s snapshot_id_or_name] [-l snapshot_param] [-S sparse_size] [-m num_coroutines] [-W] filename [filename2 [...]] output_filename")
+    "convert [--object objectdef] [--image-opts] [-c] [-p] [-q] [-n] [-f fmt] [-t cache] [-T src_cache] [-O output_fmt] [-B backing_file] [-o options] [-s snapshot_id_or_name] [-l snapshot_param] [-S sparse_size] [-m num_coroutines] [-W] filename [filename2 [...]] output_filename")
 STEXI
-@item convert [--object @var{objectdef}] [--image-opts] [-c] [-p] [-q] [-n] [-f @var{fmt}] [-t @var{cache}] [-T @var{src_cache}] [-O @var{output_fmt}] [-o @var{options}] [-s @var{snapshot_id_or_name}] [-l @var{snapshot_param}] [-S @var{sparse_size}] [-m @var{num_coroutines}] [-W] @var{filename} [@var{filename2} [...]] @var{output_filename}
+@item convert [--object @var{objectdef}] [--image-opts] [-c] [-p] [-q] [-n] [-f @var{fmt}] [-t @var{cache}] [-T @var{src_cache}] [-O @var{output_fmt}] [-B @var{backing_file}] [-o @var{options}] [-s @var{snapshot_id_or_name}] [-l @var{snapshot_param}] [-S @var{sparse_size}] [-m @var{num_coroutines}] [-W] @var{filename} [@var{filename2} [...]] @var{output_filename}
 ETEXI
 
 DEF("dd", img_dd,
diff --git a/qemu-img.texi b/qemu-img.texi
index c81db3e81c..8c573ae010 100644
--- a/qemu-img.texi
+++ b/qemu-img.texi
@@ -224,7 +224,7 @@ If @code{-r} is specified, exit codes representing the image state refer to the
 state after (the attempt at) repairing it. That is, a successful @code{-r all}
 will yield the exit code 0, independently of the image state before.
 
-@item create [-f @var{fmt}] [-o @var{options}] @var{filename} [@var{size}]
+@item create [-f @var{fmt}] [-b @var{backing_file}] [-F @var{backing_fmt}] [-o @var{options}] @var{filename} [@var{size}]
 
 Create the new disk image @var{filename} of size @var{size} and format
 @var{fmt}. Depending on the file format, you can add one or more @var{options}
@@ -302,7 +302,7 @@ Error on reading data
 
 @end table
 
-@item convert [-c] [-p] [-n] [-f @var{fmt}] [-t @var{cache}] [-T @var{src_cache}] [-O @var{output_fmt}] [-o @var{options}] [-s @var{snapshot_id_or_name}] [-l @var{snapshot_param}] [-m @var{num_coroutines}] [-W] [-S @var{sparse_size}] @var{filename} [@var{filename2} [...]] @var{output_filename}
+@item convert [-c] [-p] [-n] [-f @var{fmt}] [-t @var{cache}] [-T @var{src_cache}] [-O @var{output_fmt}] [-B @var{backing_file}] [-o @var{options}] [-s @var{snapshot_id_or_name}] [-l @var{snapshot_param}] [-m @var{num_coroutines}] [-W] [-S @var{sparse_size}] @var{filename} [@var{filename2} [...]] @var{output_filename}
 
 Convert the disk image @var{filename} or a snapshot @var{snapshot_param}(@var{snapshot_id_or_name} is deprecated)
 to disk image @var{output_filename} using format @var{output_fmt}. It can be optionally compressed (@code{-c}
-- 
2.12.2

Re: [Qemu-devel] [PATCH v2 1/3] qemu-img/convert: Use @opts for one thing only

[Eric Blake]

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

On 04/26/2017 08:46 AM, Max Reitz wrote:
> After storing the creation options for the new image into @opts, we
> fetch some things for our own information, like the backing file name,
> or whether to use encryption or preallocation.
> 
> With the -n parameter, there will not be any creation options; this is
> not too bad because this just means that querying a NULL @opts will
> always return the default value.
> 
> However, we also use @opts for the --object options. Therefore, @opts is
> not necessarily NULL if -n was specified; instead, it may contain those
> options. In practice, this probably does not cause any problems because
> there most likely is no object that supports any of the parameters we
> query here, but this is neither something we should rely on nor does
> this variable reuse make the code very nice to read.
> 
> Therefore, just use an own variable for the --object options.

Reads better with s/an own/a separate/

> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> Reviewed-by: Eric Blake <eblake@redhat.com>
> ---
>  qemu-img.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 

R-b still stands.

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


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

Re: [Qemu-devel] [PATCH v2 1/3] qemu-img/convert: Use @opts for one thing only

[Max Reitz]

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

On 26.04.2017 16:03, Eric Blake wrote:
> On 04/26/2017 08:46 AM, Max Reitz wrote:
>> After storing the creation options for the new image into @opts, we
>> fetch some things for our own information, like the backing file name,
>> or whether to use encryption or preallocation.
>>
>> With the -n parameter, there will not be any creation options; this is
>> not too bad because this just means that querying a NULL @opts will
>> always return the default value.
>>
>> However, we also use @opts for the --object options. Therefore, @opts is
>> not necessarily NULL if -n was specified; instead, it may contain those
>> options. In practice, this probably does not cause any problems because
>> there most likely is no object that supports any of the parameters we
>> query here, but this is neither something we should rely on nor does
>> this variable reuse make the code very nice to read.
>>
>> Therefore, just use an own variable for the --object options.
> 
> Reads better with s/an own/a separate/

I certainly wouldn't mind if someone (*cough*) were to update this while
applying. O:-)

Max

>>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> Reviewed-by: Eric Blake <eblake@redhat.com>
>> ---
>>  qemu-img.c | 10 ++++++----
>>  1 file changed, 6 insertions(+), 4 deletions(-)
>>
> 
> R-b still stands.


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

Re: [Qemu-devel] [PATCH v2 2/3] qemu-img/convert: Move bs_n > 1 && -B check down

[Eric Blake]

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

On 04/26/2017 08:46 AM, 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.)
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  qemu-img.c                 | 13 +++++++------
>  tests/qemu-iotests/122.out |  4 ++--
>  2 files changed, 9 insertions(+), 8 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


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

Re: [Qemu-devel] [PATCH v2 3/3] qemu-img: Document backing options

[Eric Blake]

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

On 04/26/2017 08:46 AM, Max Reitz wrote:
> The create and convert subcommands have shorthands to set the
> backing_file and, in the case of create, the backing_fmt options for the
> new image. However, they have not been documented so far, which is
> remedied by this patch.
> 
> Reported-by: Eric Blake <eblake@redhat.com>
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  qemu-img-cmds.hx | 8 ++++----
>  qemu-img.texi    | 4 ++--
>  2 files changed, 6 insertions(+), 6 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


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

Re: [Qemu-devel] [PATCH v2 3/3] qemu-img: Document backing options

[Kevin Wolf]

Am 26.04.2017 um 15:46 hat Max Reitz geschrieben:
> The create and convert subcommands have shorthands to set the
> backing_file and, in the case of create, the backing_fmt options for the
> new image. However, they have not been documented so far, which is
> remedied by this patch.
> 
> Reported-by: Eric Blake <eblake@redhat.com>
> Signed-off-by: Max Reitz <mreitz@redhat.com>

I think originally the idea was to remove the individual special case
options in the long term in favour of the uniform -o. But I don't think
the short options have fallen out of use, so we can as well document
them again...

Maybe we could change the implementation, however, so that the old
options are really just aliases for the respective -o version.

Kevin

Re: [Qemu-devel] [PATCH v2 0/3] qemu-img/convert: Some small fixes

[Kevin Wolf]

Am 26.04.2017 um 15:46 hat Max Reitz geschrieben:
> This series fixes some small issues in qemu-img's convert subcommand.

Thanks, applied to block-next.

Kevin

Re: [Qemu-devel] [PATCH v2 3/3] qemu-img: Document backing options

[Eric Blake]

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

On 04/26/2017 08:46 AM, Max Reitz wrote:
> The create and convert subcommands have shorthands to set the
> backing_file and, in the case of create, the backing_fmt options for the
> new image. However, they have not been documented so far, which is
> remedied by this patch.
> 

> +++ b/qemu-img.texi
> @@ -224,7 +224,7 @@ If @code{-r} is specified, exit codes representing the image state refer to the
>  state after (the attempt at) repairing it. That is, a successful @code{-r all}
>  will yield the exit code 0, independently of the image state before.
>  
> -@item create [-f @var{fmt}] [-o @var{options}] @var{filename} [@var{size}]
> +@item create [-f @var{fmt}] [-b @var{backing_file}] [-F @var{backing_fmt}] [-o @var{options}] @var{filename} [@var{size}]
>  
>  Create the new disk image @var{filename} of size @var{size} and format
>  @var{fmt}. Depending on the file format, you can add one or more @var{options}
> @@ -302,7 +302,7 @@ Error on reading data
>  
>  @end table
>  
> -@item convert [-c] [-p] [-n] [-f @var{fmt}] [-t @var{cache}] [-T @var{src_cache}] [-O @var{output_fmt}] [-o @var{options}] [-s @var{snapshot_id_or_name}] [-l @var{snapshot_param}] [-m @var{num_coroutines}] [-W] [-S @var{sparse_size}] @var{filename} [@var{filename2} [...]] @var{output_filename}
> +@item convert [-c] [-p] [-n] [-f @var{fmt}] [-t @var{cache}] [-T @var{src_cache}] [-O @var{output_fmt}] [-B @var{backing_file}] [-o @var{options}] [-s @var{snapshot_id_or_name}] [-l @var{snapshot_param}] [-m @var{num_coroutines}] [-W] [-S @var{sparse_size}] @var{filename} [@var{filename2} [...]] @var{output_filename}
>  
>  Convert the disk image @var{filename} or a snapshot @var{snapshot_param}(@var{snapshot_id_or_name} is deprecated)
>  to disk image @var{output_filename} using format @var{output_fmt}. It can be optionally compressed (@code{-c}


On 04/27/2017 10:25 AM, Kevin Wolf wrote:

> I think originally the idea was to remove the individual special case
> options in the long term in favour of the uniform -o. But I don't think
> the short options have fallen out of use, so we can as well document
> them again...
>
> Maybe we could change the implementation, however, so that the old
> options are really just aliases for the respective -o version.

As in, a followup patch that adds a paragraph or two to the .texi file
stating that '-B @var{backing_file}' is shorthand for '-o
backing_file=@var{backing_file}'?

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


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

Re: [Qemu-devel] [PATCH v2 3/3] qemu-img: Document backing options

[Kevin Wolf]

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

Am 27.04.2017 um 17:36 hat Eric Blake geschrieben:
> On 04/26/2017 08:46 AM, Max Reitz wrote:
> > I think originally the idea was to remove the individual special case
> > options in the long term in favour of the uniform -o. But I don't think
> > the short options have fallen out of use, so we can as well document
> > them again...
> >
> > Maybe we could change the implementation, however, so that the old
> > options are really just aliases for the respective -o version.
> 
> As in, a followup patch that adds a paragraph or two to the .texi file
> stating that '-B @var{backing_file}' is shorthand for '-o
> backing_file=@var{backing_file}'?

First and foremost, I meant in the C code where we have separate
variables for all the shorthand options and then need to handle both
ways explicitly in the code further down. Instead, -B could just
directly add a 'backing_file' key to a QemuOpts (and -o would do the
same instead of building a string that is parsed only later).

But the documentation change that you're suggesting is probably a good
idea as well.

Kevin

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: [Qemu-devel] [PATCH v2 3/3] qemu-img: Document backing options

[Max Reitz]

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

On 28.04.2017 11:38, Kevin Wolf wrote:
> Am 27.04.2017 um 17:36 hat Eric Blake geschrieben:
>> On 04/26/2017 08:46 AM, Max Reitz wrote:
>>> I think originally the idea was to remove the individual special case
>>> options in the long term in favour of the uniform -o. But I don't think
>>> the short options have fallen out of use, so we can as well document
>>> them again...

Hm, I'm not really sure what the use of deprecating them would be. We
would be able to use the single-letter options -b/-B/-F for something
else, but would we really want to do that...?

(And it should always be trivial to map these legacy parameters to
normal options.)

>>> Maybe we could change the implementation, however, so that the old
>>> options are really just aliases for the respective -o version.
>>
>> As in, a followup patch that adds a paragraph or two to the .texi file
>> stating that '-B @var{backing_file}' is shorthand for '-o
>> backing_file=@var{backing_file}'?
> 
> First and foremost, I meant in the C code where we have separate
> variables for all the shorthand options and then need to handle both
> ways explicitly in the code further down. Instead, -B could just
> directly add a 'backing_file' key to a QemuOpts (and -o would do the
> same instead of building a string that is parsed only later).

We already do that more or less for convert, actually (for -B, that is,
not for -o). We just have to keep out_baseimg separately because we
won't have a QemuOpts with -n.

Using add_old_style_options() in img_create() should be easy enough.

> But the documentation change that you're suggesting is probably a good
> idea as well.

Well, I more or less deliberately did not really document -b/-B/-F. I
just documented them in the syntax overview and named the parameters
backing_file and backing_fmt, both of which are valid options and are
*as such* documented in the explanatory text. So from my point of view,
it's already documented to be a shorthand.

Max


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