From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:46657) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Ry1FS-0001WQ-4f for qemu-devel@nongnu.org; Thu, 16 Feb 2012 08:19:50 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Ry1FM-00039a-Tm for qemu-devel@nongnu.org; Thu, 16 Feb 2012 08:19:46 -0500 Received: from mx1.redhat.com ([209.132.183.28]:35072) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Ry1FM-00039U-Lj for qemu-devel@nongnu.org; Thu, 16 Feb 2012 08:19:40 -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 q1GDJdwx015524 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Thu, 16 Feb 2012 08:19:39 -0500 Date: Thu, 16 Feb 2012 11:19:38 -0200 From: Luiz Capitulino Message-ID: <20120216111938.5437c542@doriath.home> In-Reply-To: <20120216111447.2160fa63@doriath.home> References: <1329331347-11232-1-git-send-email-lcapitulino@redhat.com> <1329331347-11232-5-git-send-email-lcapitulino@redhat.com> <20120216111447.2160fa63@doriath.home> 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: Markus Armbruster Cc: kwolf@redhat.com, pbonzini@redhat.com, eblake@redhat.com, qemu-devel@nongnu.org On Thu, 16 Feb 2012 11:14:47 -0200 Luiz Capitulino wrote: > 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. > > > > + > > > +{ "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). Forgot to say that I'm fine with either way, just picked the one that made more sense to me... > > > > > > + } > > > + if (load) { > > > + /* tray close */ > > > + bdrv_emit_qmp_eject_event(bs, false); > > > > Likewise (!tray_was_closed && load)? > > > > > + } > > > } > > > } > > > > > > @@ -3567,6 +3587,10 @@ void bdrv_eject(BlockDriverState *bs, bool eject_flag, bool tray_changed) > > > if (drv && drv->bdrv_eject) { > > > drv->bdrv_eject(bs, eject_flag); > > > } > > > + > > > + if (tray_changed) { > > > + bdrv_emit_qmp_eject_event(bs, eject_flag); > > > + } > > > > I wonder why we bother to call bdrv_eject() with false tray_changed at > > all. If there's no good reason for that, we could replace PATCH 3/4 by > > one that simply skips the call when the virtual tray stays put. > > My understanding is that this is used for passthrough: we forward ejects, > independent of the state of the tray. Seems correct to me, but I'm not > against changing it. > > > > > > } > > > > > > /** > > > diff --git a/monitor.c b/monitor.c > > > index aadbdcb..1758f03 100644 > > > --- a/monitor.c > > > +++ b/monitor.c > > > @@ -485,6 +485,9 @@ void monitor_protocol_event(MonitorEvent event, QObject *data) > > > case QEVENT_BLOCK_JOB_CANCELLED: > > > event_name = "BLOCK_JOB_CANCELLED"; > > > break; > > > + case QEVENT_BLOCK_MEDIUM_EJECT: > > > + event_name = "BLOCK_MEDIUM_EJECT"; > > > + break; > > > default: > > > abort(); > > > break; > > > diff --git a/monitor.h b/monitor.h > > > index b72ea07..a2555e5 100644 > > > --- a/monitor.h > > > +++ b/monitor.h > > > @@ -38,6 +38,7 @@ typedef enum MonitorEvent { > > > QEVENT_SPICE_DISCONNECTED, > > > QEVENT_BLOCK_JOB_COMPLETED, > > > QEVENT_BLOCK_JOB_CANCELLED, > > > + QEVENT_BLOCK_MEDIUM_EJECT, > > > QEVENT_MAX, > > > } MonitorEvent; > > >