Re: [Qemu-devel] [PATCH 4/4] qmp: add BLOCK_MEDIUM_EJECT event

[Luiz Capitulino]

On Thu, 16 Feb 2012 14:31:56 +0100
Kevin Wolf <address@hidden> wrote:

> Am 16.02.2012 14:14, schrieb Luiz Capitulino:
> > On Thu, 16 Feb 2012 10:25:43 +0100
> > Markus Armbruster <address@hidden> wrote:
> > 
> >> Luiz Capitulino <address@hidden> 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 <address@hidden>
> >>> ---
> >>>  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...