Improve convert and dd commands

The --image-opts flag can only be used to affect the parsing of the source image. The target image has to be specified in the traditional style regardless, since it needs to be passed to the bdrv_create() API which does not support the new style opts.

Reviewed-by Max Reitz <[email protected]>

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

Am 20.02.2017 um 16:19 hat Daniel P. Berrange geschrieben:

Quoted 8 lines

The --image-opts flag can only be used to affect the parsing of the source image. The target image has to be specified in the traditional style regardless, since it needs to be passed to the bdrv_create() API which does not support the new style opts.

Reviewed-by Max Reitz <[email protected]>

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

Hm. This means that...

1. --image-opts never worked for 'qemu-img dd'

2. If we ever change bdrv_create() to be more flexible, with this patch we'd be stuck with an inconsistent "filename for target, options for source" interface because we can't change the semantics any more.

Should we just remove --image-opts from qemu-img dd instead until we can provide the real thing?

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

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 |  4 +--
 qemu-img.c       | 84 +++++++++++++++++++++++++++++++++++++++-----------------
 qemu-img.texi    | 12 ++++++--
 3 files changed, 71 insertions(+), 29 deletions(-)

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 | 44 ++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 42 insertions(+), 2 deletions(-)

qemu-img.c +28 -0

index e48e676..bad19fd 100644
@@ -317,6 +317,46 @@ static BlockBackend *img_open_file(const char *filename,
 }
 
 
+static int img_add_key_secrets(void *opaque,
+                               const char *name, const char *value,
+                               Error **errp)
+{
+    QDict *options = opaque;
+
+    if (g_str_has_suffix(name, "key-secret")) {
+        qdict_put(options, name, qstring_from_str(value));
+    }
+
+    return 0;
+}
+
+static BlockBackend *img_open_new_file(const char *filename,
+                                       QemuOpts *create_opts,
+                                       const char *fmt, int flags,
+                                       bool writethrough, bool quiet)
+{
+    BlockBackend *blk;
+    Error *local_err = NULL;
+    QDict *options = NULL;
+
+    options = qdict_new();
+    if (fmt) {
+        qdict_put(options, "driver", qstring_from_str(fmt));
+    }
+
+    qemu_opt_foreach(create_opts, img_add_key_secrets, options, NULL);

Am 20.02.2017 um 16:19 hat Daniel P. Berrange geschrieben:

Quoted 47 lines

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 | 44 ++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 42 insertions(+), 2 deletions(-)

qemu-img.c +28 -0

index e48e676..bad19fd 100644
@@ -317,6 +317,46 @@ static BlockBackend *img_open_file(const char *filename,
 }
 
 
+static int img_add_key_secrets(void *opaque,
+                               const char *name, const char *value,
+                               Error **errp)
+{
+    QDict *options = opaque;
+
+    if (g_str_has_suffix(name, "key-secret")) {
+        qdict_put(options, name, qstring_from_str(value));
+    }
+
+    return 0;
+}
+
+static BlockBackend *img_open_new_file(const char *filename,
+                                       QemuOpts *create_opts,
+                                       const char *fmt, int flags,
+                                       bool writethrough, bool quiet)
+{
+    BlockBackend *blk;
+    Error *local_err = NULL;
+    QDict *options = NULL;
+
+    options = qdict_new();
+    if (fmt) {
+        qdict_put(options, "driver", qstring_from_str(fmt));
+    }
+
+    qemu_opt_foreach(create_opts, img_add_key_secrets, options, NULL);

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

Quoted 10 lines

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

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

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

Quoted 64 lines

Am 20.02.2017 um 16:19 hat Daniel P. Berrange geschrieben:

Quoted 47 lines

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 | 44 ++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 42 insertions(+), 2 deletions(-)

qemu-img.c +28 -0

index e48e676..bad19fd 100644
@@ -317,6 +317,46 @@ static BlockBackend *img_open_file(const char *filename,
 }
 
 
+static int img_add_key_secrets(void *opaque,
+                               const char *name, const char *value,
+                               Error **errp)
+{
+    QDict *options = opaque;
+
+    if (g_str_has_suffix(name, "key-secret")) {
+        qdict_put(options, name, qstring_from_str(value));
+    }
+
+    return 0;
+}
+
+static BlockBackend *img_open_new_file(const char *filename,
+                                       QemuOpts *create_opts,
+                                       const char *fmt, int flags,
+                                       bool writethrough, bool quiet)
+{
+    BlockBackend *blk;
+    Error *local_err = NULL;
+    QDict *options = NULL;
+
+    options = qdict_new();
+    if (fmt) {
+        qdict_put(options, "driver", qstring_from_str(fmt));
+    }
+
+    qemu_opt_foreach(create_opts, img_add_key_secrets, options, NULL);

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

Quoted 10 lines

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

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

Quoted 2 lines

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

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

Am 22.02.2017 um 12:33 hat Daniel P. Berrange geschrieben:

Quoted 65 lines · Kevin Wolf

Quoted 64 lines

Am 20.02.2017 um 16:19 hat Daniel P. Berrange geschrieben:

Quoted 47 lines

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 | 44 ++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 42 insertions(+), 2 deletions(-)

qemu-img.c +28 -0

index e48e676..bad19fd 100644
@@ -317,6 +317,46 @@ static BlockBackend *img_open_file(const char *filename,
 }
 
 
+static int img_add_key_secrets(void *opaque,
+                               const char *name, const char *value,
+                               Error **errp)
+{
+    QDict *options = opaque;
+
+    if (g_str_has_suffix(name, "key-secret")) {
+        qdict_put(options, name, qstring_from_str(value));
+    }
+
+    return 0;
+}
+
+static BlockBackend *img_open_new_file(const char *filename,
+                                       QemuOpts *create_opts,
+                                       const char *fmt, int flags,
+                                       bool writethrough, bool quiet)
+{
+    BlockBackend *blk;
+    Error *local_err = NULL;
+    QDict *options = NULL;
+
+    options = qdict_new();
+    if (fmt) {
+        qdict_put(options, "driver", qstring_from_str(fmt));
+    }
+
+    qemu_opt_foreach(create_opts, img_add_key_secrets, options, NULL);

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

Quoted 10 lines

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

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

Quoted 7 lines

Quoted 2 lines

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

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

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

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