[Qemu-devel] [PATCH v2 0/6] qemu-img: improve convert & dd commands

[Qemu-devel] [PATCH v2 0/6] qemu-img: improve convert & dd commands

[Daniel P. Berrange]

Update to

  v1: https://lists.gnu.org/archive/html/qemu-devel/2017-01/msg05699.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 four patches improve the 'dd' command to address
feature gaps wrt the 'convert' command. The last two patches
implement the improvements described above.

Changed in v2:

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

Daniel P. Berrange (6):
  qemu-img: add support for --object with 'dd' command
  qemu-img: fix --image-opts usage with dd command
  qemu-img: add support for conv=nocreat,notrunc args to dd command
  qemu-img: add support for -o arg to 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 |   8 +-
 qemu-img.c       | 342 ++++++++++++++++++++++++++++++++++++++++++++-----------
 qemu-img.texi    |  26 ++++-
 3 files changed, 299 insertions(+), 77 deletions(-)

-- 
2.9.3

[Qemu-devel] [PATCH v2 1/6] 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 | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/qemu-img.c b/qemu-img.c
index 74e3362..391a141 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -3949,6 +3949,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 }
     };
@@ -3971,6 +3972,14 @@ 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) {
+                return 1;
+            }
+        }   break;
         case OPTION_IMAGE_OPTS:
             image_opts = true;
             break;
@@ -4015,6 +4024,13 @@ 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)) {
+        return 1;
+    }
+
     blk1 = img_open(image_opts, in.filename, fmt, 0, false, false);
 
     if (!blk1) {
-- 
2.9.3

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

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 391a141..629f9e9 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -4098,8 +4098,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

[Qemu-devel] [PATCH v2 3/6] qemu-img: add support for conv=nocreat, notrunc args to dd command

[Daniel P. Berrange]

The -n arg to the convert command allows use of a pre-existing image,
rather than creating a new image. This adds equivalent functionality
to the dd command using the 'conv' arg. If 'conv=nocreat' is used,
then it will assume the image already exists. The existing image
will be truncated to match the required output size. 'conv=notrunc'
cna be used to preserve the existing image size.

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

diff --git a/qemu-img-cmds.hx b/qemu-img-cmds.hx
index f054599..b2c5424 100644
--- a/qemu-img-cmds.hx
+++ b/qemu-img-cmds.hx
@@ -46,9 +46,9 @@ STEXI
 ETEXI
 
 DEF("dd", img_dd,
-    "dd [--image-opts] [-f fmt] [-O output_fmt] [bs=block_size] [count=blocks] [skip=blocks] if=input of=output")
+    "dd [--image-opts] [-f fmt] [-O output_fmt] [bs=block_size] [count=blocks] [skip=blocks] [conv=nocreat,notrunc] if=input of=output")
 STEXI
-@item dd [--image-opts] [-f @var{fmt}] [-O @var{output_fmt}] [bs=@var{block_size}] [count=@var{blocks}] [skip=@var{blocks}] if=@var{input} of=@var{output}
+@item dd [--image-opts] [-f @var{fmt}] [-O @var{output_fmt}] [bs=@var{block_size}] [count=@var{blocks}] [skip=@var{blocks}] [conv=nocreat,notrunc] if=@var{input} of=@var{output}
 ETEXI
 
 DEF("info", img_info,
diff --git a/qemu-img.c b/qemu-img.c
index 629f9e9..c9ab9e5 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -174,7 +174,9 @@ static void QEMU_NORETURN help(void)
            "  'count=N' copy only N input blocks\n"
            "  'if=FILE' read from FILE\n"
            "  'of=FILE' write to FILE\n"
-           "  'skip=N' skip N bs-sized blocks at the start of input\n";
+           "  'skip=N' skip N bs-sized blocks at the start of input\n"
+           "  'conv=nocreat' don't create the output file\n"
+           "  'conv=notrunc' don't truncate the output file\n";
 
     printf("%s\nSupported formats:", help_msg);
     bdrv_iterate_format(format_print, NULL);
@@ -3814,11 +3816,13 @@ out:
     return 0;
 }
 
