block: fix spurious DEVICE_TRAY_MOVED events on shutdown
v4 cover block: fix spurious DEVICE_TRAY_MOVED events on shutdown
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(-)
Signature · 1 line
1.8.1.4
PATCH v4 1/3 block: make bdrv_dev_change_media_cb() public
From: Luiz Capitulino <[email protected]>
Next commit wants to use it.
--- block.c | 3 +-- include/block/block.h | 1 + 2 files changed, 2 insertions(+), 2 deletions(-)
block.c +1 -2
@@ -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);
include/block/block.h +1 -0
@@ -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,
Signature · 1 line
1.8.1.4
PATCH v4 2/3 block: move the bdrv_dev_change_media_cb()
In reply to Pavel HrdinaThis 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.
--- block.c | 8 -------- blockdev.c | 5 +++++ 2 files changed, 5 insertions(+), 8 deletions(-)
block.c +0 -8
@@ -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; }
blockdev.c +5 -0
@@ -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,
Signature · 1 line
1.8.1.4
PATCH v4 3/3 blockdev: do not open block device if password is not required but user provides one
In reply to Pavel HrdinaThis 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.
--- blockdev.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)
blockdev.c +5 -2
@@ -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);
Signature · 1 line
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
Quoted 14 lines · 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 <[email protected]>--- blockdev.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)blockdev.c +0
-0
blockdev.c static void qmp_bdrv_open_encrypted(BlockDriverState *bs, const char *filename, · line 1073
Quoted 9 lines
blockdev.c +5
-0@@ -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?