Skip to contentSkip to conversation

Improve convert and dd commands

PATCH Superseded Daniel P. Berrange 3 participants 9 messages Apr 12, 2017 – Apr 13, 2017

v4 of 10 Superseded Latest is v10 →

lore
Share & tools
lore
#0 lore raw

v4 cover Improve convert and dd commands

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
v3: https://lists.gnu.org/archive/html/qemu-devel/2017-02/msg04391.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 v4:

- Refactor img_open_new_file in terms of img_open_file (Kevin)

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       | 146 +++++++++++++++++++++++++++++++++++++++++++------------
 qemu-img.texi    |  12 ++++-
 3 files changed, 128 insertions(+), 34 deletions(-)
Signature · 1 line
2.9.3
#1 lore raw

PATCH v4 1/4 qemu-img: add support for --object with 'dd' command

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.

---
 qemu-img.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)
qemu-img.c +18 -0
index b220cf7..2249c21 100644
@@ -4159,6 +4159,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 }
     };
@@ -4183,6 +4184,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;
@@ -4227,6 +4237,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) {
Signature · 1 line
2.9.3
Reviewed-by on v4 1/4
#2 lore raw

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

Quoted 9 lines · 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.

---
 qemu-img.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)
#3 lore raw

PATCH v4 2/4 qemu-img: fix --image-opts usage with dd command

In reply to 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.

---
 qemu-img.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)
qemu-img.c +7 -2
index 2249c21..83aff5e 100644
@@ -4312,8 +4312,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;
Signature · 1 line
2.9.3
#4 lore raw

PATCH v4 3/4 qemu-img: introduce --target-image-opts for 'convert' command

In reply to 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.

---
 qemu-img-cmds.hx |  4 +--
 qemu-img.c       | 84 +++++++++++++++++++++++++++++++++++++++-----------------
 qemu-img.texi    | 12 ++++++--
 3 files changed, 71 insertions(+), 29 deletions(-)
qemu-img-cmds.hx +2 -2
index 8ac7822..93b50ef 100644
@@ -40,9 +40,9 @@ STEXI
 ETEXI
 
 DEF("convert", img_convert,
-    "convert [--object objectdef] [--image-opts] [-c] [-p] [-q] [-n] [-f fmt] [-t cache] [-T src_cache] [-O output_fmt] [-o options] [-s snapshot_id_or_name] [-l snapshot_param] [-S sparse_size] [-m num_coroutines] [-W] filename [filename2 [...]] output_filename")
+    "convert [--object objectdef] [--image-opts] [--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] [-m num_coroutines] [-W] filename [filename2 [...]] output_filename")
 STEXI
-@item convert [--object @var{objectdef}] [--image-opts] [-c] [-p] [-q] [-n] [-f @var{fmt}] [-t @var{cache}] [-T @var{src_cache}] [-O @var{output_fmt}] [-o @var{options}] [-s @var{snapshot_id_or_name}] [-l @var{snapshot_param}] [-S @var{sparse_size}] [-m @var{num_coroutines}] [-W] @var{filename} [@var{filename2} [...]] @var{output_filename}
+@item convert [--object @var{objectdef}] [--image-opts] [--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}] [-m @var{num_coroutines}] [-W] @var{filename} [@var{filename2} [...]] @var{output_filename}
 ETEXI
 
 DEF("dd", img_dd,
qemu-img.c +59 -25

157 lines not on this page · raw message

qemu-img.texi +10 -2
index c81db3e..63f597d 100644
@@ -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
Signature · 1 line
2.9.3
#5 lore raw

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

Quoted 8 lines · 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.

Quoted 7 lines
---
 qemu-img-cmds.hx |  4 +--
 qemu-img.c       | 84 +++++++++++++++++++++++++++++++++++++++-----------------
 qemu-img.texi    | 12 ++++++--
 3 files changed, 71 insertions(+), 29 deletions(-)

Looks good apart from some minor issues.

[...]

Quoted 4 lines
qemu-img.c +0 -0
index 83aff5e..31c4923 100644

[...]

qemu-img.c static int img_convert(int argc, char **argv) · line 2105

Quoted 7 lines
qemu-img.c +3 -0
@@ -2090,6 +2100,11 @@ static int img_convert(int argc, char **argv)
         goto fail_getopt;
     }
 