-#define C_BS      01
-#define C_COUNT   02
-#define C_IF      04
-#define C_OF      010
-#define C_SKIP    020
+#define C_BS      (1 << 0)
+#define C_COUNT   (1 << 1)
+#define C_IF      (1 << 2)
+#define C_OF      (1 << 3)
+#define C_SKIP    (1 << 4)
+#define C_NOCREAT (1 << 5)
+#define C_NOTRUNC (1 << 6)
 
 struct DdInfo {
     unsigned int flags;
@@ -3906,6 +3910,31 @@ static int img_dd_skip(const char *arg,
     return 0;
 }
 
+static int img_dd_conv(const char *arg,
+                       struct DdIo *in, struct DdIo *out,
+                       struct DdInfo *dd)
+{
+    char **flags, **tmp;
+
+    tmp = flags = g_strsplit(arg, ",", 0);
+
+    while (tmp && *tmp) {
+        if (g_str_equal(*tmp, "noconv")) {
+            dd->flags |= C_NOCREAT;
+        } else if (g_str_equal(*tmp, "notrunc")) {
+            dd->flags |= C_NOTRUNC;
+        } else {
+            error_report("invalid conv argument: '%s'", *tmp);
+            g_strfreev(flags);
+            return 1;
+        }
+        tmp++;
+    }
+
+    g_strfreev(flags);
+    return 0;
+}
+
 static int img_dd(int argc, char **argv)
 {
     int ret = 0;
@@ -3920,7 +3949,7 @@ static int img_dd(int argc, char **argv)
     int c, i;
     const char *out_fmt = "raw";
     const char *fmt = NULL;
-    int64_t size = 0;
+    int64_t size = 0, out_size;
     int64_t block_count = 0, out_pos, in_pos;
     struct DdInfo dd = {
         .flags = 0,
@@ -3945,6 +3974,7 @@ static int img_dd(int argc, char **argv)
         { "if", img_dd_if, C_IF },
         { "of", img_dd_of, C_OF },
         { "skip", img_dd_skip, C_SKIP },
+        { "conv", img_dd_conv, 0 },
         { NULL, NULL, 0 }
     };
     const struct option long_options[] = {
@@ -3954,7 +3984,7 @@ static int img_dd(int argc, char **argv)
         { 0, 0, 0, 0 }
     };
 
-    while ((c = getopt_long(argc, argv, "hf:O:", long_options, NULL))) {
+    while ((c = getopt_long(argc, argv, "hnf:O:", long_options, NULL))) {
         if (c == EOF) {
             break;
         }
@@ -4051,22 +4081,25 @@ static int img_dd(int argc, char **argv)
         ret = -1;
         goto out;
     }
-    if (!drv->create_opts) {
-        error_report("Format driver '%s' does not support image creation",
-                     drv->format_name);
-        ret = -1;
-        goto out;
-    }
-    if (!proto_drv->create_opts) {
-        error_report("Protocol driver '%s' does not support image creation",
-                     proto_drv->format_name);
-        ret = -1;
-        goto out;
-    }
-    create_opts = qemu_opts_append(create_opts, drv->create_opts);
-    create_opts = qemu_opts_append(create_opts, proto_drv->create_opts);
 
-    opts = qemu_opts_create(create_opts, NULL, 0, &error_abort);
+    if (!(dd.flags & C_NOCREAT)) {
+        if (!drv->create_opts) {
+            error_report("Format driver '%s' does not support image creation",
+                         drv->format_name);
+            ret = -1;
+            goto out;
+        }
+        if (!proto_drv->create_opts) {
+            error_report("Protocol driver '%s' does not support image creation",
+                         proto_drv->format_name);
+            ret = -1;
+            goto out;
+        }
+        create_opts = qemu_opts_append(create_opts, drv->create_opts);
+        create_opts = qemu_opts_append(create_opts, proto_drv->create_opts);
+
+        opts = qemu_opts_create(create_opts, NULL, 0, &error_abort);
+    }
 
     size = blk_getlength(blk1);
     if (size < 0) {
@@ -4083,19 +4116,22 @@ static int img_dd(int argc, char **argv)
     /* Overflow means the specified offset is beyond input image's size */
     if (dd.flags & C_SKIP && (in.offset > INT64_MAX / in.bsz ||
                               size < in.bsz * in.offset)) {
-        qemu_opt_set_number(opts, BLOCK_OPT_SIZE, 0, &error_abort);
+        out_size = 0;
     } else {
-        qemu_opt_set_number(opts, BLOCK_OPT_SIZE,
-                            size - in.bsz * in.offset, &error_abort);
+        out_size = size - in.bsz * in.offset;
     }
 
-    ret = bdrv_create(drv, out.filename, opts, &local_err);
-    if (ret < 0) {
-        error_reportf_err(local_err,
-                          "%s: error while creating output image: ",
-                          out.filename);
-        ret = -1;
-        goto out;
+    if (!(dd.flags & C_NOCREAT)) {
+        qemu_opt_set_number(opts, BLOCK_OPT_SIZE, out_size, &error_abort);
+
+        ret = bdrv_create(drv, out.filename, opts, &local_err);
+        if (ret < 0) {
+            error_reportf_err(local_err,
+                              "%s: error while creating output image: ",
+                              out.filename);
+            ret = -1;
+            goto out;
+        }
     }
 
     /* TODO, we can't honour --image-opts for the target,
@@ -4111,6 +4147,41 @@ static int img_dd(int argc, char **argv)
         goto out;
     }
 
+    if (dd.flags & C_NOCREAT) {
+        if (dd.flags & C_NOTRUNC) {
+            int64_t existing_size = blk_getlength(blk2);
+            if (existing_size < 0) {
+                error_report("unable to get output image length: %s",
+                             strerror(-existing_size));
+                ret = -1;
+                goto out;
+            } else if (existing_size < out_size) {
+                /* Not large enough, so we must enlarge it */
+                ret = blk_truncate(blk2, out_size);
+                if (ret < 0) {
+                    error_reportf_err(local_err,
+                                      "%s: error while enlarging output image: ",
+                                      out.filename);
+                    ret = -1;
+                    goto out;
+                }
+            }
+        } else {
+            /* dd would truncate to 0 length, then append out_size
+             * worth of data. Our images don't grow on demand, so
+             * we just truncate to final output size straight away
+             */
+            ret = blk_truncate(blk2, out_size);
+            if (ret < 0) {
+                error_reportf_err(local_err,
+                                  "%s: error while truncating output image: ",
+                                  out.filename);
+                ret = -1;
+                goto out;
+            }
+        }
+    }
+
     if (dd.flags & C_SKIP && (in.offset > INT64_MAX / in.bsz ||
                               size < in.offset * in.bsz)) {
         /* We give a warning if the skip option is bigger than the input
diff --git a/qemu-img.texi b/qemu-img.texi
index 174aae3..9f10562 100644
--- a/qemu-img.texi
+++ b/qemu-img.texi
@@ -326,7 +326,7 @@ skipped. This is useful for formats such as @code{rbd} if the target
 volume has already been created with site specific options that cannot
 be supplied through qemu-img.
 
-@item dd [-f @var{fmt}] [-O @var{output_fmt}] [bs=@var{block_size}] [count=@var{blocks}] [skip=@var{blocks}] if=@var{input} of=@var{output}
+@item dd [-f @var{fmt}] [-O @var{output_fmt}] [bs=@var{block_size}] [count=@var{blocks}] [skip=@var{blocks}] [conv=nocreat,notrunc] if=@var{input} of=@var{output}
 
 Dd copies from @var{input} file to @var{output} file converting it from
 @var{fmt} format to @var{output_fmt} format.
@@ -337,6 +337,14 @@ dd will stop reading input after reading @var{blocks} input blocks.
 
 The size syntax is similar to dd(1)'s size syntax.
 
+If the @code{conv=nocreat} option is specified, the target volume creation
+will be skipped. Its length will be truncated to match data length, if it
+is longer than the required data size. If the @code{conv=notrunc} option
+is specified, no file size shrinking will be done. If the existing output
+file is too small it will be enlarged to fit. These options are useful for
+formats such as @code{rbd} if the target volume has already been created
+with site specific options that cannot be supplied through qemu-img.
+
 @item info [-f @var{fmt}] [--output=@var{ofmt}] [--backing-chain] @var{filename}
 
 Give information about the disk image @var{filename}. Use it in
-- 
2.9.3

[Qemu-devel] [PATCH v2 4/6] qemu-img: add support for -o arg to dd command

[Daniel P. Berrange]

The -o arg to the convert command allows specification of format/protocol
options for the newly created image. This adds a -o arg to the dd command
to get feature parity.

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

diff --git a/qemu-img-cmds.hx b/qemu-img-cmds.hx
index b2c5424..2488cbe 100644
--- a/qemu-img-cmds.hx
+++ b/qemu-img-cmds.hx
@@ -46,7 +46,7 @@ STEXI
 ETEXI
 
 DEF("dd", img_dd,
-    "dd [--image-opts] [-f fmt] [-O output_fmt] [bs=block_size] [count=blocks] [skip=blocks] [conv=nocreat,notrunc] if=input of=output")
+    "dd [--image-opts] [-f fmt] [-O output_fmt] [-o options] [bs=block_size] [count=blocks] [skip=blocks] [conv=nocreat,notrunc] if=input of=output")
 STEXI
 @item dd [--image-opts] [-f @var{fmt}] [-O @var{output_fmt}] [bs=@var{block_size}] [count=@var{blocks}] [skip=@var{blocks}] [conv=nocreat,notrunc] if=@var{input} of=@var{output}
 ETEXI
diff --git a/qemu-img.c b/qemu-img.c
index c9ab9e5..39fcf09 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -3949,6 +3949,7 @@ static int img_dd(int argc, char **argv)
     int c, i;
     const char *out_fmt = "raw";
     const char *fmt = NULL;
+    char *optionstr = NULL;
     int64_t size = 0, out_size;
     int64_t block_count = 0, out_pos, in_pos;
     struct DdInfo dd = {
@@ -3984,7 +3985,7 @@ static int img_dd(int argc, char **argv)
         { 0, 0, 0, 0 }
     };
 
-    while ((c = getopt_long(argc, argv, "hnf:O:", long_options, NULL))) {
+    while ((c = getopt_long(argc, argv, "hno:f:O:", long_options, NULL))) {
         if (c == EOF) {
             break;
         }
@@ -3995,6 +3996,20 @@ static int img_dd(int argc, char **argv)
         case 'f':
             fmt = optarg;
             break;
+        case 'o':
+            if (!is_valid_option_list(optarg)) {
+                error_report("Invalid option list: %s", optarg);
+                ret = -1;
+                goto out;
+            }
+            if (!optionstr) {
+                optionstr = g_strdup(optarg);
+            } else {
+                char *old_options = optionstr;
+                optionstr = g_strdup_printf("%s,%s", optionstr, optarg);
+                g_free(old_options);
+            }
+            break;
         case '?':
             error_report("Try 'qemu-img --help' for more information.");
             ret = -1;
@@ -4055,6 +4070,11 @@ static int img_dd(int argc, char **argv)
         goto out;
     }
 
+    if (optionstr && has_help_option(optionstr)) {
+        ret = print_block_option_help(out.filename, out_fmt);
+        goto out;
+    }
+
     if (qemu_opts_foreach(&qemu_object_opts,
                           user_creatable_add_opts_foreach,
                           NULL, NULL)) {
@@ -4099,6 +4119,15 @@ static int img_dd(int argc, char **argv)
         create_opts = qemu_opts_append(create_opts, proto_drv->create_opts);
 
         opts = qemu_opts_create(create_opts, NULL, 0, &error_abort);
+
+        if (optionstr) {
+            qemu_opts_do_parse(opts, optionstr, NULL, &local_err);
+            if (local_err) {
+                error_report_err(local_err);
+                ret = -1;
+                goto out;
+            }
+        }
     }
 
     size = blk_getlength(blk1);
@@ -4224,6 +4253,7 @@ static int img_dd(int argc, char **argv)
 
 out:
     g_free(arg);
+    g_free(optionstr);
     qemu_opts_del(opts);
     qemu_opts_free(create_opts);
     blk_unref(blk1);
diff --git a/qemu-img.texi b/qemu-img.texi
index 9f10562..01acfb8 100644
--- a/qemu-img.texi
+++ b/qemu-img.texi
@@ -326,10 +326,12 @@ skipped. This is useful for formats such as @code{rbd} if the target
 volume has already been created with site specific options that cannot
 be supplied through qemu-img.
 
-@item dd [-f @var{fmt}] [-O @var{output_fmt}] [bs=@var{block_size}] [count=@var{blocks}] [skip=@var{blocks}] [conv=nocreat,notrunc] if=@var{input} of=@var{output}
+@item dd [-f @var{fmt}] [-O @var{output_fmt}] [-o @var{options}] [bs=@var{block_size}] [count=@var{blocks}] [skip=@var{blocks}] [conv=nocreat,notrunc] if=@var{input} of=@var{output}
 
 Dd copies from @var{input} file to @var{output} file converting it from
-@var{fmt} format to @var{output_fmt} format.
+@var{fmt} format to @var{output_fmt} format. Depending on the output file
+format, you can add one or more @var{options} that enable additional
+features of this format.
 
 The data is by default read and written using blocks of 512 bytes but can be
 modified by specifying @var{block_size}. If count=@var{blocks} is specified
-- 
2.9.3

[Qemu-devel] [PATCH v2 5/6] 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 |   6 +--
 qemu-img.c       | 131 ++++++++++++++++++++++++++++++++++++-------------------
 qemu-img.texi    |  12 ++++-
 3 files changed, 98 insertions(+), 51 deletions(-)

diff --git a/qemu-img-cmds.hx b/qemu-img-cmds.hx
index 2488cbe..13fded9 100644
--- a/qemu-img-cmds.hx
+++ b/qemu-img-cmds.hx
@@ -40,13 +40,13 @@ 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,
-    "dd [--image-opts] [-f fmt] [-O output_fmt] [-o options] [bs=block_size] [count=blocks] [skip=blocks] [conv=nocreat,notrunc] if=input of=output")
+    "dd [--image-opts] [--target-image-opts] [-f fmt] [-O output_fmt] [-o options] [bs=block_size] [count=blocks] [skip=blocks] [conv=nocreat,notrunc] if=input of=output")
 STEXI
 @item dd [--image-opts] [-f @var{fmt}] [-O @var{output_fmt}] [bs=@var{block_size}] [count=@var{blocks}] [skip=@var{blocks}] [conv=nocreat,notrunc] if=@var{input} of=@var{output}
 ETEXI
diff --git a/qemu-img.c b/qemu-img.c
index 39fcf09..dc4c6eb 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;
@@ -1918,7 +1933,7 @@ static int img_convert(int argc, char **argv)
     bs_n = argc - optind - 1;
     out_filename = bs_n >= 1 ? argv[argc - 1] : NULL;
 
-    if (options && has_help_option(options)) {
+    if (out_fmt && options && has_help_option(options)) {
         ret = print_block_option_help(out_filename, out_fmt);
         goto out;
     }
@@ -1987,22 +2002,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);
@@ -2091,12 +2106,18 @@ 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;
@@ -3946,8 +3967,9 @@ static int img_dd(int argc, char **argv)
     QemuOptsList *create_opts = NULL;
     Error *local_err = NULL;
     bool image_opts = false;
+    bool tgt_image_opts = false;
     int c, i;
-    const char *out_fmt = "raw";
+    const char *out_fmt = NULL;
     const char *fmt = NULL;
     char *optionstr = NULL;
     int64_t size = 0, out_size;
@@ -3982,6 +4004,7 @@ static int img_dd(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 }
     };
 
@@ -4028,6 +4051,9 @@ static int img_dd(int argc, char **argv)
         case OPTION_IMAGE_OPTS:
             image_opts = true;
             break;
+        case OPTION_TARGET_IMAGE_OPTS:
+            tgt_image_opts = true;
+            break;
         }
     }
 
@@ -4064,13 +4090,22 @@ static int img_dd(int argc, char **argv)
         arg = NULL;
     }
 
+    if (tgt_image_opts && !(dd.flags & C_NOCREAT)) {
+        error_report("--target-image-opts requires use of -n flag");
+        goto out;
+    }
+
+    if (!out_fmt && !tgt_image_opts) {
+        out_fmt = "raw";
+    }
+
     if (!(dd.flags & C_IF && dd.flags & C_OF)) {
         error_report("Must specify both input and output files");
         ret = -1;
         goto out;
     }
 
-    if (optionstr && has_help_option(optionstr)) {
+    if (out_fmt && optionstr && has_help_option(optionstr)) {
         ret = print_block_option_help(out.filename, out_fmt);
         goto out;
     }
@@ -4088,21 +4123,21 @@ static int img_dd(int argc, char **argv)
         goto out;
     }
 
-    drv = bdrv_find_format(out_fmt);
-    if (!drv) {
-        error_report("Unknown file format");
-        ret = -1;
-        goto out;
-    }
-    proto_drv = bdrv_find_protocol(out.filename, true, &local_err);
+    if (!(dd.flags & C_NOCREAT)) {
+        drv = bdrv_find_format(out_fmt);
+        if (!drv) {
+            error_report("Unknown file format");
+            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 (!proto_drv) {
+            error_report_err(local_err);
+            ret = -1;
+            goto out;
+        }
 
-    if (!(dd.flags & C_NOCREAT)) {
         if (!drv->create_opts) {
             error_report("Format driver '%s' does not support image creation",
                          drv->format_name);
@@ -4152,7 +4187,6 @@ static int img_dd(int argc, char **argv)
 
     if (!(dd.flags & C_NOCREAT)) {
         qemu_opt_set_number(opts, BLOCK_OPT_SIZE, out_size, &error_abort);
-
         ret = bdrv_create(drv, out.filename, opts, &local_err);
         if (ret < 0) {
             error_reportf_err(local_err,
@@ -4163,13 +4197,18 @@ static int img_dd(int argc, char **argv)
         }
     }
 
-    /* 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 (dd.flags & C_NOCREAT) {
+        blk2 = img_open(tgt_image_opts, out.filename, out_fmt,
+                        BDRV_O_RDWR, false, false);
+    } 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
+         */
+        blk2 = img_open_file(out.filename, out_fmt,
+                             BDRV_O_RDWR, false, false);
+    }
 
     if (!blk2) {
         ret = -1;
diff --git a/qemu-img.texi b/qemu-img.texi
index 01acfb8..bda3cc3 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{-F} or @var{-O} parameters. It is currently required
+to also use the @var{-n} parameter to skip image creation. This restriction
+will 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

[Qemu-devel] [PATCH v2 6/6] 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 | 51 +++++++++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 47 insertions(+), 4 deletions(-)

diff --git a/qemu-img.c b/qemu-img.c
index dc4c6eb..98522dd 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -319,6 +319,49 @@ 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")) {
+        if (!*options) {
+            *options = qdict_new();
+        }
+        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;
+
+    if (fmt) {
+        options = qdict_new();
+        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,
@@ -2115,8 +2158,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;
@@ -4206,8 +4249,8 @@ static int img_dd(int argc, char **argv)
          * That has to wait for bdrv_create to be improved
          * to allow filenames in option syntax
          */
-        blk2 = img_open_file(out.filename, out_fmt,
-                             BDRV_O_RDWR, false, false);
+        blk2 = img_open_new_file(out.filename, opts, out_fmt,
+                                 BDRV_O_RDWR, false, false);
     }
 
     if (!blk2) {
-- 
2.9.3

Re: [Qemu-devel] [PATCH v2 1/6] qemu-img: add support for --object with 'dd' command

[Max Reitz]

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

On 03.02.2017 13:02, Daniel P. Berrange wrote:
> 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 | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
> 
> diff --git a/qemu-img.c b/qemu-img.c
> index 74e3362..391a141 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -3949,6 +3949,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 }
>      };
> @@ -3971,6 +3972,14 @@ 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) {
> +                return 1;
> +            }
> +        }   break;
>          case OPTION_IMAGE_OPTS:
>              image_opts = true;
>              break;
> @@ -4015,6 +4024,13 @@ 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)) {
> +        return 1;

Why not ret = -1; goto out; like the other code around this block?

(Same for the case block above.)

Max

> +    }
> +
>      blk1 = img_open(image_opts, in.filename, fmt, 0, false, false);
>  
>      if (!blk1) {
> 



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

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

[Max Reitz]

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

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

Reviewed-by: Max Reitz <mreitz@redhat.com>


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

Re: [Qemu-devel] [PATCH v2 3/6] qemu-img: add support for conv=nocreat, notrunc args to dd command

[Max Reitz]

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

On 03.02.2017 13:02, Daniel P. Berrange wrote:
> The -n arg to the convert command allows use of a pre-existing image,
> rather than creating a new image. This adds equivalent functionality
> to the dd command using the 'conv' arg. If 'conv=nocreat' is used,
> then it will assume the image already exists. The existing image
> will be truncated to match the required output size. 'conv=notrunc'
> cna be used to preserve the existing image size.
> 
> Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
> ---
>  qemu-img-cmds.hx |   4 +-
>  qemu-img.c       | 137 +++++++++++++++++++++++++++++++++++++++++--------------
>  qemu-img.texi    |  10 +++-
>  3 files changed, 115 insertions(+), 36 deletions(-)

Quite frankly I don't like this patch very much. It's not bad in itself,
but I don't like the idea of giving qemu-img dd new features until it's
an interface for qemu-img convert. Everything that we add now encourages
new users to use it and will make the conversion a bit more difficult.

As long as qemu-img convert gets a --target-image-opts, I don't think we
need all of this functionality in qemu-img dd.

Anyway, I won't block/NACK this patch, so resuming review.

> diff --git a/qemu-img-cmds.hx b/qemu-img-cmds.hx
> index f054599..b2c5424 100644
> --- a/qemu-img-cmds.hx
> +++ b/qemu-img-cmds.hx
> @@ -46,9 +46,9 @@ STEXI
>  ETEXI
>  
>  DEF("dd", img_dd,
> -    "dd [--image-opts] [-f fmt] [-O output_fmt] [bs=block_size] [count=blocks] [skip=blocks] if=input of=output")
> +    "dd [--image-opts] [-f fmt] [-O output_fmt] [bs=block_size] [count=blocks] [skip=blocks] [conv=nocreat,notrunc] if=input of=output")
>  STEXI
> -@item dd [--image-opts] [-f @var{fmt}] [-O @var{output_fmt}] [bs=@var{block_size}] [count=@var{blocks}] [skip=@var{blocks}] if=@var{input} of=@var{output}
> +@item dd [--image-opts] [-f @var{fmt}] [-O @var{output_fmt}] [bs=@var{block_size}] [count=@var{blocks}] [skip=@var{blocks}] [conv=nocreat,notrunc] if=@var{input} of=@var{output}

