Re: [Qemu-devel] [PATCH 0/2] block: fix spurious DEVICE_TRAY_MOVED events on shutdown
On Fri, 17 May 2013 16:23:51 +0200
Pavel Hrdina phrd...@redhat.com wrote:
On 25.4.2013 16:31, Luiz Capitulino wrote:
On Thu, 25 Apr 2013 16:29:45 +0200
Stefan Hajnoczi stefa...@gmail.com wrote:
On Thu, Apr 25, 2013 at 09:51:47AM -0400, Luiz Capitulino wrote:
On Mon, 22 Apr 2013 15:53:43 +0200
Stefan Hajnoczi stefa...@gmail.com wrote:
On Fri, Apr 19, 2013 at 01:47:17PM -0400, Luiz Capitulino wrote:
Hi,
This fixes a regression introduced by commit 9ca111544, as detailed in
patch 2/2, by moving bdrv_dev_change_media_cb() calls to callers of
bdrv_close() that need it, as suggested by Kevin.
Luiz Capitulino (2):
block: make bdrv_dev_change_media_cb() public
block: move bdrv_dev_change_media_cb() to callers that really need it
block.c | 5 +
blockdev.c| 2 ++
include/block/block.h | 1 +
3 files changed, 4 insertions(+), 4 deletions(-)
Looks okay but I'll wait for Markus or Kevin to review too. The media
change code is subtle, we've had a long history of fixes :).
I wouldn't say this is hugely important, but I'm targeting 1.5.
So, maybe lack of review means you could apply it? :)
Nice try :)
Hehe.
We've never gotten media change right. I really would appreciate a
second pair of eyes. There are still a couple of days until hard
freeze.
Holding off until then.
Ok, no problem.
Hi all,
I've just tested the side effect of my original commit and the
DEVICE_TRAY_MOVED event is emitted only if the CD-ROM is opened. If you
shutdown/reboot the guest with closed CD-ROM tray there is no
DEVICE_TRAY_MOVED event emitted. I think that this behavior is correct.
That's not what I'm seeing here, unless the tray is opened right before
shutdown, but even then the events are wrong as they notify the
transition closed - opened twice.
On HMP:
(qemu) info block
ide1-cd0: removable=1 locked=0 tray-open=0 io-status=ok [not inserted]
(qemu) system_powerdown
on QMP:
{
timestamp: {
seconds: 1369139052,
microseconds: 766612
},
event: DEVICE_TRAY_MOVED,
data: {
device: ide1-cd0,
tray-open: true
}
}
`
{
timestamp: {
seconds: 1369139052,
microseconds: 766798
},
event: DEVICE_TRAY_MOVED,
data: {
device: floppy0,
tray-open: true
}
}
Re: [Qemu-devel] [PATCH 0/2] block: fix spurious DEVICE_TRAY_MOVED events on shutdown
On 21.5.2013 14:26, Luiz Capitulino wrote:
On Fri, 17 May 2013 16:23:51 +0200
Pavel Hrdina phrd...@redhat.com wrote:
On 25.4.2013 16:31, Luiz Capitulino wrote:
On Thu, 25 Apr 2013 16:29:45 +0200
Stefan Hajnoczi stefa...@gmail.com wrote:
On Thu, Apr 25, 2013 at 09:51:47AM -0400, Luiz Capitulino wrote:
On Mon, 22 Apr 2013 15:53:43 +0200
Stefan Hajnoczi stefa...@gmail.com wrote:
On Fri, Apr 19, 2013 at 01:47:17PM -0400, Luiz Capitulino wrote:
Hi,
This fixes a regression introduced by commit 9ca111544, as detailed in
patch 2/2, by moving bdrv_dev_change_media_cb() calls to callers of
bdrv_close() that need it, as suggested by Kevin.
Luiz Capitulino (2):
block: make bdrv_dev_change_media_cb() public
block: move bdrv_dev_change_media_cb() to callers that really need it
block.c | 5 +
blockdev.c| 2 ++
include/block/block.h | 1 +
3 files changed, 4 insertions(+), 4 deletions(-)
Looks okay but I'll wait for Markus or Kevin to review too. The media
change code is subtle, we've had a long history of fixes :).
I wouldn't say this is hugely important, but I'm targeting 1.5.
So, maybe lack of review means you could apply it? :)
Nice try :)
Hehe.
We've never gotten media change right. I really would appreciate a
second pair of eyes. There are still a couple of days until hard
freeze.
Holding off until then.
Ok, no problem.
Hi all,
I've just tested the side effect of my original commit and the
DEVICE_TRAY_MOVED event is emitted only if the CD-ROM is opened. If you
shutdown/reboot the guest with closed CD-ROM tray there is no
DEVICE_TRAY_MOVED event emitted. I think that this behavior is correct.
That's not what I'm seeing here, unless the tray is opened right before
shutdown, but even then the events are wrong as they notify the
transition closed - opened twice.
On HMP:
(qemu) info block
ide1-cd0: removable=1 locked=0 tray-open=0 io-status=ok [not inserted]
(qemu) system_powerdown
on QMP:
{
timestamp: {
seconds: 1369139052,
microseconds: 766612
},
event: DEVICE_TRAY_MOVED,
data: {
device: ide1-cd0,
tray-open: true
}
}
`
{
timestamp: {
seconds: 1369139052,
microseconds: 766798
},
event: DEVICE_TRAY_MOVED,
data: {
device: floppy0,
tray-open: true
}
}
I've tested it and no the devices are not opened again. It is called
through main() - bdrv_close_all() - bdrv_close() -
bdrv_dev_change_media_cb().
So your patches make sense and we make sure that the
bdrv_dev_change_media_cb() is called on all required places.
I'll try to check if that two places are good enough.
Pavel
Re: [Qemu-devel] [PATCH 0/2] block: fix spurious DEVICE_TRAY_MOVED events on shutdown
On 25.4.2013 16:31, Luiz Capitulino wrote: On Thu, 25 Apr 2013 16:29:45 +0200 Stefan Hajnoczi stefa...@gmail.com wrote: On Thu, Apr 25, 2013 at 09:51:47AM -0400, Luiz Capitulino wrote: On Mon, 22 Apr 2013 15:53:43 +0200 Stefan Hajnoczi stefa...@gmail.com wrote: On Fri, Apr 19, 2013 at 01:47:17PM -0400, Luiz Capitulino wrote: Hi, This fixes a regression introduced by commit 9ca111544, as detailed in patch 2/2, by moving bdrv_dev_change_media_cb() calls to callers of bdrv_close() that need it, as suggested by Kevin. Luiz Capitulino (2): block: make bdrv_dev_change_media_cb() public block: move bdrv_dev_change_media_cb() to callers that really need it block.c | 5 + blockdev.c| 2 ++ include/block/block.h | 1 + 3 files changed, 4 insertions(+), 4 deletions(-) Looks okay but I'll wait for Markus or Kevin to review too. The media change code is subtle, we've had a long history of fixes :). I wouldn't say this is hugely important, but I'm targeting 1.5. So, maybe lack of review means you could apply it? :) Nice try :) Hehe. We've never gotten media change right. I really would appreciate a second pair of eyes. There are still a couple of days until hard freeze. Holding off until then. Ok, no problem. Hi all, I've just tested the side effect of my original commit and the DEVICE_TRAY_MOVED event is emitted only if the CD-ROM is opened. If you shutdown/reboot the guest with closed CD-ROM tray there is no DEVICE_TRAY_MOVED event emitted. I think that this behavior is correct. From what I know, these events are for notifying the QMP users about every changed state and it is correct to notify then in all cases when the CD-ROM tray is moved. Even during shutdown or reboot. Pavel
Re: [Qemu-devel] [PATCH 0/2] block: fix spurious DEVICE_TRAY_MOVED events on shutdown
On Mon, 22 Apr 2013 15:53:43 +0200 Stefan Hajnoczi stefa...@gmail.com wrote: On Fri, Apr 19, 2013 at 01:47:17PM -0400, Luiz Capitulino wrote: Hi, This fixes a regression introduced by commit 9ca111544, as detailed in patch 2/2, by moving bdrv_dev_change_media_cb() calls to callers of bdrv_close() that need it, as suggested by Kevin. Luiz Capitulino (2): block: make bdrv_dev_change_media_cb() public block: move bdrv_dev_change_media_cb() to callers that really need it block.c | 5 + blockdev.c| 2 ++ include/block/block.h | 1 + 3 files changed, 4 insertions(+), 4 deletions(-) Looks okay but I'll wait for Markus or Kevin to review too. The media change code is subtle, we've had a long history of fixes :). I wouldn't say this is hugely important, but I'm targeting 1.5. So, maybe lack of review means you could apply it? :)
Re: [Qemu-devel] [PATCH 0/2] block: fix spurious DEVICE_TRAY_MOVED events on shutdown
On Thu, Apr 25, 2013 at 09:51:47AM -0400, Luiz Capitulino wrote: On Mon, 22 Apr 2013 15:53:43 +0200 Stefan Hajnoczi stefa...@gmail.com wrote: On Fri, Apr 19, 2013 at 01:47:17PM -0400, Luiz Capitulino wrote: Hi, This fixes a regression introduced by commit 9ca111544, as detailed in patch 2/2, by moving bdrv_dev_change_media_cb() calls to callers of bdrv_close() that need it, as suggested by Kevin. Luiz Capitulino (2): block: make bdrv_dev_change_media_cb() public block: move bdrv_dev_change_media_cb() to callers that really need it block.c | 5 + blockdev.c| 2 ++ include/block/block.h | 1 + 3 files changed, 4 insertions(+), 4 deletions(-) Looks okay but I'll wait for Markus or Kevin to review too. The media change code is subtle, we've had a long history of fixes :). I wouldn't say this is hugely important, but I'm targeting 1.5. So, maybe lack of review means you could apply it? :) Nice try :) We've never gotten media change right. I really would appreciate a second pair of eyes. There are still a couple of days until hard freeze. Holding off until then. Stefan
Re: [Qemu-devel] [PATCH 0/2] block: fix spurious DEVICE_TRAY_MOVED events on shutdown
On Thu, 25 Apr 2013 16:29:45 +0200 Stefan Hajnoczi stefa...@gmail.com wrote: On Thu, Apr 25, 2013 at 09:51:47AM -0400, Luiz Capitulino wrote: On Mon, 22 Apr 2013 15:53:43 +0200 Stefan Hajnoczi stefa...@gmail.com wrote: On Fri, Apr 19, 2013 at 01:47:17PM -0400, Luiz Capitulino wrote: Hi, This fixes a regression introduced by commit 9ca111544, as detailed in patch 2/2, by moving bdrv_dev_change_media_cb() calls to callers of bdrv_close() that need it, as suggested by Kevin. Luiz Capitulino (2): block: make bdrv_dev_change_media_cb() public block: move bdrv_dev_change_media_cb() to callers that really need it block.c | 5 + blockdev.c| 2 ++ include/block/block.h | 1 + 3 files changed, 4 insertions(+), 4 deletions(-) Looks okay but I'll wait for Markus or Kevin to review too. The media change code is subtle, we've had a long history of fixes :). I wouldn't say this is hugely important, but I'm targeting 1.5. So, maybe lack of review means you could apply it? :) Nice try :) Hehe. We've never gotten media change right. I really would appreciate a second pair of eyes. There are still a couple of days until hard freeze. Holding off until then. Ok, no problem.
Re: [Qemu-devel] [PATCH 0/2] block: fix spurious DEVICE_TRAY_MOVED events on shutdown
On Fri, Apr 19, 2013 at 01:47:17PM -0400, Luiz Capitulino wrote: Hi, This fixes a regression introduced by commit 9ca111544, as detailed in patch 2/2, by moving bdrv_dev_change_media_cb() calls to callers of bdrv_close() that need it, as suggested by Kevin. Luiz Capitulino (2): block: make bdrv_dev_change_media_cb() public block: move bdrv_dev_change_media_cb() to callers that really need it block.c | 5 + blockdev.c| 2 ++ include/block/block.h | 1 + 3 files changed, 4 insertions(+), 4 deletions(-) Looks okay but I'll wait for Markus or Kevin to review too. The media change code is subtle, we've had a long history of fixes :). Stefan
[Qemu-devel] [PATCH 0/2] block: fix spurious DEVICE_TRAY_MOVED events on shutdown
Hi, This fixes a regression introduced by commit 9ca111544, as detailed in patch 2/2, by moving bdrv_dev_change_media_cb() calls to callers of bdrv_close() that need it, as suggested by Kevin. Luiz Capitulino (2): block: make bdrv_dev_change_media_cb() public block: move bdrv_dev_change_media_cb() to callers that really need it block.c | 5 + blockdev.c| 2 ++ include/block/block.h | 1 + 3 files changed, 4 insertions(+), 4 deletions(-) -- 1.8.1.4
