qemu-img: improve convert & dd commands

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 <[email protected]>

Signed-off-by Daniel P. Berrange <[email protected]>

---
 qemu-img.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

qemu-img.c +9 -0

index 74e3362..391a141 100644
@@ -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;

qemu-img.c +2 -0

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

Quoted 38 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.

Reviewed-by Eric Blake <[email protected]>

Signed-off-by Daniel P. Berrange <[email protected]>

---
 qemu-img.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

qemu-img.c +9 -0

index 74e3362..391a141 100644
@@ -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;

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

Quoted 8 lines

qemu-img.c +5 -0

@@ -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.)

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 <[email protected]>

---
 qemu-img.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

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 <[email protected]>

---
 qemu-img-cmds.hx |   4 +-
 qemu-img.c       | 137 +++++++++++++++++++++++++++++++++++++++++--------------
 qemu-img.texi    |  10 +++-
 3 files changed, 115 insertions(+), 36 deletions(-)

qemu-img-cmds.hx +0 -0

 ETEXI
 
 DEF("info", img_info,

qemu-img.c +0 -0

index 629f9e9..c9ab9e5 100644

qemu-img.c +18 -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;

qemu-img.c +0 -0

         if (c == EOF) {
             break;
         }

qemu-img.texi +1 -1

index 174aae3..9f10562 100644
@@ -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.
 

qemu-img.texi +0 -0

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

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

Quoted 6 lines

Signed-off-by Daniel P. Berrange <[email protected]>

---
 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.

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 <[email protected]>

---
 qemu-img-cmds.hx |  2 +-
 qemu-img.c       | 32 +++++++++++++++++++++++++++++++-
 qemu-img.texi    |  6 ++++--
 3 files changed, 36 insertions(+), 4 deletions(-)

Quoted 10 lines · 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 <[email protected]>

---
 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 <[email protected]>

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 <[email protected]>

---
 qemu-img-cmds.hx |   6 +--
 qemu-img.c       | 131 ++++++++++++++++++++++++++++++++++++-------------------
 qemu-img.texi    |  12 ++++-
 3 files changed, 98 insertions(+), 51 deletions(-)

qemu-img.c +0 -0

index 39fcf09..dc4c6eb 100644

qemu-img.c +9 -13

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

qemu-img.c +5 -0

+        goto out;
+    }
+
+    if (!out_fmt && !tgt_image_opts) {
+        out_fmt = "raw";

qemu-img.c +0 -0

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

qemu-img.texi +2 -0

+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

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 6 lines

Signed-off-by Daniel P. Berrange <[email protected]>

---
 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.

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

Quoted 6 lines

qemu-img.c +1 -1

@@ -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.

Quoted 27 lines

qemu-img.c +9 -13

@@ -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);

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

Quoted 10 lines

qemu-img.c +5 -1

+        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.

qemu-img.texi keys. · line 50

Quoted 13 lines

qemu-img.texi +2 -2

index 01acfb8..bda3cc3 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.

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

qemu-img.texi keys. · line 56

Quoted 6 lines

qemu-img.texi +6 -0

+
+@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.

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 <[email protected]>

---
 qemu-img.c | 51 +++++++++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 47 insertions(+), 4 deletions(-)

qemu-img.c +21 -0

index dc4c6eb..98522dd 100644
@@ -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)
+{

qemu-img.c +16 -4

+
+    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) {

Quoted 45 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.

Signed-off-by Daniel P. Berrange <[email protected]>

---
 qemu-img.c | 51 +++++++++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 47 insertions(+), 4 deletions(-)

qemu-img.c +26 -0

index dc4c6eb..98522dd 100644
@@ -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) {

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

Quoted 5 lines

qemu-img.c +5 -0

+        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 <[email protected]>

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

Quoted 9 lines

qemu-img.c +9 -0

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

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

Quoted 3 lines

qemu-img.c +3 -0

+}
+
+

Quoted 46 lines · Max Reitz

Quoted 45 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.

Signed-off-by Daniel P. Berrange <[email protected]>

---
 qemu-img.c | 51 +++++++++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 47 insertions(+), 4 deletions(-)

qemu-img.c +26 -0

index dc4c6eb..98522dd 100644
@@ -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) {

Quoted 8 lines

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

Quoted 5 lines

qemu-img.c +5 -0

+        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.

Quoted 4 lines

Anyway:

Reviewed-by Max Reitz <[email protected]>

Quoted 9 lines

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

Quoted 9 lines

qemu-img.c +9 -0

+
+    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