I'd just write something like conv=@var{convs} or I don't know. There
are other conversion specifiers (or however it's called) that we may
want to support in the future, e.g. sparse or noerror.

>  ETEXI
>  
>  DEF("info", img_info,
> diff --git a/qemu-img.c b/qemu-img.c
> index 629f9e9..c9ab9e5 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c


[...]

> @@ -3906,6 +3910,31 @@ static int img_dd_skip(const char *arg,
>      return 0;
>  }
>  
> +static int img_dd_conv(const char *arg,
> +                       struct DdIo *in, struct DdIo *out,
> +                       struct DdInfo *dd)
> +{
> +    char **flags, **tmp;
> +
> +    tmp = flags = g_strsplit(arg, ",", 0);

"flag_pointer", "cur_flag_pointer" or "flat_iterator" instead of "tmp"
might be more suitable names.

> +
> +    while (tmp && *tmp) {
> +        if (g_str_equal(*tmp, "noconv")) {
> +            dd->flags |= C_NOCREAT;
> +        } else if (g_str_equal(*tmp, "notrunc")) {
> +            dd->flags |= C_NOTRUNC;
> +        } else {
> +            error_report("invalid conv argument: '%s'", *tmp);
> +            g_strfreev(flags);
> +            return 1;
> +        }
> +        tmp++;
> +    }
> +
> +    g_strfreev(flags);
> +    return 0;
> +}
> +
>  static int img_dd(int argc, char **argv)
>  {
>      int ret = 0;

[...]

> @@ -3954,7 +3984,7 @@ static int img_dd(int argc, char **argv)
>          { 0, 0, 0, 0 }
>      };
>  
> -    while ((c = getopt_long(argc, argv, "hf:O:", long_options, NULL))) {
> +    while ((c = getopt_long(argc, argv, "hnf:O:", long_options, NULL))) {

Looks like a relic from v1.

>          if (c == EOF) {
>              break;
>          }

[...]

> diff --git a/qemu-img.texi b/qemu-img.texi
> index 174aae3..9f10562 100644
> --- a/qemu-img.texi
> +++ b/qemu-img.texi
> @@ -326,7 +326,7 @@ skipped. This is useful for formats such as @code{rbd} if the target
>  volume has already been created with site specific options that cannot
>  be supplied through qemu-img.
>  
> -@item dd [-f @var{fmt}] [-O @var{output_fmt}] [bs=@var{block_size}] [count=@var{blocks}] [skip=@var{blocks}] if=@var{input} of=@var{output}
> +@item dd [-f @var{fmt}] [-O @var{output_fmt}] [bs=@var{block_size}] [count=@var{blocks}] [skip=@var{blocks}] [conv=nocreat,notrunc] if=@var{input} of=@var{output}
>  
>  Dd copies from @var{input} file to @var{output} file converting it from
>  @var{fmt} format to @var{output_fmt} format.
> @@ -337,6 +337,14 @@ dd will stop reading input after reading @var{blocks} input blocks.
>  
>  The size syntax is similar to dd(1)'s size syntax.
>  
> +If the @code{conv=nocreat} option is specified, the target volume creation
> +will be skipped. Its length will be truncated to match data length, if it
> +is longer than the required data size. If the @code{conv=notrunc} option
> +is specified, no file size shrinking will be done. If the existing output
> +file is too small it will be enlarged to fit. These options are useful for
> +formats such as @code{rbd} if the target volume has already been created
> +with site specific options that cannot be supplied through qemu-img.
> +

Good enough for now, but a list/table would probably be more extensible
in the future.

So all in all a patch that does well what it's supposed to do -- I just
don't quite agree on whether that goal is right.

Max

>  @item info [-f @var{fmt}] [--output=@var{ofmt}] [--backing-chain] @var{filename}
>  
>  Give information about the disk image @var{filename}. Use it in
> 



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

Re: [Qemu-devel] [PATCH v2 4/6] qemu-img: add support for -o arg to dd command

[Max Reitz]

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

On 03.02.2017 13:02, Daniel P. Berrange wrote:
> The -o arg to the convert command allows specification of format/protocol
> options for the newly created image. This adds a -o arg to the dd command
> to get feature parity.
> 
> Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
> ---
>  qemu-img-cmds.hx |  2 +-
>  qemu-img.c       | 32 +++++++++++++++++++++++++++++++-
>  qemu-img.texi    |  6 ++++--
>  3 files changed, 36 insertions(+), 4 deletions(-)

I don't like this patch for the same reasons as for patch 3, but I like
it a bit better. The code introduced here is exactly the same as for
img_convert(), so merging the two would (or "is going to", I hope) be
trivial.

So a pretty weak

Reviewed-by: Max Reitz <mreitz@redhat.com>


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

Re: [Qemu-devel] [PATCH v2 5/6] qemu-img: introduce --target-image-opts for 'convert' command

[Max Reitz]

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

On 03.02.2017 13:02, 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 |   6 +--
>  qemu-img.c       | 131 ++++++++++++++++++++++++++++++++++++-------------------
>  qemu-img.texi    |  12 ++++-
>  3 files changed, 98 insertions(+), 51 deletions(-)

Apart from what the commit message says, it also introduces that switch
for dd, which I again don't like too much (quelle surprise), if only
because it requires conv=nocreat and thus patch 5 to be useful.

[...]

> diff --git a/qemu-img.c b/qemu-img.c
> index 39fcf09..dc4c6eb 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c

[...]

> @@ -1918,7 +1933,7 @@ static int img_convert(int argc, char **argv)
>      bs_n = argc - optind - 1;
>      out_filename = bs_n >= 1 ? argv[argc - 1] : NULL;
>  
> -    if (options && has_help_option(options)) {
> +    if (out_fmt && options && has_help_option(options)) {

"!out_fmt && options && has_help_option(options)" should probably be an
error.

>          ret = print_block_option_help(out_filename, out_fmt);
>          goto out;
>      }
> @@ -1987,22 +2002,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);

Compression may be used with -n. This involves the check whether
drv->bdrv_co_pwritev_compressed is NULL or not -- which is bad if drv is
still NULL:

$ ./qemu-img create -f qcow2 foo.qcow2 64M
Formatting 'foo.qcow2', fmt=qcow2 size=67108864 encryption=off
cluster_size=65536 lazy_refcounts=off refcount_bits=16
$ ./qemu-img convert -c -O qcow2 -n null-co:// foo.qcow2
[1]    17179 segmentation fault (core dumped)  ./qemu-img convert -c -O
qcow2 -n null-co:// foo.qcow

Therefore, you should probably only do the check whether compression is
supported if drv is non-NULL; and if it is NULL, do the check again
after the target image has been opened and its driver is known.

[...]

> @@ -4064,13 +4090,22 @@ static int img_dd(int argc, char **argv)
>          arg = NULL;
>      }
>  
> +    if (tgt_image_opts && !(dd.flags & C_NOCREAT)) {
> +        error_report("--target-image-opts requires use of -n flag");

*conv=nocreat

> +        goto out;
> +    }
> +
> +    if (!out_fmt && !tgt_image_opts) {
> +        out_fmt = "raw";
> +    }
> +
>      if (!(dd.flags & C_IF && dd.flags & C_OF)) {
>          error_report("Must specify both input and output files");
>          ret = -1;
>          goto out;
>      }
>  
> -    if (optionstr && has_help_option(optionstr)) {
> +    if (out_fmt && optionstr && has_help_option(optionstr)) {

Same as in img_convert().

>          ret = print_block_option_help(out.filename, out_fmt);
>          goto out;
>      }

[...]

> @@ -4152,7 +4187,6 @@ static int img_dd(int argc, char **argv)
>  
>      if (!(dd.flags & C_NOCREAT)) {
>          qemu_opt_set_number(opts, BLOCK_OPT_SIZE, out_size, &error_abort);
> -
>          ret = bdrv_create(drv, out.filename, opts, &local_err);
>          if (ret < 0) {
>              error_reportf_err(local_err,

I'm not sure whether this hunk is necessary...

[...]

> diff --git a/qemu-img.texi b/qemu-img.texi
> index 01acfb8..bda3cc3 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.

-F is correct, that's the one for qemu-img compare, after all (which
takes two source filenames).

> +
> +@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{-F} or @var{-O} parameters. It is currently required

-F is unrelated to a target ("output") file.

Max

> +to also use the @var{-n} parameter to skip image creation. This restriction
> +will be relaxed in a future release.
>  
>  @item fmt
>  is the disk image format. It is guessed automatically in most cases. See below
> 



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

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

[Max Reitz]

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

On 03.02.2017 13:02, Daniel P. Berrange wrote:
> 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 | 51 +++++++++++++++++++++++++++++++++++++++++++++++----
>  1 file changed, 47 insertions(+), 4 deletions(-)
> 
> diff --git a/qemu-img.c b/qemu-img.c
> index dc4c6eb..98522dd 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -319,6 +319,49 @@ 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")) {
> +        if (!*options) {
> +            *options = qdict_new();
> +        }
> +        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;
> +
> +    if (fmt) {
> +        options = qdict_new();
> +        qdict_put(options, "driver", qstring_from_str(fmt));
> +    }
> +
> +    qemu_opt_foreach(create_opts, img_add_key_secrets, &options, NULL);

It would probably be easier to just unconditionally create an options
QDict. It doesn't hurt if it's empty.

Anyway:

Reviewed-by: Max Reitz <mreitz@redhat.com>

> +
> +    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,
> @@ -2115,8 +2158,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;
> @@ -4206,8 +4249,8 @@ static int img_dd(int argc, char **argv)
>           * That has to wait for bdrv_create to be improved
>           * to allow filenames in option syntax
>           */
> -        blk2 = img_open_file(out.filename, out_fmt,
> -                             BDRV_O_RDWR, false, false);
> +        blk2 = img_open_new_file(out.filename, opts, out_fmt,
> +                                 BDRV_O_RDWR, false, false);
>      }
>  
>      if (!blk2) {
> 



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

Re: [Qemu-devel] [PATCH v2 1/6] qemu-img: add support for --object with 'dd' command

[Daniel P. Berrange]

On Fri, Feb 03, 2017 at 10:01:53PM +0100, Max Reitz wrote:
> On 03.02.2017 13:02, Daniel P. Berrange wrote:
> > 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 | 16 ++++++++++++++++
> >  1 file changed, 16 insertions(+)
> > 
> > diff --git a/qemu-img.c b/qemu-img.c
> > index 74e3362..391a141 100644
> > --- a/qemu-img.c
> > +++ b/qemu-img.c
> > @@ -3949,6 +3949,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 }
> >      };
> > @@ -3971,6 +3972,14 @@ 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) {
> > +                return 1;
> > +            }
> > +        }   break;
> >          case OPTION_IMAGE_OPTS:
> >              image_opts = true;
> >              break;
> > @@ -4015,6 +4024,13 @@ 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)) {
> > +        return 1;
> 
> Why not ret = -1; goto out; like the other code around this block?
> 
> (Same for the case block above.)

This was just copy+paste from other previous command handlers. I'll
change it to match img_dd() code style.

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 v2 3/6] qemu-img: add support for conv=nocreat, notrunc args to dd command

