[v3,2/4] qemu-img: fix --image-opts usage with dd command

Commit Message

Daniel P. Berrangé Feb. 20, 2017, 3:19 p.m. UTC

The --image-opts flag can only be used to affect the parsing
of the source image. The target image has to be specified in
the traditional style regardless, since it needs to be passed
to the bdrv_create() API which does not support the new style
opts.

Reviewed-by: Max Reitz <[e-mail]>
Signed-off-by: Daniel P. Berrange <[e-mail]>
---
 qemu-img.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

Comments

Kevin Wolf Feb. 22, 2017, 10:46 a.m. UTC | #1

Am 20.02.2017 um 16:19 hat Daniel P. Berrange geschrieben:
> The --image-opts flag can only be used to affect the parsing
> of the source image. The target image has to be specified in
> the traditional style regardless, since it needs to be passed
> to the bdrv_create() API which does not support the new style
> opts.
> 
> Reviewed-by: Max Reitz <[e-mail]>
> Signed-off-by: Daniel P. Berrange <[e-mail]>

Hm. This means that...

1. --image-opts never worked for 'qemu-img dd'

2. If we ever change bdrv_create() to be more flexible, with this patch
   we'd be stuck with an inconsistent "filename for target, options for
   source" interface because we can't change the semantics any more.

Should we just remove --image-opts from qemu-img dd instead until we can
provide the real thing?

Kevin

Daniel P. Berrangé Feb. 22, 2017, 11:31 a.m. UTC | #2

On Wed, Feb 22, 2017 at 11:46:06AM +0100, Kevin Wolf wrote:
> Am 20.02.2017 um 16:19 hat Daniel P. Berrange geschrieben:
> > The --image-opts flag can only be used to affect the parsing
> > of the source image. The target image has to be specified in
> > the traditional style regardless, since it needs to be passed
> > to the bdrv_create() API which does not support the new style
> > opts.
> > 
> > Reviewed-by: Max Reitz <[e-mail]>
> > Signed-off-by: Daniel P. Berrange <[e-mail]>
> 
> Hm. This means that...
> 
> 1. --image-opts never worked for 'qemu-img dd'
> 
> 2. If we ever change bdrv_create() to be more flexible, with this patch
>    we'd be stuck with an inconsistent "filename for target, options for
>    source" interface because we can't change the semantics any more.
> 
> Should we just remove --image-opts from qemu-img dd instead until we can
> provide the real thing?

We're already in that situation wrt bdrv_create() for other commands, so
we need a separate flag to request use of image opts for the target
image. So I don't think we want to special case dd in this respect.


Regards,
Daniel

Patch

diff --git a/qemu-img.c b/qemu-img.c
index 739345e..d8a737f 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -4102,8 +4102,13 @@  static int img_dd(int argc, char **argv)
         goto out;
     }
 
-    blk2 = img_open(image_opts, out.filename, out_fmt, BDRV_O_RDWR,
-                    false, false);
+    /* TODO, we can't honour --image-opts for the target,
+     * since it needs to be given in a format compatible
+     * with the bdrv_create() call above which does not
+     * support image-opts style.
+     */
+    blk2 = img_open_file(out.filename, out_fmt, BDRV_O_RDWR,
+                         false, false);
 
     if (!blk2) {
         ret = -1;