[Qemu-devel] [PATCH v3 0/4] Improve convert and dd commands

[Qemu-devel] [PATCH v3 0/4] Improve convert and dd commands

[Daniel P. Berrange]

Update to

  v1: https://lists.gnu.org/archive/html/qemu-devel/2017-01/msg05699.html
  v2: https://lists.gnu.org/archive/html/qemu-devel/2017-02/msg00728.html

This series is in response to Max pointing out that you cannot
use 'convert' for an encrypted target image.

The 'convert' and 'dd' commands need to first create the image
and then open it. The bdrv_create() method takes a set of options
for creating the image, which let us provide a key-secret for the
encryption key. When the commands then open the new image, they
don't provide any options, so the image is unable to be opened
due to lack of encryption key. It is also not possible to use
the --image-opts argument to provide structured options in the
target image name - it must be a plain filename to satisfy the
bdrv_create() API contract.

This series addresses these problems to some extent

 - Adds a new --target-image-opts flag which is used to say
   that the target filename is using structured options.
   It is *only* permitted to use this when -n is also set.
   ie the target image must be pre-created so convert/dd
   don't need to run bdrv_create().

 - When --target-image-opts is not used, add special case
   code that identifies options passed to bdrv_create()
   named "*key-secret" and adds them to the options used
   to open the new image

In future it is desirable to make --target-image-opts work
even when -n is *not* given. This requires considerable
work to create a new bdrv_create() API impl.

The first patch fixes a bug in the 'dd' command while the second adds support
for the missing '--object' arg to 'dd', allowing it to reference secrets when
opening files.  The last two patches implement the new features described above
for the 'convert' command.

Changed in v3:

 - Drop all patches affecting the 'dd' command except for the clear bug fix
   and the --object support. They can be re-considered once dd is rewritten
   to run ontop of convert.
 - Use consistent return/goto style in dd command (Max)
 - Fix error reporting when using compressed image and skip-create (Max)
 - Unconditionally create QDict when open files (Max)

Changed in v2:

 - Replace dd -n flag with support for conv=nocreat,notrunc
 - Misc typos (Eric, Fam)

Daniel P. Berrange (4):
  qemu-img: add support for --object with 'dd' command
  qemu-img: fix --image-opts usage with dd command
  qemu-img: introduce --target-image-opts for 'convert' command
  qemu-img: copy *key-secret opts when opening newly created files

 qemu-img-cmds.hx |   4 +-
 qemu-img.c       | 151 +++++++++++++++++++++++++++++++++++++++++++++----------
 qemu-img.texi    |  12 ++++-
 3 files changed, 136 insertions(+), 31 deletions(-)

-- 
2.9.3

[Qemu-devel] [PATCH v3 1/4] qemu-img: add support for --object with 'dd' command

[Daniel P. Berrange]

The qemu-img dd command added --image-opts support, but missed
the corresponding --object support. This prevented passing
secrets (eg auth passwords) needed by certain disk images.

Reviewed-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
---
 qemu-img.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/qemu-img.c b/qemu-img.c
index cff22e3..739345e 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -3951,6 +3951,7 @@ static int img_dd(int argc, char **argv)
     };
     const struct option long_options[] = {
         { "help", no_argument, 0, 'h'},
+        { "object", required_argument, 0, OPTION_OBJECT},
         { "image-opts", no_argument, 0, OPTION_IMAGE_OPTS},
         { 0, 0, 0, 0 }
     };
@@ -3973,6 +3974,15 @@ static int img_dd(int argc, char **argv)
         case 'h':
             help();
             break;
+        case OPTION_OBJECT: {
+            QemuOpts *opts;
+            opts = qemu_opts_parse_noisily(&qemu_object_opts,
+                                           optarg, true);
+            if (!opts) {
+                ret = -1;
+                goto out;
+            }
+        }   break;
         case OPTION_IMAGE_OPTS:
             image_opts = true;
             break;