[Daniel P. Berrange]

On Fri, Feb 03, 2017 at 10:44:46PM +0100, Max Reitz wrote:
> On 03.02.2017 13:02, Daniel P. Berrange wrote:
> > The -n arg to the convert command allows use of a pre-existing image,
> > rather than creating a new image. This adds equivalent functionality
> > to the dd command using the 'conv' arg. If 'conv=nocreat' is used,
> > then it will assume the image already exists. The existing image
> > will be truncated to match the required output size. 'conv=notrunc'
> > cna be used to preserve the existing image size.
> > 
> > Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
> > ---
> >  qemu-img-cmds.hx |   4 +-
> >  qemu-img.c       | 137 +++++++++++++++++++++++++++++++++++++++++--------------
> >  qemu-img.texi    |  10 +++-
> >  3 files changed, 115 insertions(+), 36 deletions(-)
> 
> Quite frankly I don't like this patch very much. It's not bad in itself,
> but I don't like the idea of giving qemu-img dd new features until it's
> an interface for qemu-img convert. Everything that we add now encourages
> new users to use it and will make the conversion a bit more difficult.
> 
> As long as qemu-img convert gets a --target-image-opts, I don't think we
> need all of this functionality in qemu-img dd.
> 
> Anyway, I won't block/NACK this patch, so resuming review.

