From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:50297) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Ry22N-0005IS-Gx for qemu-devel@nongnu.org; Thu, 16 Feb 2012 09:10:25 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Ry22L-000502-Ot for qemu-devel@nongnu.org; Thu, 16 Feb 2012 09:10:19 -0500 Received: from mx1.redhat.com ([209.132.183.28]:33816) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Ry22L-0004zk-Gl for qemu-devel@nongnu.org; Thu, 16 Feb 2012 09:10:17 -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 q1GEAGwZ022738 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Thu, 16 Feb 2012 09:10:16 -0500 Date: Thu, 16 Feb 2012 12:10:15 -0200 From: Luiz Capitulino Message-ID: <20120216121015.7ccaa462@doriath.home> In-Reply-To: <4F3D054C.9020603@redhat.com> References: <1329331347-11232-1-git-send-email-lcapitulino@redhat.com> <1329331347-11232-5-git-send-email-lcapitulino@redhat.com> <20120216111447.2160fa63@doriath.home> <4F3D054C.9020603@redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 4/4] qmp: add BLOCK_MEDIUM_EJECT event List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Kevin Wolf Cc: pbonzini@redhat.com, eblake@redhat.com, Markus Armbruster , qemu-devel@nongnu.org On Thu, 16 Feb 2012 14:31:56 +0100 Kevin Wolf wrote: > Am 16.02.2012 14:14, schrieb Luiz Capitulino: > > On Thu, 16 Feb 2012 10:25:43 +0100 > > Markus Armbruster wrote: > > > >> Luiz Capitulino writes: > >> > >>> It's emitted whenever the tray is moved by the guest or by HMP/QMP > >>> commands. > >> > >> I like the simplicity of this patch. A few remarks inline. > >> > >>> Signed-off-by: Luiz Capitulino > >>> --- > >>> QMP/qmp-events.txt | 17 +++++++++++++++++ > >>> block.c | 24 ++++++++++++++++++++++++ > >>> monitor.c | 3 +++ > >>> monitor.h | 1 + > >>> 4 files changed, 45 insertions(+), 0 deletions(-) > >>> > >>> diff --git a/QMP/qmp-events.txt b/QMP/qmp-events.txt > >>> index 06cb404..c52e7fe 100644 > >>> --- a/QMP/qmp-events.txt > >>> +++ b/QMP/qmp-events.txt > >>> @@ -26,6 +26,23 @@ Example: > >>> Note: If action is "stop", a STOP event will eventually follow the > >>> BLOCK_IO_ERROR event. > >>> > >>> +BLOCK_MEDIUM_EJECT > >>> +------------------ > >>> + > >>> +It's emitted whenever the tray is moved by the guest or by HMP/QMP > >>> +commands. > >>> + > >>> +Data: > >>> + > >>> +- "device": device name (json-string) > >>> +- "ejected": true if the tray has been opened or false if it has been closed > >> > >> I'd make this "load", because I find "true to load, false to eject" more > >> pleasing, but that's strictly a matter of taste. > > > > I also think that in the end it's just matter of taste. > > > > I don't like "load" because (at least in my mind) it suggests a medium has been > > loaded and that might not be true. > > > > Another good option (and now I think I'll change to it) is "tray-open". That > > matches with query-block's output and I think the meaning is more direct. > > I chose "ejected" because that matches with the well-known eject program. > > I like tray-open. > > >>> + > >>> +{ "event": "BLOCK_MEDIUM_EJECT", > >>> + "data": { "device": "ide1-cd0", > >>> + "ejected": true > >>> + }, > >>> + "timestamp": { "seconds": 1265044230, "microseconds": 450486 } } > >>> + > >>> RESET > >>> ----- > >>> > >>> diff --git a/block.c b/block.c > >>> index 47f1823..e5e2a5f 100644 > >>> --- a/block.c > >>> +++ b/block.c > >>> @@ -970,10 +970,30 @@ void bdrv_emit_qmp_error_event(const BlockDriverState *bdrv, > >>> qobject_decref(data); > >>> } > >>> > >>> +static void bdrv_emit_qmp_eject_event(BlockDriverState *bs, bool ejected) > >>> +{ > >>> + QObject *data; > >>> + > >>> + data = qobject_from_jsonf("{ 'device': %s, 'ejected': %i }", > >>> + bdrv_get_device_name(bs), ejected); > >>> + monitor_protocol_event(QEVENT_BLOCK_MEDIUM_EJECT, data); > >>> + > >>> + qobject_decref(data); > >>> +} > >>> + > >>> static 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); > >>> bs->dev_ops->change_media_cb(bs->dev_opaque, load); > >>> + if (tray_was_closed) { > >>> + /* tray open */ > >>> + bdrv_emit_qmp_eject_event(bs, true); > >> > >> Emits event even when tray stays closed (tray_was_closed && !load), > >> doesn't it? > > > > Yes, but that's on purpose. There are two approaches here: > > > > 1. Emit the event as the operations would be carried on real hw. On real > > hw, your example would be the equivalent of: open the tray, remove the > > medium if any, close the tray. This patch would report the opening and > > closing of the tray > > > > 2. Emit the event only if the final state of the tray is different from > > the previous state. In this case, we wouldn't emit any event when > > change is issued on a closed tray > > > > I agree that item 2 matches better with the description "the event is emitted > > whenever the tray moves", but then the change command would only emit the event > > when the tray is already open (ie. it will emit the event for tray close). > > What we want to have is the semantics of 1. which is how I understand > "tray has moved". We don't observe the state before a 'change' monitor > command and after it and emit an event if tray status isn't the same any > more, but we actually observe the tray moves in the single steps that > are done for implementing the monitor command. > > If you need magic like if (tray_was_closed) in bdrv_dev_change_media_cb, > this seems to indicate that we're not doing things completely right in > the internal model. We're probably taking shortcuts so that this > function really sees a closed -> closed transition when we really have > closed -> open -> closed. Agreed. > Depending on how hard it would be to fix I would suggest that either you > fix the internal model, or that we check that the externally visible > behaviour is the same as if we did it right and postpone the fix for the > internal model. We have two external entities: the guest and the mngt app. It seems to me that the guest is seeing each step at a time. With this patch, the mngt app would see two events when the change command is issued and the tray is already closed (open tray & close tray) and one event if the tray is opened. Seems correct to me. Now, I'm wondering if BLOCK_MEDIUM_EJECT is a good name for the event, maybe BLOCK_TRAY_MOVED is a better one? Btw, wrt fixing the internal model, I could do it in a different series, But I don't know exactly how to. Maybe bdrv_dev_change_media_cb() should be broken into multiple operations...