@@ -4017,6 +4027,14 @@ static int img_dd(int argc, char **argv)
         ret = -1;
         goto out;
     }
+
+    if (qemu_opts_foreach(&qemu_object_opts,
+                          user_creatable_add_opts_foreach,
+                          NULL, NULL)) {
+        ret = -1;
+        goto out;
+    }
+
     blk1 = img_open(image_opts, in.filename, fmt, 0, false, false);
 
     if (!blk1) {
-- 
2.9.3

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

[Daniel P. Berrange]

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 <mreitz@redhat.com>
Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
---
 qemu-img.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

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;
-- 
2.9.3

Re: [Qemu-devel] [PATCH v3 2/4] qemu-img: fix --image-opts usage with dd command

[Kevin Wolf]

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 <mreitz@redhat.com>
> Signed-off-by: Daniel P. Berrange <berrange@redhat.com>

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

Re: [Qemu-devel] [PATCH v3 2/4] qemu-img: fix --image-opts usage with dd command

[Daniel P. Berrange]

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 <mreitz@redhat.com>
> > Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
> 
> 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
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://entangle-photo.org       -o-    http://search.cpan.org/~danberr/ :|

[Qemu-devel] [PATCH v3 3/4] qemu-img: introduce --target-image-opts for 'convert' command

[Daniel P. Berrange]

The '--image-opts' flags indicates whether the source filename
includes options. The target filename has to remain in the
plain filename format though, since it needs to be passed to
bdrv_create().  When using --skip-create though, it would be
possible to use image-opts syntax. This adds --target-image-opts
to indicate that the target filename includes options. Currently
this mandates use of the --skip-create flag too.

Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
---
 qemu-img-cmds.hx |  4 +--
 qemu-img.c       | 84 +++++++++++++++++++++++++++++++++++++++-----------------
 qemu-img.texi    | 12 ++++++--
 3 files changed, 71 insertions(+), 29 deletions(-)

diff --git a/qemu-img-cmds.hx b/qemu-img-cmds.hx
index f054599..f838e58 100644
--- a/qemu-img-cmds.hx
+++ b/qemu-img-cmds.hx
@@ -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] filename [filename2 [...]] output_filename")
+    "convert [--object objectdef] [--image-opts] [--target-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] 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}] @var{filename} [@var{filename2} [...]] @var{output_filename}
+@item convert [--object @var{objectdef}] [--image-opts] [--target-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}] @var{filename} [@var{filename2} [...]] @var{output_filename}
 ETEXI
 
 DEF("dd", img_dd,
diff --git a/qemu-img.c b/qemu-img.c
index d8a737f..e48e676 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -59,6 +59,7 @@ enum {
     OPTION_PATTERN = 260,
     OPTION_FLUSH_INTERVAL = 261,
     OPTION_NO_DRAIN = 262,
+    OPTION_TARGET_IMAGE_OPTS = 263,
 };
 
 typedef enum OutputFormat {
@@ -1765,7 +1766,7 @@ static int img_convert(int argc, char **argv)
     int progress = 0, flags, src_flags;
     bool writethrough, src_writethrough;
     const char *fmt, *out_fmt, *cache, *src_cache, *out_baseimg, *out_filename;
-    BlockDriver *drv, *proto_drv;
+    BlockDriver *drv = NULL, *proto_drv = NULL;
     BlockBackend **blk = NULL, *out_blk = NULL;
     BlockDriverState **bs = NULL, *out_bs = NULL;
     int64_t total_sectors;
@@ -1783,9 +1784,10 @@ static int img_convert(int argc, char **argv)
     QemuOpts *sn_opts = NULL;
     ImgConvertState state;
     bool image_opts = false;
+    bool tgt_image_opts = false;
 
+    out_fmt = NULL;
     fmt = NULL;
-    out_fmt = "raw";
     cache = "unsafe";
     src_cache = BDRV_DEFAULT_CACHE;
     out_baseimg = NULL;
@@ -1796,6 +1798,7 @@ static int img_convert(int argc, char **argv)
             {"help", no_argument, 0, 'h'},
             {"object", required_argument, 0, OPTION_OBJECT},
             {"image-opts", no_argument, 0, OPTION_IMAGE_OPTS},
+            {"target-image-opts", no_argument, 0, OPTION_TARGET_IMAGE_OPTS},
             {0, 0, 0, 0}
         };
         c = getopt_long(argc, argv, "hf:O:B:ce6o:s:l:S:pt:T:qn",
@@ -1900,15 +1903,27 @@ static int img_convert(int argc, char **argv)
         case OPTION_IMAGE_OPTS:
             image_opts = true;
             break;
+        case OPTION_TARGET_IMAGE_OPTS:
+            tgt_image_opts = true;
+            break;
         }
     }
 