I'm going to drop this patch and focus on just fixing qemu-img convert
for now. We can re-visit 'dd' at a later date, once it is clear what
will happen to it wrt refactoring to run above 'convert'.

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 v2 4/6] qemu-img: add support for -o arg to dd command

[Daniel P. Berrange]

On Fri, Feb 03, 2017 at 11:07:13PM +0100, Max Reitz wrote:
> On 03.02.2017 13:02, Daniel P. Berrange wrote:
> > The -o arg to the convert command allows specification of format/protocol
> > options for the newly created image. This adds a -o arg to the dd command
> > to get feature parity.
> > 
> > Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
> > ---
> >  qemu-img-cmds.hx |  2 +-
> >  qemu-img.c       | 32 +++++++++++++++++++++++++++++++-
> >  qemu-img.texi    |  6 ++++--
> >  3 files changed, 36 insertions(+), 4 deletions(-)
> 
> I don't like this patch for the same reasons as for patch 3, but I like
> it a bit better. The code introduced here is exactly the same as for
> img_convert(), so merging the two would (or "is going to", I hope) be
> trivial.
> 
> So a pretty weak
> 
> Reviewed-by: Max Reitz <mreitz@redhat.com>

Again, I'll drop this patch for now.

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 v2 5/6] qemu-img: introduce --target-image-opts for 'convert' command

