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

[Pavel Hrdina <phrdina@redhat.com>]

On 21.5.2013 14:26, Luiz Capitulino wrote:
> On Fri, 17 May 2013 16:23:51 +0200
> Pavel Hrdina <phrdina@redhat.com> wrote:
>
>> On 25.4.2013 16:31, Luiz Capitulino wrote:
>>> On Thu, 25 Apr 2013 16:29:45 +0200
>>> Stefan Hajnoczi <stefanha@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 <stefanha@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