On Thu, 16 Feb 2012 14:31:56 +0100 Kevin Wolf <kw...@redhat.com> wrote:
> Am 16.02.2012 14:14, schrieb Luiz Capitulino: > > On Thu, 16 Feb 2012 10:25:43 +0100 > > Markus Armbruster <arm...@redhat.com> wrote: > > > >> Luiz Capitulino <lcapitul...@redhat.com> 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 <lcapitul...@redhat.com> > >>> --- > >>> 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...