+    if (!out_fmt && !tgt_image_opts) {
+        out_fmt = "raw";
+    }
+
     if (qemu_opts_foreach(&qemu_object_opts,
                           user_creatable_add_opts_foreach,
                           NULL, NULL)) {
         goto fail_getopt;
     }
 
+    if (tgt_image_opts && !skip_create) {
+        error_report("--target-image-opts requires use of -n flag");
+        goto fail_getopt;
+    }
+
     /* Initialize before goto out */
     if (quiet) {
         progress = 0;
@@ -1919,8 +1934,13 @@ static int img_convert(int argc, char **argv)
     out_filename = bs_n >= 1 ? argv[argc - 1] : NULL;
 
     if (options && has_help_option(options)) {
-        ret = print_block_option_help(out_filename, out_fmt);
-        goto out;
+        if (out_fmt) {
+            ret = print_block_option_help(out_filename, out_fmt);
+            goto out;
+        } else {
+            error_report("Option help requires a format be specified");
+            goto fail_getopt;
+        }
     }
 
     if (bs_n < 1) {
@@ -1987,22 +2007,22 @@ static int img_convert(int argc, char **argv)
         goto out;
     }
 
-    /* Find driver and parse its options */
-    drv = bdrv_find_format(out_fmt);
-    if (!drv) {
-        error_report("Unknown file format '%s'", out_fmt);
-        ret = -1;
-        goto out;
-    }
+    if (!skip_create) {
+        /* Find driver and parse its options */
+        drv = bdrv_find_format(out_fmt);
+        if (!drv) {
+            error_report("Unknown file format '%s'", out_fmt);
+            ret = -1;
+            goto out;
+        }
 
-    proto_drv = bdrv_find_protocol(out_filename, true, &local_err);
-    if (!proto_drv) {
-        error_report_err(local_err);
-        ret = -1;
-        goto out;
-    }
+        proto_drv = bdrv_find_protocol(out_filename, true, &local_err);
+        if (!proto_drv) {
+            error_report_err(local_err);
+            ret = -1;
+            goto out;
+        }
 
-    if (!skip_create) {
         if (!drv->create_opts) {
             error_report("Format driver '%s' does not support image creation",
                          drv->format_name);
@@ -2051,7 +2071,7 @@ static int img_convert(int argc, char **argv)
         const char *preallocation =
             qemu_opt_get(opts, BLOCK_OPT_PREALLOC);
 
-        if (!drv->bdrv_co_pwritev_compressed) {
+        if (drv && !drv->bdrv_co_pwritev_compressed) {
             error_report("Compression not supported for this file format");
             ret = -1;
             goto out;
@@ -2091,18 +2111,32 @@ static int img_convert(int argc, char **argv)
         goto out;
     }
 
-    /* XXX we should allow --image-opts to trigger use of
-     * img_open() here, but then we have trouble with
-     * the bdrv_create() call which takes different params.
-     * Not critical right now, so fix can wait...
-     */
-    out_blk = img_open_file(out_filename, out_fmt, flags, writethrough, quiet);
+    if (skip_create) {
+        out_blk = img_open(tgt_image_opts, out_filename, out_fmt,
+                           flags, writethrough, quiet);
+    } else {
+        /* TODO ultimately we should allow --target-image-opts
+         * to be used even when -n is not given.
+         * That has to wait for bdrv_create to be improved
+         * to allow filenames in option syntax
+         */
+        out_blk = img_open_file(out_filename, out_fmt,
+                                flags, writethrough, quiet);
+    }
     if (!out_blk) {
         ret = -1;
         goto out;
     }
     out_bs = blk_bs(out_blk);
 
+    if (compress) {
+        if (!out_bs->drv->bdrv_co_pwritev_compressed) {
+            error_report("Compression not supported for this file format");
+            ret = -1;
+            goto out;
+        }
+    }
+
     /* increase bufsectors from the default 4096 (2M) if opt_transfer
      * or discard_alignment of the out_bs is greater. Limit to 32768 (16MB)
      * as maximum. */
diff --git a/qemu-img.texi b/qemu-img.texi
index 174aae3..d525e3a 100644
--- a/qemu-img.texi
+++ b/qemu-img.texi
@@ -45,9 +45,17 @@ keys.
 
 @item --image-opts
 
-Indicates that the @var{filename} parameter is to be interpreted as a
+Indicates that the source @var{filename} parameter is to be interpreted as a
 full option string, not a plain filename. This parameter is mutually
-exclusive with the @var{-f} and @var{-F} parameters.
+exclusive with the @var{-f} parameter.
+
+@item --target-image-opts
+
+Indicates that the target @var{filename} parameter(s) are to be interpreted a
+a full option string, not a plain filename. This parameter is mutually
+exclusive with the @var{-O} parameters. It is currently required to also use
+the @var{-n} parameter to skip image creation. This restriction may be relaxed
+in a future release.
 
 @item fmt
 is the disk image format. It is guessed automatically in most cases. See below
-- 
2.9.3

Re: [Qemu-devel] [PATCH v3 3/4] qemu-img: introduce --target-image-opts for 'convert' command

[Kevin Wolf]

Am 20.02.2017 um 16:19 hat Daniel P. Berrange geschrieben:
> The '--image-opts' flags indicates whether the source filename
> includes options. The target filename has to remain in the
> plain filename format though, since it needs to be passed to
> bdrv_create().  When using --skip-create though, it would be
> possible to use image-opts syntax. This adds --target-image-opts
> to indicate that the target filename includes options. Currently
> this mandates use of the --skip-create flag too.
> 
> Signed-off-by: Daniel P. Berrange <berrange@redhat.com>

Hm, okay, with a separate --target-image-opts here, too, I guess the
'qemu-img dd' interface would be consistent after all.

Kevin

Re: [Qemu-devel] [PATCH v3 3/4] qemu-img: introduce --target-image-opts for 'convert' command

[Eric Blake]

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

On 02/20/2017 09:19 AM, Daniel P. Berrange wrote:
> The '--image-opts' flags indicates whether the source filename
> includes options. The target filename has to remain in the
> plain filename format though, since it needs to be passed to
> bdrv_create().  When using --skip-create though, it would be
> possible to use image-opts syntax. This adds --target-image-opts
> to indicate that the target filename includes options. Currently
> this mandates use of the --skip-create flag too.
> 
> Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
> ---
>  qemu-img-cmds.hx |  4 +--
>  qemu-img.c       | 84 +++++++++++++++++++++++++++++++++++++++-----------------
>  qemu-img.texi    | 12 ++++++--
>  3 files changed, 71 insertions(+), 29 deletions(-)
> 

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

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


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

[Qemu-devel] [PATCH v3 4/4] qemu-img: copy *key-secret opts when opening newly created files

[Daniel P. Berrange]

The qemu-img dd/convert commands will create a image file and
then try to open it. Historically it has been possible to open
new files without passing any options. With encrypted files
though, the *key-secret options are mandatory, so we need to
provide those options when opening the newly created file.

Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
---
 qemu-img.c | 44 ++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 42 insertions(+), 2 deletions(-)

diff --git a/qemu-img.c b/qemu-img.c
index e48e676..bad19fd 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -317,6 +317,46 @@ static BlockBackend *img_open_file(const char *filename,
 }
 
 
+static int img_add_key_secrets(void *opaque,
+                               const char *name, const char *value,
+                               Error **errp)
+{
+    QDict *options = opaque;
+
+    if (g_str_has_suffix(name, "key-secret")) {
+        qdict_put(options, name, qstring_from_str(value));
+    }
+
+    return 0;
+}
+
+static BlockBackend *img_open_new_file(const char *filename,
+                                       QemuOpts *create_opts,
+                                       const char *fmt, int flags,
+                                       bool writethrough, bool quiet)
+{
+    BlockBackend *blk;
+    Error *local_err = NULL;
+    QDict *options = NULL;
+
+    options = qdict_new();
+    if (fmt) {
+        qdict_put(options, "driver", qstring_from_str(fmt));
+    }
+
+    qemu_opt_foreach(create_opts, img_add_key_secrets, options, NULL);
+
+    blk = blk_new_open(filename, NULL, options, flags, &local_err);
+    if (!blk) {
+        error_reportf_err(local_err, "Could not open '%s': ", filename);
+        return NULL;
+    }
+    blk_set_enable_write_cache(blk, !writethrough);
+
+    return blk;
+}
+
+
 static BlockBackend *img_open(bool image_opts,
                               const char *filename,
                               const char *fmt, int flags, bool writethrough,
@@ -2120,8 +2160,8 @@ static int img_convert(int argc, char **argv)
          * That has to wait for bdrv_create to be improved
          * to allow filenames in option syntax
          */
-        out_blk = img_open_file(out_filename, out_fmt,
-                                flags, writethrough, quiet);
+        out_blk = img_open_new_file(out_filename, opts, out_fmt,
+                                    flags, writethrough, quiet);
     }
     if (!out_blk) {
         ret = -1;
-- 
2.9.3

Re: [Qemu-devel] [PATCH v3 4/4] qemu-img: copy *key-secret opts when opening newly created files

[Kevin Wolf]

Am 20.02.2017 um 16:19 hat Daniel P. Berrange geschrieben:
> The qemu-img dd/convert commands will create a image file and
> then try to open it. Historically it has been possible to open
> new files without passing any options. With encrypted files
> though, the *key-secret options are mandatory, so we need to
> provide those options when opening the newly created file.
> 
> Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
> ---
>  qemu-img.c | 44 ++++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 42 insertions(+), 2 deletions(-)
> 
> diff --git a/qemu-img.c b/qemu-img.c
> index e48e676..bad19fd 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -317,6 +317,46 @@ static BlockBackend *img_open_file(const char *filename,
>  }
>  
>  
> +static int img_add_key_secrets(void *opaque,
> +                               const char *name, const char *value,
> +                               Error **errp)
> +{
> +    QDict *options = opaque;
> +
> +    if (g_str_has_suffix(name, "key-secret")) {
> +        qdict_put(options, name, qstring_from_str(value));
> +    }
> +
> +    return 0;
> +}
> +
> +static BlockBackend *img_open_new_file(const char *filename,
> +                                       QemuOpts *create_opts,
> +                                       const char *fmt, int flags,
> +                                       bool writethrough, bool quiet)
> +{
> +    BlockBackend *blk;
> +    Error *local_err = NULL;
> +    QDict *options = NULL;
> +
> +    options = qdict_new();
> +    if (fmt) {
> +        qdict_put(options, "driver", qstring_from_str(fmt));
> +    }
> +
> +    qemu_opt_foreach(create_opts, img_add_key_secrets, options, NULL);
> +
> +    blk = blk_new_open(filename, NULL, options, flags, &local_err);
> +    if (!blk) {
> +        error_reportf_err(local_err, "Could not open '%s': ", filename);
> +        return NULL;
> +    }
> +    blk_set_enable_write_cache(blk, !writethrough);
> +
> +    return blk;
> +}

Why not make this a small wrapper around img_open_file(), which does
almost the same except that it can ask for a password? Leaving out the
img_open_password() call means that simple '-o encryption=on' breaks,
so it's a bug anyway:

$ ./qemu-img convert -O qcow2 -o encryption ~/images/hd.img /tmp/crypt.qcow2
qemu-img: block/qcow2.c:1613: qcow2_co_pwritev: Assertion `s->cipher' failed.

Kevin

Re: [Qemu-devel] [PATCH v3 4/4] qemu-img: copy *key-secret opts when opening newly created files

[Daniel P. Berrange]

On Wed, Feb 22, 2017 at 12:20:36PM +0100, Kevin Wolf wrote:
> Am 20.02.2017 um 16:19 hat Daniel P. Berrange geschrieben:
> > The qemu-img dd/convert commands will create a image file and
> > then try to open it. Historically it has been possible to open
> > new files without passing any options. With encrypted files
> > though, the *key-secret options are mandatory, so we need to
> > provide those options when opening the newly created file.
> > 
> > Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
> > ---
> >  qemu-img.c | 44 ++++++++++++++++++++++++++++++++++++++++++--
> >  1 file changed, 42 insertions(+), 2 deletions(-)
> > 
> > diff --git a/qemu-img.c b/qemu-img.c
> > index e48e676..bad19fd 100644
> > --- a/qemu-img.c
> > +++ b/qemu-img.c
> > @@ -317,6 +317,46 @@ static BlockBackend *img_open_file(const char *filename,
> >  }
> >  
> >  
> > +static int img_add_key_secrets(void *opaque,
> > +                               const char *name, const char *value,
> > +                               Error **errp)
> > +{
> > +    QDict *options = opaque;
> > +
> > +    if (g_str_has_suffix(name, "key-secret")) {
> > +        qdict_put(options, name, qstring_from_str(value));
> > +    }
> > +
> > +    return 0;
> > +}
> > +
> > +static BlockBackend *img_open_new_file(const char *filename,
> > +                                       QemuOpts *create_opts,
> > +                                       const char *fmt, int flags,
> > +                                       bool writethrough, bool quiet)
> > +{
> > +    BlockBackend *blk;
> > +    Error *local_err = NULL;
> > +    QDict *options = NULL;
> > +
> > +    options = qdict_new();
> > +    if (fmt) {
> > +        qdict_put(options, "driver", qstring_from_str(fmt));
> > +    }
> > +
> > +    qemu_opt_foreach(create_opts, img_add_key_secrets, options, NULL);
> > +
> > +    blk = blk_new_open(filename, NULL, options, flags, &local_err);
> > +    if (!blk) {
> > +        error_reportf_err(local_err, "Could not open '%s': ", filename);
> > +        return NULL;
> > +    }
> > +    blk_set_enable_write_cache(blk, !writethrough);
> > +
> > +    return blk;
> > +}
> 
> Why not make this a small wrapper around img_open_file(), which does
> almost the same except that it can ask for a password? Leaving out the
> img_open_password() call means that simple '-o encryption=on' breaks,
> so it's a bug anyway:
> 
> $ ./qemu-img convert -O qcow2 -o encryption ~/images/hd.img /tmp/crypt.qcow2
> qemu-img: block/qcow2.c:1613: qcow2_co_pwritev: Assertion `s->cipher' failed.

I had written this after my conversion of qcow2 to use secrets, but I
presume you just tested this series in isolation. If this series merges
before my qcow2+luks series, then yeah, we'd need to handle the scearnio
you describe.

Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://entangle-photo.org       -o-    http://search.cpan.org/~danberr/ :|

Re: [Qemu-devel] [PATCH v3 4/4] qemu-img: copy *key-secret opts when opening newly created files

[Kevin Wolf]

Am 22.02.2017 um 12:33 hat Daniel P. Berrange geschrieben:
> On Wed, Feb 22, 2017 at 12:20:36PM +0100, Kevin Wolf wrote:
> > Am 20.02.2017 um 16:19 hat Daniel P. Berrange geschrieben:
> > > The qemu-img dd/convert commands will create a image file and
> > > then try to open it. Historically it has been possible to open
> > > new files without passing any options. With encrypted files
> > > though, the *key-secret options are mandatory, so we need to
> > > provide those options when opening the newly created file.
> > > 
> > > Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
> > > ---
> > >  qemu-img.c | 44 ++++++++++++++++++++++++++++++++++++++++++--
> > >  1 file changed, 42 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/qemu-img.c b/qemu-img.c
> > > index e48e676..bad19fd 100644
> > > --- a/qemu-img.c
> > > +++ b/qemu-img.c
> > > @@ -317,6 +317,46 @@ static BlockBackend *img_open_file(const char *filename,
> > >  }
> > >  
> > >  
> > > +static int img_add_key_secrets(void *opaque,
> > > +                               const char *name, const char *value,
> > > +                               Error **errp)
> > > +{
> > > +    QDict *options = opaque;
> > > +
> > > +    if (g_str_has_suffix(name, "key-secret")) {
> > > +        qdict_put(options, name, qstring_from_str(value));
> > > +    }
> > > +
> > > +    return 0;
> > > +}
> > > +
> > > +static BlockBackend *img_open_new_file(const char *filename,
> > > +                                       QemuOpts *create_opts,
> > > +                                       const char *fmt, int flags,
> > > +                                       bool writethrough, bool quiet)
> > > +{
> > > +    BlockBackend *blk;
> > > +    Error *local_err = NULL;
> > > +    QDict *options = NULL;
> > > +
> > > +    options = qdict_new();
> > > +    if (fmt) {
> > > +        qdict_put(options, "driver", qstring_from_str(fmt));
> > > +    }
> > > +
> > > +    qemu_opt_foreach(create_opts, img_add_key_secrets, options, NULL);
> > > +
> > > +    blk = blk_new_open(filename, NULL, options, flags, &local_err);
> > > +    if (!blk) {
> > > +        error_reportf_err(local_err, "Could not open '%s': ", filename);
> > > +        return NULL;
> > > +    }
> > > +    blk_set_enable_write_cache(blk, !writethrough);
> > > +
> > > +    return blk;
> > > +}
> > 
> > Why not make this a small wrapper around img_open_file(), which does
> > almost the same except that it can ask for a password? Leaving out the
> > img_open_password() call means that simple '-o encryption=on' breaks,
> > so it's a bug anyway:
> > 
> > $ ./qemu-img convert -O qcow2 -o encryption ~/images/hd.img /tmp/crypt.qcow2
> > qemu-img: block/qcow2.c:1613: qcow2_co_pwritev: Assertion `s->cipher' failed.
> 
> I had written this after my conversion of qcow2 to use secrets, but I
> presume you just tested this series in isolation. If this series merges
> before my qcow2+luks series, then yeah, we'd need to handle the scearnio
> you describe.

I see. Your commit message doesn't make clear that it depends on the
qcow2 series, so I was thinking that this could go into 2.9 even if the
qcow2 series might not make it (I think it touches the I/O path, so I'm
a bit more cautious there so short before the freeze).

Anyway, doesn't the wrapper instead of duplicating code make sense
anyway, even if the duplication didn't result in a bug?

Kevin

Re: [Qemu-devel] [PATCH v3 4/4] qemu-img: copy *key-secret opts when opening newly created files

[Daniel P. Berrange]

On Wed, Feb 22, 2017 at 01:18:49PM +0100, Kevin Wolf wrote:
> Am 22.02.2017 um 12:33 hat Daniel P. Berrange geschrieben:
> > On Wed, Feb 22, 2017 at 12:20:36PM +0100, Kevin Wolf wrote:
> > > Am 20.02.2017 um 16:19 hat Daniel P. Berrange geschrieben:
> > > > The qemu-img dd/convert commands will create a image file and
> > > > then try to open it. Historically it has been possible to open
> > > > new files without passing any options. With encrypted files
> > > > though, the *key-secret options are mandatory, so we need to
> > > > provide those options when opening the newly created file.
> > > > 
> > > > Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
> > > > ---
> > > >  qemu-img.c | 44 ++++++++++++++++++++++++++++++++++++++++++--
> > > >  1 file changed, 42 insertions(+), 2 deletions(-)
> > > > 
> > > > diff --git a/qemu-img.c b/qemu-img.c
> > > > index e48e676..bad19fd 100644
> > > > --- a/qemu-img.c
> > > > +++ b/qemu-img.c
> > > > @@ -317,6 +317,46 @@ static BlockBackend *img_open_file(const char *filename,
> > > >  }
> > > >  
> > > >  
> > > > +static int img_add_key_secrets(void *opaque,
> > > > +                               const char *name, const char *value,
> > > > +                               Error **errp)
> > > > +{
> > > > +    QDict *options = opaque;
> > > > +
> > > > +    if (g_str_has_suffix(name, "key-secret")) {
> > > > +        qdict_put(options, name, qstring_from_str(value));
> > > > +    }
> > > > +
> > > > +    return 0;
> > > > +}
> > > > +
> > > > +static BlockBackend *img_open_new_file(const char *filename,
> > > > +                                       QemuOpts *create_opts,
> > > > +                                       const char *fmt, int flags,
> > > > +                                       bool writethrough, bool quiet)
> > > > +{
> > > > +    BlockBackend *blk;
> > > > +    Error *local_err = NULL;
> > > > +    QDict *options = NULL;
> > > > +
> > > > +    options = qdict_new();
> > > > +    if (fmt) {
> > > > +        qdict_put(options, "driver", qstring_from_str(fmt));
> > > > +    }
> > > > +
> > > > +    qemu_opt_foreach(create_opts, img_add_key_secrets, options, NULL);
> > > > +
> > > > +    blk = blk_new_open(filename, NULL, options, flags, &local_err);
> > > > +    if (!blk) {
> > > > +        error_reportf_err(local_err, "Could not open '%s': ", filename);
> > > > +        return NULL;
> > > > +    }
> > > > +    blk_set_enable_write_cache(blk, !writethrough);
> > > > +
> > > > +    return blk;
> > > > +}
> > > 
> > > Why not make this a small wrapper around img_open_file(), which does
> > > almost the same except that it can ask for a password? Leaving out the
> > > img_open_password() call means that simple '-o encryption=on' breaks,
> > > so it's a bug anyway:
> > > 
> > > $ ./qemu-img convert -O qcow2 -o encryption ~/images/hd.img /tmp/crypt.qcow2
> > > qemu-img: block/qcow2.c:1613: qcow2_co_pwritev: Assertion `s->cipher' failed.
> > 
> > I had written this after my conversion of qcow2 to use secrets, but I
> > presume you just tested this series in isolation. If this series merges
> > before my qcow2+luks series, then yeah, we'd need to handle the scearnio
> > you describe.
> 
> I see. Your commit message doesn't make clear that it depends on the
> qcow2 series, so I was thinking that this could go into 2.9 even if the
> qcow2 series might not make it (I think it touches the I/O path, so I'm
> a bit more cautious there so short before the freeze).

Yep, understood. It is wise to be cautious with crypto stuff too.

> Anyway, doesn't the wrapper instead of duplicating code make sense
> anyway, even if the duplication didn't result in a bug?

Ok, I see what you mean. I can repost with that change.

Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://entangle-photo.org       -o-    http://search.cpan.org/~danberr/ :|