From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:59851) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Uem7e-00026r-ER for qemu-devel@nongnu.org; Tue, 21 May 2013 08:57:03 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Uem7X-0000TR-Hs for qemu-devel@nongnu.org; Tue, 21 May 2013 08:56:58 -0400 Received: from mx1.redhat.com ([209.132.183.28]:52464) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Uem7X-0000TE-AR for qemu-devel@nongnu.org; Tue, 21 May 2013 08:56:51 -0400 Message-ID: <519B6F0F.2010008@redhat.com> Date: Tue, 21 May 2013 14:56:47 +0200 From: Pavel Hrdina MIME-Version: 1.0 References: <1366393639-20651-1-git-send-email-lcapitulino@redhat.com> <20130422135343.GF21317@stefanha-thinkpad.redhat.com> <20130425095147.75b90841@redhat.com> <20130425142945.GA11388@stefanha-thinkpad.redhat.com> <20130425103159.6bcf026d@redhat.com> <51963D77.4090107@redhat.com> <20130521082622.0fcb2061@redhat.com> In-Reply-To: <20130521082622.0fcb2061@redhat.com> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 0/2] block: fix spurious DEVICE_TRAY_MOVED events on shutdown List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Luiz Capitulino Cc: kwolf@redhat.com, Stefan Hajnoczi , qemu-devel@nongnu.org, armbru@redhat.com On 21.5.2013 14:26, Luiz Capitulino wrote: > On Fri, 17 May 2013 16:23:51 +0200 > Pavel Hrdina wrote: > >> On 25.4.2013 16:31, Luiz Capitulino wrote: >>> On Thu, 25 Apr 2013 16:29:45 +0200 >>> Stefan Hajnoczi 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 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