[Daniel P. Berrange]

On Fri, Feb 03, 2017 at 11:32:13PM +0100, Max Reitz wrote:
> On 03.02.2017 13:02, 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 |   6 +--
> >  qemu-img.c       | 131 ++++++++++++++++++++++++++++++++++++-------------------
> >  qemu-img.texi    |  12 ++++-
> >  3 files changed, 98 insertions(+), 51 deletions(-)
> 
> Apart from what the commit message says, it also introduces that switch
> for dd, which I again don't like too much (quelle surprise), if only
> because it requires conv=nocreat and thus patch 5 to be useful.

Once again, I'll drop the 'dd' parts of this patch.

> > @@ -1918,7 +1933,7 @@ static int img_convert(int argc, char **argv)
> >      bs_n = argc - optind - 1;
> >      out_filename = bs_n >= 1 ? argv[argc - 1] : NULL;
> >  
> > -    if (options && has_help_option(options)) {
> > +    if (out_fmt && options && has_help_option(options)) {
> 
> "!out_fmt && options && has_help_option(options)" should probably be an
> error.

Ok.

> > @@ -1987,22 +2002,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);
> 
> Compression may be used with -n. This involves the check whether
> drv->bdrv_co_pwritev_compressed is NULL or not -- which is bad if drv is
> still NULL:
> 
> $ ./qemu-img create -f qcow2 foo.qcow2 64M
> Formatting 'foo.qcow2', fmt=qcow2 size=67108864 encryption=off
> cluster_size=65536 lazy_refcounts=off refcount_bits=16
> $ ./qemu-img convert -c -O qcow2 -n null-co:// foo.qcow2
> [1]    17179 segmentation fault (core dumped)  ./qemu-img convert -c -O
> qcow2 -n null-co:// foo.qcow
> 
> Therefore, you should probably only do the check whether compression is
> supported if drv is non-NULL; and if it is NULL, do the check again
> after the target image has been opened and its driver is known.

Oh, yes, that's a fun combination. I'll do that.


> > diff --git a/qemu-img.texi b/qemu-img.texi
> > index 01acfb8..bda3cc3 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.
> 
> -F is correct, that's the one for qemu-img compare, after all (which
> takes two source filenames).
> 
> > +
> > +@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{-F} or @var{-O} parameters. It is currently required
> 
> -F is unrelated to a target ("output") file.

