block: fix spurious DEVICE_TRAY_MOVED events on shutdown
v2 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've just use 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.
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.
Luiz Capitulino (1):
block: make bdrv_dev_change_media_cb() public
Pavel Hrdina (1):
block: move the bdrv_dev_change_media_cb()
block.c | 11 +---------- blockdev.c | 5 +++++ include/block/block.h | 1 + 3 files changed, 7 insertions(+), 10 deletions(-)
Signature · 1 line
1.8.1.4
PATCH v2 1/2 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);
@@ -1681,7 +1680,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
@@ -158,6 +158,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 v2 2/2 block: move the bdrv_dev_change_media_cb()
In reply to Pavel HrdinaThe bdrv_dev_change_media_cb() should be called only for eject and change commands. 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
@@ -1084,10 +1084,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);
@@ -1389,8 +1385,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);
@@ -2845,8 +2839,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)
@@ -1071,14 +1072,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
Re: [Qemu-devel] [PATCH v2 2/2] block: move the bdrv_dev_change_media_cb()
Quoted 7 lines · Pavel Hrdina
blockdev.c +1
-0@@ -1071,14 +1072,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 {
blockdev.c static void qmp_bdrv_open_encrypted(BlockDriverState *bs, const char *filename, · line 1087
Quoted 10 lines
blockdev.c +3
-0error_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); }
Calling bdrv_dev_change_media_cb() after raising QERR_DEVICE_NOT_ENCRYPTED is intentional? It might warrant a comment.
Re: [Qemu-devel] [PATCH v2 2/2] block: move the bdrv_dev_change_media_cb()
Quoted 13 lines · Stefan Hajnoczi
Quoted 12 lines · Pavel Hrdina
blockdev.c +2
-0@@ -1071,14 +1072,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) {
Quoted 9 lines
blockdev.c static void qmp_bdrv_open_encrypted(BlockDriverState *bs, const char *filename, · line 1087
Quoted 5 lines
blockdev.c +2
-0error_set (errp,QERR_DEVICE_NOT_ENCRYPTED ,bdrv_get_device_name (bs)); } + +bdrv_dev_change_media_cb (bs, true); }Calling
bdrv_dev_change_media_cb()after raisingQERR_DEVICE_NOT_ENCRYPTEDis intentional? It might warrant a comment.
Sorry for my late answer.
It's just a warning, that you used a password for a block device that doesn't require it. The device is opened successfully and should be handled correctly (call the bdrv_dev_change_media_cb() ).
Pavel
Re: [Qemu-devel] [PATCH v2 2/2] block: move the bdrv_dev_change_media_cb()
Quoted 24 lines · Pavel Hrdina
Quoted 13 lines · Stefan Hajnoczi
Quoted 12 lines · Pavel Hrdina
blockdev.c +2
-0@@ -1071,14 +1072,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) {Quoted 9 lines
blockdev.c static void qmp_bdrv_open_encrypted(BlockDriverState *bs, const char *filename, · line 1087
Quoted 5 lines
blockdev.c +2
-0error_set (errp,QERR_DEVICE_NOT_ENCRYPTED ,bdrv_get_device_name (bs)); } + +bdrv_dev_change_media_cb (bs, true); }Calling
bdrv_dev_change_media_cb()after raisingQERR_DEVICE_NOT_ENCRYPTEDis intentional? It might warrant a comment.
Quoted 5 lines
Sorry for my late answer.
It's just a warning, that you used a password for a block device that doesn't require it. The device is opened successfully and should be handled correctly (call the
bdrv_dev_change_media_cb()).
Yep, IMO it's worth a comment that this isn't an "error" just a "warning".
Stefan
Re: [Qemu-devel] [PATCH v2 2/2] block: move the bdrv_dev_change_media_cb()
Quoted 25 lines · On Mon, 17 Jun 2013 14:33:10 +0200 Stefan Hajnoczi <[email protected]> wrote:
Quoted 24 lines · Pavel Hrdina
Quoted 22 lines · Stefan Hajnoczi
Quoted 7 lines · Pavel Hrdina
blockdev.c +1
-0@@ -1071,14 +1072,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 {blockdev.c static void qmp_bdrv_open_encrypted(BlockDriverState *bs, const char *filename, · line 1087
Quoted 10 lines
blockdev.c +3
-0error_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); }Calling
bdrv_dev_change_media_cb()after raisingQERR_DEVICE_NOT_ENCRYPTEDis intentional? It might warrant a comment.
Quoted 8 lines
Quoted 5 lines
Sorry for my late answer.
It's just a warning, that you used a password for a block device that doesn't require it. The device is opened successfully and should be handled correctly (call the
bdrv_dev_change_media_cb()).Yep, IMO it's worth a comment that this isn't an "error" just a "warning".
Actually, you can't have such a warning in QMP. You either fail or you succeed. We should just do what the current code does.
Re: [Qemu-devel] [PATCH v2 2/2] block: move the bdrv_dev_change_media_cb()
Quoted 34 lines · Luiz Capitulino
Quoted 31 lines · On Mon, 17 Jun 2013 14:33:10 +0200 Stefan Hajnoczi <[email protected]> wrote:
Quoted 29 lines · Pavel Hrdina
Quoted 13 lines · Stefan Hajnoczi
Quoted 12 lines · Pavel Hrdina
blockdev.c +2
-0@@ -1071,14 +1072,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) {Quoted 9 lines
blockdev.c static void qmp_bdrv_open_encrypted(BlockDriverState *bs, const char *filename, · line 1087
Quoted 5 lines
blockdev.c +2
-0error_set (errp,QERR_DEVICE_NOT_ENCRYPTED ,bdrv_get_device_name (bs)); } + +bdrv_dev_change_media_cb (bs, true); }Calling
bdrv_dev_change_media_cb()after raisingQERR_DEVICE_NOT_ENCRYPTEDis intentional? It might warrant a comment.Sorry for my late answer.
It's just a warning, that you used a password for a block device that doesn't require it. The device is opened successfully and should be handled correctly (call the
bdrv_dev_change_media_cb()).
Quoted 6 lines
Quoted 2 lines
Yep, IMO it's worth a comment that this isn't an "error" just a "warning".
Actually, you can't have such a warning in QMP. You either fail or you succeed. We should just do what the current code does.
This is the same logic as the old one. The device is loaded but the error is emitted.
Pavel
Re: [Qemu-devel] [PATCH v2 2/2] block: move the bdrv_dev_change_media_cb()
Quoted 38 lines · On Mon, 17 Jun 2013 15:25:24 +0200 Pavel Hrdina <[email protected]> wrote:
Quoted 37 lines · Luiz Capitulino
Quoted 33 lines · On Mon, 17 Jun 2013 14:33:10 +0200 Stefan Hajnoczi <[email protected]> wrote:
Quoted 24 lines · Pavel Hrdina
Quoted 13 lines · Stefan Hajnoczi
Quoted 12 lines · Pavel Hrdina
blockdev.c +2
-0@@ -1071,14 +1072,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) {Quoted 9 lines
blockdev.c static void qmp_bdrv_open_encrypted(BlockDriverState *bs, const char *filename, · line 1087
Quoted 5 lines
blockdev.c +2
-0error_set (errp,QERR_DEVICE_NOT_ENCRYPTED ,bdrv_get_device_name (bs)); } + +bdrv_dev_change_media_cb (bs, true); }Calling
bdrv_dev_change_media_cb()after raisingQERR_DEVICE_NOT_ENCRYPTEDis intentional? It might warrant a comment.Quoted 5 lines
Sorry for my late answer.
It's just a warning, that you used a password for a block device that doesn't require it. The device is opened successfully and should be handled correctly (call the
bdrv_dev_change_media_cb()).Yep, IMO it's worth a comment that this isn't an "error" just a "warning".
Quoted 6 lines
Quoted 3 lines
Actually, you can't have such a warning in QMP. You either fail or you succeed. We should just do what the current code does.
This is the same logic as the old one. The device is loaded but the error is emitted.
That's a bug if the operation succeeded.
Re: [Qemu-devel] [PATCH v2 2/2] block: move the bdrv_dev_change_media_cb()
Quoted 41 lines · Luiz Capitulino
Quoted 38 lines · On Mon, 17 Jun 2013 15:25:24 +0200 Pavel Hrdina <[email protected]> wrote:
Quoted 37 lines · Luiz Capitulino
Quoted 33 lines · On Mon, 17 Jun 2013 14:33:10 +0200 Stefan Hajnoczi <[email protected]> wrote:
Quoted 24 lines · Pavel Hrdina
Quoted 13 lines · Stefan Hajnoczi
Quoted 12 lines · Pavel Hrdina
(quoted diff) +2
-0@@ -1071,14 +1072,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) {Quoted 9 lines
Review static void qmp_bdrv_open_encrypted(BlockDriverState *bs, const char *filename, · line 1087
Quoted 5 lines
(quoted diff) +2
-0error_set(errp, QERR_DEVICE_NOT_ENCRYPTED, bdrv_get_device_name(bs)); } + + bdrv_dev_change_media_cb(bs, true); }Calling
bdrv_dev_change_media_cb()after raisingQERR_DEVICE_NOT_ENCRYPTEDis intentional? It might warrant a comment.Quoted 5 lines
Sorry for my late answer.
It's just a warning, that you used a password for a block device that doesn't require it. The device is opened successfully and should be handled correctly (call the
bdrv_dev_change_media_cb()).Yep, IMO it's worth a comment that this isn't an "error" just a "warning".
Quoted 9 lines
Quoted 6 lines
Quoted 3 lines
Actually, you can't have such a warning in QMP. You either fail or you succeed. We should just do what the current code does.
This is the same logic as the old one. The device is loaded but the error is emitted.
That's a bug if the operation succeeded.
In that case, how do you think, that we should handle the situation that user is trying to open device that isn't require the password, but user will provide the password?
I don't think that we should fail and abort that operation.
Pavel
Re: [Qemu-devel] [PATCH v2 2/2] block: move the bdrv_dev_change_media_cb()
Quoted 49 lines · On Mon, 17 Jun 2013 15:38:23 +0200 Pavel Hrdina <[email protected]> wrote:
Quoted 48 lines · Luiz Capitulino
Quoted 44 lines · On Mon, 17 Jun 2013 15:25:24 +0200 Pavel Hrdina <[email protected]> wrote:
Quoted 34 lines · Luiz Capitulino
Quoted 31 lines · On Mon, 17 Jun 2013 14:33:10 +0200 Stefan Hajnoczi <[email protected]> wrote:
Quoted 29 lines · Pavel Hrdina
Quoted 13 lines · Stefan Hajnoczi
Quoted 12 lines · Pavel Hrdina
(quoted diff) +2
-0@@ -1071,14 +1072,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) {Quoted 9 lines
Review static void qmp_bdrv_open_encrypted(BlockDriverState *bs, const char *filename, · line 1087
Quoted 5 lines
(quoted diff) +2
-0error_set(errp, QERR_DEVICE_NOT_ENCRYPTED, bdrv_get_device_name(bs)); } + + bdrv_dev_change_media_cb(bs, true); }Calling
bdrv_dev_change_media_cb()after raisingQERR_DEVICE_NOT_ENCRYPTEDis intentional? It might warrant a comment.Sorry for my late answer.
It's just a warning, that you used a password for a block device that doesn't require it. The device is opened successfully and should be handled correctly (call the
bdrv_dev_change_media_cb()).Quoted 6 lines
Quoted 2 lines
Yep, IMO it's worth a comment that this isn't an "error" just a "warning".
Actually, you can't have such a warning in QMP. You either fail or you succeed. We should just do what the current code does.
This is the same logic as the old one. The device is loaded but the error is emitted.
Quoted 8 lines
Quoted 2 lines
That's a bug if the operation succeeded.
In that case, how do you think, that we should handle the situation that user is trying to open device that isn't require the password, but user will provide the password?
I don't think that we should fail and abort that operation.
I agree, failing for real is probably incompatible by now.
I'm tempted to just drop the error then. But it's a very good idea to isolate this change on its own commit with a clear changelog, so that it's easy to identify it and revert case it also generates compatibility problems in the future.
Re: [Qemu-devel] [PATCH v2 2/2] block: move the bdrv_dev_change_media_cb()
In reply to Pavel HrdinaAm 17.06.2013 um 15:38 hat Pavel Hrdina geschrieben:
Quoted 14 lines
Quoted 14 lines
Quoted 13 lines
Quoted 10 lines
Quoted 6 lines
Quoted 3 lines
It's just a warning, that you used a password for a block device that doesn't require it. The device is opened successfully and should be handled correctly (call the
bdrv_dev_change_media_cb()).Yep, IMO it's worth a comment that this isn't an "error" just a "warning".
Actually, you can't have such a warning in QMP. You either fail or you succeed. We should just do what the current code does.
This is the same logic as the old one. The device is loaded but the error is emitted.
Quoted 8 lines
Quoted 2 lines
That's a bug if the operation succeeded.
In that case, how do you think, that we should handle the situation that user is trying to open device that isn't require the password, but user will provide the password?
I don't think that we should fail and abort that operation.
I think we should. The image and the options passed for it don't fit together, this is an error condition. Probably the user meant to pass a different image.
Kevin
Re: [Qemu-devel] [PATCH v2 2/2] block: move the bdrv_dev_change_media_cb()
Quoted 22 lines · On Mon, 17 Jun 2013 15:46:52 +0200 Kevin Wolf <[email protected]> wrote:
Am 17.06.2013 um 15:38 hat Pavel Hrdina geschrieben:
Quoted 21 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 6 lines
Quoted 3 lines
It's just a warning, that you used a password for a block device that doesn't require it. The device is opened successfully and should be handled correctly (call the
bdrv_dev_change_media_cb()).Yep, IMO it's worth a comment that this isn't an "error" just a "warning".
Quoted 9 lines
Quoted 6 lines
Quoted 3 lines
Actually, you can't have such a warning in QMP. You either fail or you succeed. We should just do what the current code does.
This is the same logic as the old one. The device is loaded but the error is emitted.
That's a bug if the operation succeeded.
In that case, how do you think, that we should handle the situation that user is trying to open device that isn't require the password, but user will provide the password?
Quoted 5 lines
Quoted 1 line
I don't think that we should fail and abort that operation.
I think we should. The image and the options passed for it don't fit together, this is an error condition. Probably the user meant to pass a different image.
I agree in principle, but I fear this might be an incompatible change as there might be clients out there assuming the VM is up and running (because it's what ends up happening).
Thinking about this again though, the client does get an error...
Re: [Qemu-devel] [PATCH v2 2/2] block: move the bdrv_dev_change_media_cb()
Am 17.06.2013 um 15:51 hat Luiz Capitulino geschrieben:
Quoted 31 lines
Quoted 22 lines · On Mon, 17 Jun 2013 15:46:52 +0200 Kevin Wolf <[email protected]> wrote:
Am 17.06.2013 um 15:38 hat Pavel Hrdina geschrieben:
Quoted 21 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 6 lines
Quoted 3 lines
It's just a warning, that you used a password for a block device that doesn't require it. The device is opened successfully and should be handled correctly (call the
bdrv_dev_change_media_cb()).Yep, IMO it's worth a comment that this isn't an "error" just a "warning".
Quoted 9 lines
Quoted 6 lines
Quoted 3 lines
Actually, you can't have such a warning in QMP. You either fail or you succeed. We should just do what the current code does.
This is the same logic as the old one. The device is loaded but the error is emitted.
That's a bug if the operation succeeded.
In that case, how do you think, that we should handle the situation that user is trying to open device that isn't require the password, but user will provide the password?
Quoted 5 lines
Quoted 1 line
I don't think that we should fail and abort that operation.
I think we should. The image and the options passed for it don't fit together, this is an error condition. Probably the user meant to pass a different image.
Quoted 5 lines
I agree in principle, but I fear this might be an incompatible change as there might be clients out there assuming the VM is up and running (because it's what ends up happening).
Thinking about this again though, the client does get an error...
Do you think any client is sending passwords for unencrypted images? Because if there is none (and I think we have reason to believe so), we don't break anything if we change the behaviour. And if something does break, we have uncovered a management tool bug, so that's not too bad either.
Kevin
Re: [Qemu-devel] [PATCH v2 2/2] block: move the bdrv_dev_change_media_cb()
Quoted 36 lines · On Mon, 17 Jun 2013 16:49:11 +0200 Kevin Wolf <[email protected]> wrote:
Am 17.06.2013 um 15:51 hat Luiz Capitulino geschrieben:
Quoted 35 lines
Quoted 22 lines · On Mon, 17 Jun 2013 15:46:52 +0200 Kevin Wolf <[email protected]> wrote:
Am 17.06.2013 um 15:38 hat Pavel Hrdina geschrieben:
Quoted 21 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 6 lines
Quoted 3 lines
It's just a warning, that you used a password for a block device that doesn't require it. The device is opened successfully and should be handled correctly (call the
bdrv_dev_change_media_cb()).Yep, IMO it's worth a comment that this isn't an "error" just a "warning".
Quoted 9 lines
Quoted 6 lines
Quoted 3 lines
Actually, you can't have such a warning in QMP. You either fail or you succeed. We should just do what the current code does.
This is the same logic as the old one. The device is loaded but the error is emitted.
That's a bug if the operation succeeded.
In that case, how do you think, that we should handle the situation that user is trying to open device that isn't require the password, but user will provide the password?
Quoted 5 lines
Quoted 1 line
I don't think that we should fail and abort that operation.
I think we should. The image and the options passed for it don't fit together, this is an error condition. Probably the user meant to pass a different image.
I agree in principle, but I fear this might be an incompatible change as there might be clients out there assuming the VM is up and running (because it's what ends up happening).
Quoted 7 lines
Quoted 1 line
Thinking about this again though, the client does get an error...
Do you think any client is sending passwords for unencrypted images? Because if there is none (and I think we have reason to believe so), we don't break anything if we change the behaviour. And if something does break, we have uncovered a management tool bug, so that's not too bad either.
Yes, I agree. I was being overly cautious when I suggested dropping the error, but I think you're right: we do send an error, so a well written client should just fail and shouldn't brake if we do the right thing.
So let's do the Right Thing, but I also suggest to do this in a separate commit so that it's easy to spot.
Re: [Qemu-devel] [PATCH v2 2/2] block: move the bdrv_dev_change_media_cb()
Am 17.06.2013 um 16:59 hat Luiz Capitulino geschrieben:
Quoted 47 lines
Quoted 36 lines · On Mon, 17 Jun 2013 16:49:11 +0200 Kevin Wolf <[email protected]> wrote:
Am 17.06.2013 um 15:51 hat Luiz Capitulino geschrieben:
Quoted 35 lines
Quoted 22 lines · On Mon, 17 Jun 2013 15:46:52 +0200 Kevin Wolf <[email protected]> wrote:
Am 17.06.2013 um 15:38 hat Pavel Hrdina geschrieben:
Quoted 21 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 6 lines
Quoted 3 lines
It's just a warning, that you used a password for a block device that doesn't require it. The device is opened successfully and should be handled correctly (call the
bdrv_dev_change_media_cb()).Yep, IMO it's worth a comment that this isn't an "error" just a "warning".
Quoted 9 lines
Quoted 6 lines
Quoted 3 lines
Actually, you can't have such a warning in QMP. You either fail or you succeed. We should just do what the current code does.
This is the same logic as the old one. The device is loaded but the error is emitted.
That's a bug if the operation succeeded.
In that case, how do you think, that we should handle the situation that user is trying to open device that isn't require the password, but user will provide the password?
Quoted 5 lines
Quoted 1 line
I don't think that we should fail and abort that operation.
I think we should. The image and the options passed for it don't fit together, this is an error condition. Probably the user meant to pass a different image.
I agree in principle, but I fear this might be an incompatible change as there might be clients out there assuming the VM is up and running (because it's what ends up happening).
Quoted 7 lines
Quoted 1 line
Thinking about this again though, the client does get an error...
Do you think any client is sending passwords for unencrypted images? Because if there is none (and I think we have reason to believe so), we don't break anything if we change the behaviour. And if something does break, we have uncovered a management tool bug, so that's not too bad either.
Quoted 6 lines
Yes, I agree. I was being overly cautious when I suggested dropping the error, but I think you're right: we do send an error, so a well written client should just fail and shouldn't brake if we do the right thing.
So let's do the Right Thing, but I also suggest to do this in a separate commit so that it's easy to spot.
Sure, I agree with one patch per logical change.
Kevin
Re: [Qemu-devel] [PATCH v2 2/2] block: move the bdrv_dev_change_media_cb()
Quoted 52 lines · Kevin Wolf
Am 17.06.2013 um 16:59 hat Luiz Capitulino geschrieben:
Quoted 51 lines
Quoted 36 lines · On Mon, 17 Jun 2013 16:49:11 +0200 Kevin Wolf <[email protected]> wrote:
Am 17.06.2013 um 15:51 hat Luiz Capitulino geschrieben:
Quoted 35 lines
Quoted 22 lines · On Mon, 17 Jun 2013 15:46:52 +0200 Kevin Wolf <[email protected]> wrote:
Am 17.06.2013 um 15:38 hat Pavel Hrdina geschrieben:
Quoted 21 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 6 lines
Quoted 3 lines
It's just a warning, that you used a password for a block device that doesn't require it. The device is opened successfully and should be handled correctly (call the
bdrv_dev_change_media_cb()).Yep, IMO it's worth a comment that this isn't an "error" just a "warning".
Quoted 9 lines
Quoted 6 lines
Quoted 3 lines
Actually, you can't have such a warning in QMP. You either fail or you succeed. We should just do what the current code does.
This is the same logic as the old one. The device is loaded but the error is emitted.
That's a bug if the operation succeeded.
In that case, how do you think, that we should handle the situation that user is trying to open device that isn't require the password, but user will provide the password?
Quoted 5 lines
Quoted 1 line
I don't think that we should fail and abort that operation.
I think we should. The image and the options passed for it don't fit together, this is an error condition. Probably the user meant to pass a different image.
I agree in principle, but I fear this might be an incompatible change as there might be clients out there assuming the VM is up and running (because it's what ends up happening).
Quoted 7 lines
Quoted 1 line
Thinking about this again though, the client does get an error...
Do you think any client is sending passwords for unencrypted images? Because if there is none (and I think we have reason to believe so), we don't break anything if we change the behaviour. And if something does break, we have uncovered a management tool bug, so that's not too bad either.
Yes, I agree. I was being overly cautious when I suggested dropping the error, but I think you're right: we do send an error, so a well written client should just fail and shouldn't brake if we do the right thing.
Quoted 7 lines
Quoted 2 lines
So let's do the Right Thing, but I also suggest to do this in a separate commit so that it's easy to spot.
Sure, I agree with one patch per logical change.
Kevin
In that case the v3 series is ready for apply?
I will write another patch to fix this bug.
Pavel
Quoted 11 lines · On Wed, 29 May 2013 18:18:17 +0200 Pavel Hrdina <[email protected]> wrote:
This fixes a regression introduced by commit
9ca111544.The first commit is done by Luiz and I've just use it as it is.
The second commit moves the
bdrv_dev_change_media_cb()intoeject_device(), called by QMP and HMP eject command, and intoqmp_bdrv_open_encrypted(), called by QMP and HMP change command. These are the only place where I think that should call thebdrv_dev_change_media_cb()function.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.
I'll defer review to the block guys, but as this works for me:
Quoted 12 lines
Luiz Capitulino (1):
block: makebdrv_dev_change_media_cb()publicPavel Hrdina (1):
block: move thebdrv_dev_change_media_cb()block.c | 11 +---------- blockdev.c | 5 +++++ include/block/block.h | 1 + 3 files changed, 7 insertions(+), 10 deletions(-)