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

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

[Max Reitz]

This series fixes some small issues I found in qemu-img's convert
subcommand while reviewing Dan's convert/dd series.

Nothing serious at all, so all for 2.10.


Max Reitz (3):
  qemu-img/convert: Always set ret < 0 on error
  qemu-img/convert: Use @opts for one thing only
  qemu-img/convert: Move bs_n > 1 && -B check down

 qemu-img.c                 | 26 +++++++++++++++-----------
 tests/qemu-iotests/122.out |  4 ++--
 2 files changed, 17 insertions(+), 13 deletions(-)

-- 
2.12.2

[Qemu-devel] [PATCH for-2.10 1/3] qemu-img/convert: Always set ret < 0 on error

[Max Reitz]

Otherwise the qemu-img process will exit with EXIT_SUCCESS instead of
EXIT_FAILURE.

Cc: qemu-stable <qemu-stable@nongnu.org>
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 qemu-img.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/qemu-img.c b/qemu-img.c
index 37c2894678..f2809e1ab4 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -2069,6 +2069,7 @@ static int img_convert(int argc, char **argv)
             opts = qemu_opts_parse_noisily(&qemu_object_opts,
                                            optarg, true);
             if (!opts) {
+                ret = -1;
                 goto fail_getopt;
             }
             break;
@@ -2081,6 +2082,7 @@ static int img_convert(int argc, char **argv)
     if (qemu_opts_foreach(&qemu_object_opts,
                           user_creatable_add_opts_foreach,
                           NULL, NULL)) {
+        ret = -1;
         goto fail_getopt;
     }
 
-- 
2.12.2

Re: [Qemu-devel] [PATCH for-2.10 1/3] qemu-img/convert: Always set ret < 0 on error

[Eric Blake]

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

On 04/13/2017 03:33 PM, Max Reitz wrote:
> Otherwise the qemu-img process will exit with EXIT_SUCCESS instead of
> EXIT_FAILURE.
> 
> Cc: qemu-stable <qemu-stable@nongnu.org>
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  qemu-img.c | 2 ++
>  1 file changed, 2 insertions(+)

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

> 
> diff --git a/qemu-img.c b/qemu-img.c
> index 37c2894678..f2809e1ab4 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -2069,6 +2069,7 @@ static int img_convert(int argc, char **argv)
>              opts = qemu_opts_parse_noisily(&qemu_object_opts,
>                                             optarg, true);
>              if (!opts) {
> +                ret = -1;
>                  goto fail_getopt;
>              }
>              break;
> @@ -2081,6 +2082,7 @@ static int img_convert(int argc, char **argv)
>      if (qemu_opts_foreach(&qemu_object_opts,
>                            user_creatable_add_opts_foreach,
>                            NULL, NULL)) {
> +        ret = -1;
>          goto fail_getopt;
>      }
>  
> 

-- 
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 for-2.10 1/3] qemu-img/convert: Always set ret < 0 on error

[Philippe Mathieu-Daudé]

On 04/13/2017 05:33 PM, Max Reitz wrote:
> Otherwise the qemu-img process will exit with EXIT_SUCCESS instead of
> EXIT_FAILURE.
>
> Cc: qemu-stable <qemu-stable@nongnu.org>
> Signed-off-by: Max Reitz <mreitz@redhat.com>

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

> ---
>  qemu-img.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/qemu-img.c b/qemu-img.c
> index 37c2894678..f2809e1ab4 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -2069,6 +2069,7 @@ static int img_convert(int argc, char **argv)
>              opts = qemu_opts_parse_noisily(&qemu_object_opts,
>                                             optarg, true);
>              if (!opts) {
> +                ret = -1;
>                  goto fail_getopt;
>              }
>              break;
> @@ -2081,6 +2082,7 @@ static int img_convert(int argc, char **argv)
>      if (qemu_opts_foreach(&qemu_object_opts,
>                            user_creatable_add_opts_foreach,
>                            NULL, NULL)) {
> +        ret = -1;
>          goto fail_getopt;
>      }
>
>

[Qemu-devel] [PATCH for-2.10 2/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.

Cc: qemu-stable <qemu-stable@nongnu.org>
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 qemu-img.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/qemu-img.c b/qemu-img.c
index f2809e1ab4..70ffb07447 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -2065,14 +2065,16 @@ static int img_convert(int argc, char **argv)
         case 'W':
             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) {
                 ret = -1;
                 goto fail_getopt;
             }
             break;
+        }
         case OPTION_IMAGE_OPTS:
             image_opts = true;
             break;
-- 
2.12.2

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

[Eric Blake]

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

On 04/13/2017 03:34 PM, 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.
> 
> Cc: qemu-stable <qemu-stable@nongnu.org>
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  qemu-img.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 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 --]

[Qemu-devel] [PATCH for-2.10 3/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.)

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

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 ===
 
-- 
2.12.2

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

[Eric Blake]

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

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.

-- 
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 for-2.10 3/3] qemu-img/convert: Move bs_n > 1 && -B check down

[Max Reitz]

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

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.


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

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

[Kevin Wolf]

Am 13.04.2017 um 22:33 hat Max Reitz geschrieben:
> This series fixes some small issues I found in qemu-img's convert
> subcommand while reviewing Dan's convert/dd series.
> 
> Nothing serious at all, so all for 2.10.

This series conflicts with Peter Lieven's "qemu-img: simplify
img_convert" in my block-next branch.

Patch 1 is unnecessary now (ret is still -EINVAL at this point), the
other two probably just need rebasing.

Kevin

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

[Max Reitz]

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

On 19.04.2017 17:49, Kevin Wolf wrote:
> Am 13.04.2017 um 22:33 hat Max Reitz geschrieben:
>> This series fixes some small issues I found in qemu-img's convert
>> subcommand while reviewing Dan's convert/dd series.
>>
>> Nothing serious at all, so all for 2.10.
> 
> This series conflicts with Peter Lieven's "qemu-img: simplify
> img_convert" in my block-next branch.
> 
> Patch 1 is unnecessary now (ret is still -EINVAL at this point), the
> other two probably just need rebasing.

Hm... What should I do about qemu-stable, though? I don't think that
Peter's series will end up there, so qemu-stable should probably indeed
take v1...

Max


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

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

[Kevin Wolf]

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

Am 26.04.2017 um 14:48 hat Max Reitz geschrieben:
> On 19.04.2017 17:49, Kevin Wolf wrote:
> > Am 13.04.2017 um 22:33 hat Max Reitz geschrieben:
> >> This series fixes some small issues I found in qemu-img's convert
> >> subcommand while reviewing Dan's convert/dd series.
> >>
> >> Nothing serious at all, so all for 2.10.
> > 
> > This series conflicts with Peter Lieven's "qemu-img: simplify
> > img_convert" in my block-next branch.
> > 
> > Patch 1 is unnecessary now (ret is still -EINVAL at this point), the
> > other two probably just need rebasing.
> 
> Hm... What should I do about qemu-stable, though? I don't think that
> Peter's series will end up there, so qemu-stable should probably indeed
> take v1...

Yes, this is true. We probably need a patch specifically for qemu-stable
then and a different one for master.

Kevin

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