[Qemu-devel] [PATCH v4 0/3] block: fix spurious DEVICE_TRAY_MOVED events on shutdown

[Qemu-devel] [PATCH v4 0/3] block: fix spurious DEVICE_TRAY_MOVED events on shutdown

[Pavel Hrdina]

This fixes a regression introduced by commit 9ca111544.

The first commit is done by Luiz and I'm just using it as it is.

The second commit moves the bdrv_dev_change_media_cb() into eject_device(),
called by QMP and HMP eject command, and into qmp_bdrv_open_encrypted(),
called by QMP and HMP change command. These are the only place where I think
that should call the bdrv_dev_change_media_cb() function.

As we discussed the third patch fixes the issue that user can provide
a password for device not requiring any password. The device is opened with
an error as warning. We should fail and don't open the device at all.

There is no reason to call this function while we are removing the device
from the guest, for example while closing and deleting all devices on shutdown.

* changes from v1:
    - used the first patch from Luiz's series
 
* changes from v2:
    - added comment that the error QERR_DEVICE_NOT_ENCRYPTED is used as warning

* changes from v3:
    - added more info into second commit
    - added new patch to fix the issue with providing password for device not
      requiring password

Luiz Capitulino (1):
  block: make bdrv_dev_change_media_cb() public

Pavel Hrdina (2):
  block: move the bdrv_dev_change_media_cb()
  blockdev: do not open block device if password is not required but
    user provides one

 block.c               | 11 +----------
 blockdev.c            | 12 ++++++++++--
 include/block/block.h |  1 +
 3 files changed, 12 insertions(+), 12 deletions(-)

-- 
1.8.1.4

[Qemu-devel] [PATCH v4 1/3] block: make bdrv_dev_change_media_cb() public

[Pavel Hrdina]

From: Luiz Capitulino <lcapitulino@redhat.com>

Next commit wants to use it.

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
---
 block.c               | 3 +--
 include/block/block.h | 1 +
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/block.c b/block.c
index b88ad2f..89574de 100644
--- a/block.c
+++ b/block.c
@@ -56,7 +56,6 @@ typedef enum {
     BDRV_REQ_ZERO_WRITE   = 0x2,
 } BdrvRequestFlags;
 
-static void bdrv_dev_change_media_cb(BlockDriverState *bs, bool load);
 static BlockDriverAIOCB *bdrv_aio_readv_em(BlockDriverState *bs,
         int64_t sector_num, QEMUIOVector *qiov, int nb_sectors,
         BlockDriverCompletionFunc *cb, void *opaque);
@@ -1682,7 +1681,7 @@ static void bdrv_emit_qmp_eject_event(BlockDriverState *bs, bool ejected)
     qobject_decref(data);
 }
 
-static void bdrv_dev_change_media_cb(BlockDriverState *bs, bool load)
+void bdrv_dev_change_media_cb(BlockDriverState *bs, bool load)
 {
     if (bs->dev_ops && bs->dev_ops->change_media_cb) {
         bool tray_was_closed = !bdrv_dev_is_tray_open(bs);
diff --git a/include/block/block.h b/include/block/block.h
index 2307f67..2cc93ab 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -148,6 +148,7 @@ void bdrv_set_dev_ops(BlockDriverState *bs, const BlockDevOps *ops,
                       void *opaque);
 void bdrv_dev_eject_request(BlockDriverState *bs, bool force);
 bool bdrv_dev_has_removable_media(BlockDriverState *bs);
+void bdrv_dev_change_media_cb(BlockDriverState *bs, bool load);
 bool bdrv_dev_is_tray_open(BlockDriverState *bs);
 bool bdrv_dev_is_medium_locked(BlockDriverState *bs);
 int bdrv_read(BlockDriverState *bs, int64_t sector_num,
-- 
1.8.1.4

[Qemu-devel] [PATCH v4 2/3] block: move the bdrv_dev_change_media_cb()

[Pavel Hrdina]

This fixes a regression introduced by commit 9ca111544.
The QEVENT_DEVICE_TRAY_MOVED is emitted while shutting down the guest, which
is not right because it comes from bdrv_close_all() and therefore this
event doesn't happen at all.

The bdrv_dev_change_media_cb() should be called only for eject and change
commands because these are the only commands to emit this event. We should
call that function only if that command is successful.

What this function does is that it calls the change_media_cb() and also emit
the QEVENT_DEVICE_TRAY_MOVED event.

Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
---
 block.c    | 8 --------
 blockdev.c | 5 +++++
 2 files changed, 5 insertions(+), 8 deletions(-)

diff --git a/block.c b/block.c
index 89574de..aad3091 100644
--- a/block.c
+++ b/block.c
@@ -1092,10 +1092,6 @@ int bdrv_open(BlockDriverState *bs, const char *filename, QDict *options,
     }
     QDECREF(options);
 
-    if (!bdrv_key_required(bs)) {
-        bdrv_dev_change_media_cb(bs, true);
-    }
-
     /* throttling disk I/O limits */
     if (bs->io_limits_enabled) {
         bdrv_io_limits_enable(bs);
@@ -1394,8 +1390,6 @@ void bdrv_close(BlockDriverState *bs)
         }
     }
 
-    bdrv_dev_change_media_cb(bs, false);
-
     /*throttling disk I/O limits*/
     if (bs->io_limits_enabled) {
         bdrv_io_limits_disable(bs);
@@ -2846,8 +2840,6 @@ int bdrv_set_key(BlockDriverState *bs, const char *key)
         bs->valid_key = 0;
     } else if (!bs->valid_key) {
         bs->valid_key = 1;
-        /* call the change callback now, we skipped it on open */
-        bdrv_dev_change_media_cb(bs, true);
     }
     return ret;
 }
diff --git a/blockdev.c b/blockdev.c
index 5975dde..2dbd781 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1022,6 +1022,7 @@ static void eject_device(BlockDriverState *bs, int force, Error **errp)
     }
 
     bdrv_close(bs);