+    if (tgt_image_opts && !skip_create) {
+        error_report("--target-image-opts requires use of -n flag");
+        goto fail_getopt;

ret should be set to -1.

Quoted 7 lines
qemu-img.c +2 -0
+    }
+
     /* Initialize before goto out */
     if (quiet) {
         progress = 0;
@@ -2100,8 +2115,13 @@ static int img_convert(int argc, char **argv)
     out_filename = bs_n >= 1 ? argv[argc - 1] : NULL;

qemu-img.c static int img_convert(int argc, char **argv) · line 2123

Quoted 10 lines
qemu-img.c +6 -2
 
     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;

Same here.

Quoted 4 lines
qemu-img.c +1 -0
+        }
     }
 
     if (bs_n < 1) {

[...]

Quoted 7 lines
qemu-img.texi +0 -0
index c81db3e..63f597d 100644
@@ -45,9 +45,17 @@ keys.
 
 @item --image-opts

qemu-img.texi keys. · line 54

Quoted 10 lines
qemu-img.texi +6 -2
 
-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

s/a$/as/

qemu-img.texi keys. · line 58

Quoted 4 lines
qemu-img.texi +4 -0
+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.

By the way, since you're referring to -n, you could just call it "@var{output_filename}" instead of "target @var{filename}", because that's how convert's target filename is described in this man page.

Max

Quoted 4 lines
qemu-img.texi +0 -0
 @item fmt
 is the disk image format. It is guessed automatically in most cases. See below
 
#6 lore raw

PATCH v4 4/4 qemu-img: copy *key-secret opts when opening newly created files

In reply to 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.

---
 qemu-img.c | 41 +++++++++++++++++++++++++++++++++++------
 1 file changed, 35 insertions(+), 6 deletions(-)
qemu-img.c +35 -6
index 31c4923..3d9e7b3 100644
@@ -305,15 +305,17 @@ static BlockBackend *img_open_opts(const char *optstr,
 }
 
 static BlockBackend *img_open_file(const char *filename,
+                                   QDict *options,
                                    const char *fmt, int flags,
                                    bool writethrough, bool quiet)
 {
     BlockBackend *blk;
     Error *local_err = NULL;
-    QDict *options = NULL;
 
     if (fmt) {
-        options = qdict_new();
+        if (!options) {
+            options = qdict_new();
+        }
         qdict_put(options, "driver", qstring_from_str(fmt));
     }
 
@@ -332,6 +334,33 @@ 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)
+{
+    QDict *options = NULL;
+
+    options = qdict_new();
+    qemu_opt_foreach(create_opts, img_add_key_secrets, options, NULL);
+
+    return img_open_file(filename, options, fmt, flags, writethrough, quiet);
+}
+
+
 static BlockBackend *img_open(bool image_opts,
                               const char *filename,
                               const char *fmt, int flags, bool writethrough,
@@ -351,7 +380,7 @@ static BlockBackend *img_open(bool image_opts,
         }
         blk = img_open_opts(filename, opts, flags, writethrough, quiet);
     } else {
-        blk = img_open_file(filename, fmt, flags, writethrough, quiet);
+        blk = img_open_file(filename, NULL, fmt, flags, writethrough, quiet);
     }
     return blk;
 }
@@ -2301,8 +2330,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;
@@ -4351,7 +4380,7 @@ static int img_dd(int argc, char **argv)
      * with the bdrv_create() call above which does not
      * support image-opts style.
      */
-    blk2 = img_open_file(out.filename, out_fmt, BDRV_O_RDWR,
+    blk2 = img_open_file(out.filename, NULL, out_fmt, BDRV_O_RDWR,
                          false, false);
 
     if (!blk2) {
Signature · 1 line
2.9.3
Reviewed-by on v4 4/4
#7 lore raw

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

Quoted 10 lines · 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.

---
 qemu-img.c | 41 +++++++++++++++++++++++++++++++++++------
 1 file changed, 35 insertions(+), 6 deletions(-)

qemu-img.c static BlockBackend *img_open_file(const char *filename, · line 344

Quoted 12 lines
qemu-img.c +8 -0
@@ -332,6 +334,33 @@ 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));

If my patch to add qdict_put_str() lands (probably through Markus' qapi-next tree) before yours, you can simplify this line. If yours lands first, we just rerun my Coccinelle script to simplify it as part of my patch.

Signature · 3 lines
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org
#8 lore raw

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

In reply to Daniel P. Berrange
Quoted 10 lines · 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.

---
 qemu-img.c | 41 +++++++++++++++++++++++++++++++++++------
 1 file changed, 35 insertions(+), 6 deletions(-)

Thanks!

Quoted 4 lines
qemu-img.c +0 -0
index 31c4923..3d9e7b3 100644

[...]

Quoted 16 lines
qemu-img.c +12 -0
@@ -332,6 +334,33 @@ 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;
+}

qemu-img.c static BlockBackend *img_open_file(const char *filename, · line 358

Quoted 10 lines
qemu-img.c +10 -0
+
+static BlockBackend *img_open_new_file(const char *filename,
+                                       QemuOpts *create_opts,
+                                       const char *fmt, int flags,
+                                       bool writethrough, bool quiet)
+{
+    QDict *options = NULL;
+
+    options = qdict_new();
+    qemu_opt_foreach(create_opts, img_add_key_secrets, options, NULL);

I'd prefer &error_abort instead of NULL there. Not a hard requirement, but I don't see a reason not to use it.

Max

Quoted 9 lines
qemu-img.c +5 -0
+
+    return img_open_file(filename, options, fmt, flags, writethrough, quiet);
+}
+
+
 static BlockBackend *img_open(bool image_opts,
                               const char *filename,
                               const char *fmt, int flags, bool writethrough,
@@ -351,7 +380,7 @@ static BlockBackend *img_open(bool image_opts,

[...]