Opps, yes.

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 v2 6/6] qemu-img: copy *key-secret opts when opening newly created files

[Daniel P. Berrange]

On Fri, Feb 03, 2017 at 11:39:35PM +0100, Max Reitz wrote:
> On 03.02.2017 13:02, Daniel P. Berrange wrote:
> > 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 | 51 +++++++++++++++++++++++++++++++++++++++++++++++----
> >  1 file changed, 47 insertions(+), 4 deletions(-)
> > 
> > diff --git a/qemu-img.c b/qemu-img.c
> > index dc4c6eb..98522dd 100644
> > --- a/qemu-img.c
> > +++ b/qemu-img.c
> > @@ -319,6 +319,49 @@ 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")) {
> > +        if (!*options) {
> > +            *options = qdict_new();
> > +        }
> > +        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;
> > +
> > +    if (fmt) {
> > +        options = qdict_new();
> > +        qdict_put(options, "driver", qstring_from_str(fmt));
> > +    }
> > +
> > +    qemu_opt_foreach(create_opts, img_add_key_secrets, &options, NULL);
> 
> It would probably be easier to just unconditionally create an options
> QDict. It doesn't hurt if it's empty.

Ok, I will make that change.

> Anyway:
> 
> Reviewed-by: Max Reitz <mreitz@redhat.com>
> 
> > +
> > +    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;