+    bdrv_dev_change_media_cb(bs, false);
 }
 
 void qmp_eject(const char *device, bool has_force, bool force, Error **errp)
@@ -1074,14 +1075,18 @@ static void qmp_bdrv_open_encrypted(BlockDriverState *bs, const char *filename,
         if (password) {
             if (bdrv_set_key(bs, password) < 0) {
                 error_set(errp, QERR_INVALID_PASSWORD);
+                return;
             }
         } else {
             error_set(errp, QERR_DEVICE_ENCRYPTED, bdrv_get_device_name(bs),
                       bdrv_get_encrypted_filename(bs));
+            return;
         }
     } else if (password) {
         error_set(errp, QERR_DEVICE_NOT_ENCRYPTED, bdrv_get_device_name(bs));
     }
+
+    bdrv_dev_change_media_cb(bs, true);
 }
 
 void qmp_change_blockdev(const char *device, const char *filename,
-- 
1.8.1.4

[Qemu-devel] [PATCH v4 3/3] blockdev: do not open block device if password is not required but user provides one

[Pavel Hrdina]

This fixes the issue that block device is openned successfully with an error
as warning if password is provided but not required. Now the open fails with
the error message.

Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
---
 blockdev.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index 2dbd781..7fc8dcf 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1065,6 +1065,11 @@ static void qmp_bdrv_open_encrypted(BlockDriverState *bs, const char *filename,
 {
     int ret;
 
+    if (!bdrv_key_required(bs) && password) {
+        error_set(errp, QERR_DEVICE_NOT_ENCRYPTED, bdrv_get_device_name(bs));
+        return;
+    }
+
     ret = bdrv_open(bs, filename, NULL, bdrv_flags, drv);
     if (ret < 0) {
         error_setg_file_open(errp, -ret, filename);
@@ -1082,8 +1087,6 @@ static void qmp_bdrv_open_encrypted(BlockDriverState *bs, const char *filename,
                       bdrv_get_encrypted_filename(bs));
             return;
         }
-    } else if (password) {
-        error_set(errp, QERR_DEVICE_NOT_ENCRYPTED, bdrv_get_device_name(bs));
     }
 
     bdrv_dev_change_media_cb(bs, true);
-- 
1.8.1.4

Re: [Qemu-devel] [PATCH v4 3/3] blockdev: do not open block device if password is not required but user provides one

[Stefan Hajnoczi]

On Mon, Jun 24, 2013 at 02:12:52PM +0200, Pavel Hrdina wrote:
> This fixes the issue that block device is openned successfully with an error
> as warning if password is provided but not required. Now the open fails with
> the error message.
> 
> Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
> ---
>  blockdev.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/blockdev.c b/blockdev.c
> index 2dbd781..7fc8dcf 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -1065,6 +1065,11 @@ static void qmp_bdrv_open_encrypted(BlockDriverState *bs, const char *filename,
>  {
>      int ret;
>  
> +    if (!bdrv_key_required(bs) && password) {
> +        error_set(errp, QERR_DEVICE_NOT_ENCRYPTED, bdrv_get_device_name(bs));
> +        return;
> +    }
> +
>      ret = bdrv_open(bs, filename, NULL, bdrv_flags, drv);

How can bdrv_key_required(bs) work when bs isn't opened yet?