Improve convert and dd commands

The qemu-img dd/convert commands will create an 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.

Reviewed-by Max Reitz <[email protected]>

Reviewed-by Fam Zheng <[email protected]>

Reviewed-by Eric Blake <[email protected]>

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

---
 qemu-img.c | 42 +++++++++++++++++++++++++++++++++++++-----
 1 file changed, 37 insertions(+), 5 deletions(-)

qemu-img.c +1 -0

index e0e3d31..dcddded 100644
@@ -314,15 +314,18 @@ 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,

qemu-img.c +33 -4

         qdict_put_str(options, "driver", fmt);
     }
 

@@ -344,6 +347,35 @@ 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,
+                                       bool force_share)
+{
+    QDict *options = NULL;
+
+    options = qdict_new();
+    qemu_opt_foreach(create_opts, img_add_key_secrets, options, &error_abort);
+
+    return img_open_file(filename, options, fmt, flags, writethrough, quiet,
+                         force_share);
+}
+
+
 static BlockBackend *img_open(bool image_opts,
                               const char *filename,
                               const char *fmt, int flags, bool writethrough,

@@ -364,7 +396,7 @@ static BlockBackend *img_open(bool image_opts,
         blk = img_open_opts(filename, opts, flags, writethrough, quiet,
                             force_share);
     } else {
-        blk = img_open_file(filename, fmt, flags, writethrough, quiet,
+        blk = img_open_file(filename, NULL, fmt, flags, writethrough, quiet,
                             force_share);
     }
     return blk;

@@ -2286,8 +2318,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
          */
-        s.target = img_open_file(out_filename, out_fmt, flags,
-                                 writethrough, quiet, false);
+        s.target = img_open_new_file(out_filename, opts, out_fmt,
+                                     flags, writethrough, quiet, false);
     }
     if (!s.target) {
         ret = -1;

@@ -4351,7 +4383,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, false);
 
     if (!blk2) {

Quoted 25 lines · Daniel P. Berrange

The qemu-img dd/convert commands will create an 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.

Reviewed-by Max Reitz <[email protected]>

Reviewed-by Fam Zheng <[email protected]>

Reviewed-by Eric Blake <[email protected]>

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

---
 qemu-img.c | 42 +++++++++++++++++++++++++++++++++++++-----
 1 file changed, 37 insertions(+), 5 deletions(-)

qemu-img.c +1 -0

index e0e3d31..dcddded 100644
@@ -314,15 +314,18 @@ 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,

qemu-img.c static BlockBackend *img_open_opts(const char *optstr, · line 328

Quoted 10 lines

qemu-img.c +3 -1

                                    bool force_share)
 {
     BlockBackend *blk;
     Error *local_err = NULL;
-    QDict *options = qdict_new();
 
     if (fmt) {
+        if (!options) {
+            options = qdict_new();
+        }

This is the only place where my attempted rebase and your version differ. I think this has to be done unconditionally, because otherwise:

$ ./qemu-img info -U null-co://
[1]    16327 segmentation fault (core dumped)  ./qemu-img info -U null-co://

Also, I'm not sure the R-bs apply for this patch any longer.

(They do for patch 1 because it's just a contextual difference. For patch 2, it's a borderline case (I would drop it, but I can understand keeping it). For patch 3 it's more than just borderline - I would definitely drop the R-b, but the differences are still rather mechanical, so it is acceptable to keep it.
But I think there are too many changes here in this patch to keep the R-bs. In fact, I'm pretty sure none of Eric, Fam and me have given an R-b to this segfault...)

Quoted 25 lines · Daniel P. Berrange

The qemu-img dd/convert commands will create an 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.

Reviewed-by Max Reitz <[email protected]>

Reviewed-by Fam Zheng <[email protected]>

Reviewed-by Eric Blake <[email protected]>

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

---
 qemu-img.c | 42 +++++++++++++++++++++++++++++++++++++-----
 1 file changed, 37 insertions(+), 5 deletions(-)

qemu-img.c +1 -0

index e0e3d31..dcddded 100644
@@ -314,15 +314,18 @@ 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,

qemu-img.c static BlockBackend *img_open_opts(const char *optstr, · line 328

Quoted 10 lines

qemu-img.c +3 -1

                                    bool force_share)
 {
     BlockBackend *blk;
     Error *local_err = NULL;
-    QDict *options = qdict_new();
 
     if (fmt) {
+        if (!options) {
+            options = qdict_new();
+        }

This is the only place where my attempted rebase and your version differ. I think this has to be done unconditionally, because otherwise:

$ ./qemu-img info -U null-co://
[1]    16327 segmentation fault (core dumped)  ./qemu-img info -U null-co://

Also, I'm not sure the R-bs apply for this patch any longer.

(They do for patch 1 because it's just a contextual difference. For patch 2, it's a borderline case (I would drop it, but I can understand keeping it). For patch 3 it's more than just borderline - I would definitely drop the R-b, but the differences are still rather mechanical, so it is acceptable to keep it.
But I think there are too many changes here in this patch to keep the R-bs. In fact, I'm pretty sure none of Eric, Fam and me have given an R-b to this segfault...)