From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:44049) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1RrtKy-00040F-NM for qemu-devel@nongnu.org; Mon, 30 Jan 2012 10:40:13 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1RrtKs-0006BP-4u for qemu-devel@nongnu.org; Mon, 30 Jan 2012 10:40:08 -0500 Received: from mx1.redhat.com ([209.132.183.28]:23014) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1RrtKr-0006B5-Qu for qemu-devel@nongnu.org; Mon, 30 Jan 2012 10:40:02 -0500 Received: from int-mx10.intmail.prod.int.phx2.redhat.com (int-mx10.intmail.prod.int.phx2.redhat.com [10.5.11.23]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id q0UFe00N023362 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Mon, 30 Jan 2012 10:40:00 -0500 Message-ID: <4F26BA9A.1050705@redhat.com> Date: Mon, 30 Jan 2012 16:43:22 +0100 From: Kevin Wolf MIME-Version: 1.0 References: <20120124161628.4bf2592c@doriath.home> <4F1F0E9F.8080302@redhat.com> <4F1FC030.2030705@redhat.com> <20120125104204.55eec0a1@doriath.home> <20120126155757.3e97a0c1@doriath.home> <4F2273CF.3050107@redhat.com> <20120130131843.7f88117a@doriath> In-Reply-To: <20120130131843.7f88117a@doriath> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH] qmp: add BLOCK_MEDIUM_EJECT event List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Luiz Capitulino Cc: Eric Blake , qemu-devel , Markus Armbruster Am 30.01.2012 16:18, schrieb Luiz Capitulino: > On Fri, 27 Jan 2012 10:52:15 +0100 > Kevin Wolf wrote: > >> Am 26.01.2012 18:57, schrieb Luiz Capitulino: >>> On Wed, 25 Jan 2012 10:42:04 -0200 >>> Luiz Capitulino wrote: >>> >>>> On Wed, 25 Jan 2012 09:41:20 +0100 >>>> Kevin Wolf wrote: >>>> >>>>> Am 24.01.2012 21:03, schrieb Eric Blake: >>>>>> On 01/24/2012 11:16 AM, Luiz Capitulino wrote: >>>>>>> Libvirt wants to be notified when the guest ejects a medium, so that >>>>>>> it can update its view of the guest. >>>>>>> >>>>>>> This code has been originally written by Daniel Berrange. It adds >>>>>>> the event to IDE and SCSI emulation. >>>>>>> >>>>>>> Please, note that this only covers guest initiated ejects, that's, >>>>>>> the QMP/HMP commands 'eject' and 'change' are not covered. >>>>> >>>>> What's the reason for this behaviour? It feels inconsistent. >>>> >>>> I don't think it's inconsistent because we're limiting it to guest initiated >>>> actions. Also, the mngt app knows when it sends a 'eject' or 'change' command. >>>> The exception is if it allows HMP to run in parallel with QMP, but I don't think >>>> this is recommended (at least not for commands that change any VM state). >>> >>> Let me elaborate more. We have two options: >>> >>> 1. Emit the event for guest initiated ejects (this patch, although I think >>> the event should be renamed to GUEST_MEDIUM_EJECT) >>> >>> 2. Emit the event for guest & QMP initiated ejects, that's, also emit the >>> event for the eject and change commands >>> >>> The first thing to note is that, they're not mutually exclusive. If we do >>> item 1 now, we can add 2 later (as a different event). >>> >>> But my point is that doing 2 is problematic and we should avoid it. The problem >>> is that the semantics of eject and change are complex and/or buggy. And something >>> I've learned about confusing semantics in QMP is that, they will give you headaches >>> soon or later. >> >> But I'm not really interested in the semantics of QMP commands, because >> they are irrelevant for the events. > > I do think they are relevant, because the event will have to match what > the eject/change commands do with the tray. If what they do is messy, the > event will turn out to be messy too. > > Now, I don't doubt this can be all fixed and made clean. I'm just not sure > if it's worth it. If a mess best describes to the outside what we're doing to the device, then having a messy event is the best you can expect. Or in other words, if you're doing messy things with the device and you straighten things out in the event generation, then your events are lying to the management tools. >> My view is that a device generates an event each time its try status >> changes. > > Agreed. > >> If it opens the tray, it does so by calling bdrv_eject - an >> obvious place to send an event. > > Yes, bdrv_eject() is called from device models. That's the easy part. But note > that device models will also call bdrv_eject() when closing the tray. We should have events for both, so this is a good thing. But even if we didn't want to, devices tell us which one it is, so we could send an event only on tray open. >> If it closes the tray or the eject >> monitor command is used, we go through bdrv_dev_change_media_cb - not >> quite as obvious, but I think this despite its name this is really a >> "tray status changed externally" callback. > > Not so simple for the eject command, because bdrv_dev_change_media_cb() is > called from bdrv_close() but if the tray is locked by the guest and it later > unlocks it (and that _will_ happen with you run eject with the tray locked > by the guest) the event will be also emitted. Which is correct behaviour because the medium _has_ been ejected, right? The eject command doesn't go through bdrv_closed() with a locked tray, unless force=true. So you get the event only when the guest really ejects it, and not when your button press happens to do nothing more than sending an eject request to the guest. >> QMP commands may cause any of these actions to occur. But the event is >> tied to the actions and not to the QMP commands that may or may not >> cause them according to their confusing semantics. >> >>> The main problem with eject is that it's inconsistent with the handling of >>> the tray status. Try this: >>> >>> 1. Eject with no medium inserted >>> >>> (qemu) info block >>> ide1-cd0: removable=1 locked=0 tray-open=0 io-status=ok [not inserted] >>> (qemu) eject ide1-cd0 >>> (qemu) info block >>> ide1-cd0: removable=1 locked=0 tray-open=0 io-status=ok [not inserted] >>> (qemu) >>> >>> Conclusion: the tray didn't move, we shouldn't emit the event. >> >> Fails before bdrv_close, so bdrv_dev_change_media_cb is never called. >> Works as it is today. > > Yes, my point with these two examples is that, ideally, eject should left > the tray in the same state when it's issued. Today the tray status after > eject depends on if a media is inserted or not. > > Maybe not a big issue, but I felt I should raise it. Oh, I didn't even notice that. Would be less surprising if the tray was opened after ejecting an empty drive. I don't think it makes a real difference because we don't have a separate close_tray command, but the action is (just like tray_open, where necessary) automatically included in change. >>> 2. Eject with medium inserted >>> >>> (qemu) info block >>> ide1-cd0: removable=1 locked=0 tray-open=0 io-status=ok file=/tmp/vl.iKvBAF backing_file=/mnt/fernando/isos/linux/Fedora-14-x86_64-DVD.iso ro=0 drv=qcow2 encrypted=0 bps=0 bps_rd=0 bps_wr=0 iops=0 iops_rd=0 iops_wr=0 >>> (qemu) eject ide1-cd0 >>> (qemu) info block >>> ide1-cd0: removable=1 locked=0 tray-open=1 io-status=ok [not inserted] >>> (qemu) >>> >>> Conclusion: the tray opened and the media was purged. We should emit the event. >> >> bdrv_dev_change_media_cb is called, we can emit an event there. >> >>> >>> 3. Eject with medium inserted and locked >>> >>> (qemu) info block >>> ide1-cd0: removable=1 locked=1 tray-open=0 io-status=ok file=/tmp/vl.2LHApn backing_file=/mnt/fernando/isos/linux/Fedora-16-x86_64-DVD.iso ro=0 drv=qcow2 encrypted=0 bps=0 bps_rd=0 bps_wr=0 iops=0 iops_rd=0 iops_wr=0 >>> (qemu) eject ide1-cd0 >>> Device 'ide1-cd0' is locked >>> (qemu) info block >>> ide1-cd0: removable=1 locked=0 tray-open=1 io-status=ok file=/tmp/vl.2LHApn backing_file=/mnt/fernando/isos/linux/Fedora-16-x86_64-DVD.iso ro=0 drv=qcow2 encrypted=0 bps=0 bps_rd=0 bps_wr=0 iops=0 iops_rd=0 iops_wr=0 >>> (qemu) >>> >>> Conclusion: eject returned an error, but a few seconds later the tray opened and >>> the media wasn't purged. What happened here is that, the _guest_ >>> opened the tray. The code in this patch would trigger the event, but >>> we shouldn't emit it twice if we cover eject & change (ie. special case) >> >> bdrv_dev_change_media_cb is not called because media cannot be ejected >> with a locked drive. Instead bdrv_dev_eject_request is called which >> doesn't emit an event. >> >> If the guest happens to initiate an eject itself after receiving the >> eject request, it calls bdrv_eject, where we can emit an event. >> >> If we had force=true in the initial eject command, bdrv_close is called, >> which in turn goes through bdrv_dev_change_media_cb where an event is >> emitted. > > Can't this race with the guest eject? Can't see how, there's nothing asynchronous in the path. Kevin