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

[Kevin Wolf]

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.

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.

Kevin