it seems I'm also leaking "options" so will adda  QDECREF too

> > +}
> > +
> > +



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 v2 6/6] qemu-img: copy *key-secret opts when opening newly created files

[Daniel P. Berrange]

On Mon, Feb 20, 2017 at 12:46:52PM +0000, Daniel P. Berrange wrote:
> On Fri, Feb 03, 2017 at 11:39:35PM +0100, Max Reitz wrote:
> > On 03.02.2017 13:02, Daniel P. Berrange wrote:
> > > 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 | 51 +++++++++++++++++++++++++++++++++++++++++++++++----
> > >  1 file changed, 47 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/qemu-img.c b/qemu-img.c
> > > index dc4c6eb..98522dd 100644
> > > --- a/qemu-img.c
> > > +++ b/qemu-img.c
> > > @@ -319,6 +319,49 @@ 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")) {
> > > +        if (!*options) {
> > > +            *options = qdict_new();
> > > +        }
> > > +        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;
> > > +
> > > +    if (fmt) {
> > > +        options = qdict_new();
> > > +        qdict_put(options, "driver", qstring_from_str(fmt));
> > > +    }
> > > +
> > > +    qemu_opt_foreach(create_opts, img_add_key_secrets, &options, NULL);
> > 
> > It would probably be easier to just unconditionally create an options
> > QDict. It doesn't hurt if it's empty.
> 
> Ok, I will make that change.
> 
> > Anyway:
> > 
> > Reviewed-by: Max Reitz <mreitz@redhat.com>
> > 
> > > +
> > > +    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;
> 
> it seems I'm also leaking "options" so will adda  QDECREF too

Actually I'm wrong here - I forgot this is an unusual case where
blk_new_open grabs the existing reference rather than incref'ing
it.

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/ :|