qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH 12/14] qapi: introduce change-blockdev


From: Anthony Liguori
Subject: Re: [Qemu-devel] [PATCH 12/14] qapi: introduce change-blockdev
Date: Thu, 25 Aug 2011 07:56:31 -0500
User-agent: Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9.2.17) Gecko/20110516 Lightning/1.0b2 Thunderbird/3.1.10

On 08/25/2011 07:46 AM, Kevin Wolf wrote:
Am 24.08.2011 20:43, schrieb Anthony Liguori:
A new QMP only command to change the blockdev associated with a block device.
The semantics of change right now are just plain scary.  This command introduces
sane semantics.  For more details, see the documentation in the schema file.

Changes to semantics should be mentioned in the commit message, and in
more detail than just that they are sane (the documentation in the
schema file doesn't describe differences to the old command).

Right, but the schema describes the old command.

There are a couple major differences with the new command vs old:

1) If you had a block device named 'vnc', the old command is broken.

2) For an encrypted device, the old change command returns an error but has semantically completed the operation. The usage is:

(qemu) change ide0 foo.img
Error: device ide0 is encrypted
(qemu) block_passwd ide0 secret

Because even though an error is returned, the change operation has actually succeeded. The new command has transactional semantics. If an error is returned, nothing has happened. To set passwords, an additional option is supported to specify the password. So you would do:

(qemu) change-blockdev ide0 foo.img
Error: device ide0 is encrypted
(qemu) change-blockdev ide0 foo.img secret
(qemu)

In addition, change-blockdev will not allow you to perform unsafe probing. You can only change to a raw device if you specify a raw format.


Just assuming that you are right about sanity, I wonder how sanity and
compatibility could possibly be achieved at once... I suspect the
semantics is clearer now in that it doesn't overload one command with
block and VNC subcommands, but I doubt that the semantics of the block
version is much better than before.

I don't see how we can change the behavior of the change command.

So please call the command drive_change, it's similar to drive_add and
drive_del, not to the future blockdev_add/remove.

Ack.

Signed-off-by: Anthony Liguori<address@hidden>
---
  blockdev.c       |  108 +++++++++++++++++++++++++++++++++++++++++++++++++++---
  qapi-schema.json |   30 +++++++++++++++
  qmp-commands.hx  |    8 ++++
  3 files changed, 140 insertions(+), 6 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index 37b2f29..c00c69d 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -697,12 +697,101 @@ void qmp_block_passwd(const char *device, const char 
*password, Error **err)
      qmp_set_blockdev_password(device, password, err);
  }

+static void qmp_bdrv_open_encrypted(BlockDriverState *bs, const char *filename,
+                                    int bdrv_flags, BlockDriver *drv,
+                                    const char *password, Error **errp)
+{
+    if (bdrv_open(bs, filename, bdrv_flags, drv)<  0) {
+        error_set(errp, QERR_OPEN_FILE_FAILED, filename);
+        return;
+    }
+
+    if (bdrv_key_required(bs)) {
+        if (password) {
+            if (bdrv_set_key(bs, password)<  0) {
+                error_set(errp, QERR_INVALID_PASSWORD);
+            }
+        } else {
+            error_set(errp, QERR_DEVICE_ENCRYPTED, bdrv_get_device_name(bs),
+                      bdrv_get_encrypted_filename(bs));
+        }
+    } else if (password) {
+        error_set(errp, QERR_DEVICE_NOT_ENCRYPTED, bdrv_get_device_name(bs));
+    }
+}
+
+void qmp_change_blockdev(const char *device, const char *filename,
+                         bool has_format, const char *format,
+                         bool has_password, const char *password,
+                         Error **errp)
+{
+    BlockDriverState *bs, *bs1;
+    BlockDriver *drv = NULL;
+    int bdrv_flags;
+    Error *err = NULL;
+    bool probed_raw = false;
+
+    bs = bdrv_find(device);
+    if (!bs) {
+        error_set(errp, QERR_DEVICE_NOT_FOUND, device);
+        return;
+    }
+
+    if (has_format) {
+        drv = bdrv_find_whitelisted_format(format);
+        if (!drv) {
+            error_set(errp, QERR_INVALID_BLOCK_FORMAT, format);
+            return;
+        }
+    }
+
+    bdrv_flags = bdrv_is_read_only(bs) ? 0 : BDRV_O_RDWR;
+    bdrv_flags |= bdrv_is_snapshot(bs) ? BDRV_O_SNAPSHOT : 0;
+
+    if (!has_password) {
+        password = NULL;
+    }
+
+    /* Try to open once with a temporary block device to make sure that
+     * the disk isn't encrypted and we lack a key.  This also helps keep
+     * this operation as a transaction.  That is, if we fail, we try very
+     * hard to make sure that the state is the same as before the operation
+     * was started.
+     */
+    bs1 = bdrv_new("");
+    qmp_bdrv_open_encrypted(bs1, filename, bdrv_flags, drv, password,&err);
+    if (!has_format&&  bs1->drv->unsafe_probe) {
+        probed_raw = true;
+    }
+    bdrv_delete(bs1);
+
+    if (err) {
+        error_propagate(errp, err);
+        return;
+    }
+
+    if (probed_raw) {
+        /* We will not auto probe raw files. */
+        error_set(errp, QERR_MISSING_PARAMETER, "format");
+        return;
+    }
+
+    eject_device(bs, 0,&err);
+    if (err) {
+        error_propagate(errp, err);
+        return;
+    }
+
+    qmp_bdrv_open_encrypted(bs, filename, bdrv_flags, drv, password, errp);
+}

Can't this share code with the old command? I don't like this duplication.

qmp_bdrv_open_encrypted() is the code that can be shared. The next patch I think changes the old command to use this function.

Regards,

Anthony Liguori

Kevin





reply via email to

[Prev in Thread] Current Thread [Next in Thread]