Re: [Qemu-devel] [PATCH 3/3] QMP: Introduce the BLOCK_MEDIA_EJECT event

[Markus Armbruster]

Luiz Capitulino <address@hidden> writes:

> On Sat, 28 May 2011 09:58:24 +0200
> Markus Armbruster <address@hidden> wrote:
>
>> Luiz Capitulino <address@hidden> writes:
>> 
>> > Conforms to the event specification defined in the
>> > QMP/qmp-events.txt file.
>> 
>> I'd squash PATCH 2+3.
>
> I agree this would be more logical, but people have complained that it's hard
> to review new commands/events when its documentation is hidden or mixed with
> code somewhere in the series. I think that make sense.
>
> Maybe, it would be better to copy & paste the documentation in patch 0/0...
>
>> > Please, note the following details:
>> >
>> >  o The event should be emitted only by devices which support the
>> >    eject operation, which currently are: CDROMs (IDE and SCSI)
>> >    and floppies
>> >
>> >  o Human monitor commands "eject" and "change" also cause the
>> >    event to be emitted
>> >
>> >  o The event is only emitted when there's a tray transition from
>> >    closed to opened. To implement this in the human monitor, we
>> >    only emit the event if the device is removable and a media is
>> >    present
>> 
>> Rationale?
>
> This implementation covers the basic use case, which is to let clients know
> that an already inserted media has been ejected. I was under the assumption
> that this is the most important thing a client wants to know, as it will
> have to update its internal state and that only has to be done for the media
> it knows about (ie. the ones inserted by the client itself).
>
> But...
>
>> Wouldn't it be easier if we just report tray status change, regardless
>> of media?
>
> Yes, this seems to make sense.
>
>> > Signed-off-by: Luiz Capitulino <address@hidden>
>> > ---
>> >  block.c    |   12 ++++++++++++
>> >  block.h    |    1 +
>> >  blockdev.c |    5 +++++
>> >  monitor.c  |    3 +++
>> >  monitor.h  |    1 +
>> >  5 files changed, 22 insertions(+), 0 deletions(-)
>> >
>> 
>> Copied from PATCH 2 for review purposes:
>> 
>>   diff --git a/QMP/qmp-events.txt b/QMP/qmp-events.txt
>>   index 0ce5d4e..d53c129 100644
>>   --- a/QMP/qmp-events.txt
>>   +++ b/QMP/qmp-events.txt
>>   @@ -1,6 +1,24 @@
>>                       QEMU Monitor Protocol Events
>>                       ============================
>> 
>>   +BLOCK_MEDIA_EJECT
>>   +-----------------
>>   +
>>   +Emitted when a removable disk media (such as a CDROM or floppy) is 
>> ejected.
>>   +
>>   +Data:
>>   +
>>   +- "device": device name (json-string)
>>   +
>>   +Example:
>>   +
>>   +{ "event": "BLOCK_MEDIA_EJECT",
>>   +    "data": { "device": "ide1-cd0" },
>>   +    "timestamp": { "seconds": 1265044230, "microseconds": 450486 } }
>>   +
>>   +NOTE: A disk media can be ejected by the guest or by monitor commands 
>> (such
>>   +as "eject" and "change")
>>   +
>>    BLOCK_IO_ERROR
>>    --------------
>> 
>> The event reports "tray opening".  Do we need one for "tray closing" as
>> well?
>
> At first I thought it wouldn't be needed (and maybe most use cases don't
> require it), but reporting the tray status is more general and easier to do,
> so I think it's what I'm going to do.
>
>> > diff --git a/block.c b/block.c
>> > index f5014cf..dbe813b 100644
>> > --- a/block.c
>> > +++ b/block.c
>> > @@ -1656,6 +1656,15 @@ int bdrv_is_allocated(BlockDriverState *bs, int64_t 
>> > sector_num, int nb_sectors,
>> >      return bs->drv->bdrv_is_allocated(bs, sector_num, nb_sectors, pnum);
>> >  }
>> >  
>> > +void bdrv_eject_mon_event(const BlockDriverState *bdrv)
>> > +{
>> > +    QObject *data;
>> > +
>> > +    data = qobject_from_jsonf("{ 'device': %s }", bdrv->device_name);
>> > +    monitor_protocol_event(QEVENT_BLOCK_MEDIA_EJECT, data);
>> > +    qobject_decref(data);
>> > +}
>> > +
>> >  void bdrv_error_mon_event(const BlockDriverState *bdrv,
>> >                            BlockMonEventAction action, int is_read)
>> >  {
>> > @@ -2770,6 +2779,9 @@ int bdrv_eject(BlockDriverState *bs, int eject_flag)
>> >          ret = 0;
>> >      }
>> >      if (ret >= 0) {
>> > +        if (eject_flag && !bs->tray_open) {
>> > +            bdrv_eject_mon_event(bs);
>> > +        }
>> >          bs->tray_open = eject_flag;
>> >      }
>> >  
>> 
>> This covers guest-initiated eject.
>>
>> 
>> The event is suppressed when the tray is already open.
>
> Yes, because there's no visible state change.

No objection to that, as long as it's consistently done.

>> The event is not suppressed when the tray is empty, is it?  Contradicts
>> spec.
>
> Why does it contradicts the spec? No media is ejected if the tray is empty.

Yes.  Nevertheless, the event is triggered.

> But this is probably not going to be an issue when we implement the more
> general BLOCK_TRAY_STATUS (or something like it).

No need to debate the fine points of "media eject" then.

>> > diff --git a/block.h b/block.h
>> > index 1f58eab..e4053dd 100644
>> > --- a/block.h
>> > +++ b/block.h
>> > @@ -50,6 +50,7 @@ typedef enum {
>> >      BDRV_ACTION_REPORT, BDRV_ACTION_IGNORE, BDRV_ACTION_STOP
>> >  } BlockMonEventAction;
>> >  
>> > +void bdrv_eject_mon_event(const BlockDriverState *bdrv);
>> >  void bdrv_error_mon_event(const BlockDriverState *bdrv,
>> >                            BlockMonEventAction action, int is_read);
>> >  void bdrv_info_print(Monitor *mon, const QObject *data);
>> > diff --git a/blockdev.c b/blockdev.c
>> > index 6e0eb83..5fd0043 100644
>> > --- a/blockdev.c
>> > +++ b/blockdev.c
>> > @@ -661,6 +661,11 @@ static int eject_device(Monitor *mon, 
>> > BlockDriverState *bs, int force)
>> >              return -1;
>> >          }
>> >      }
>> > +
>> > +    if (bdrv_is_removable(bs) && bdrv_is_inserted(bs)) {
>> > +        bdrv_eject_mon_event(bs);
>> > +    }
>> > +
>> >      bdrv_close(bs);
>> >      return 0;
>> >  }
>> 
>> This covers monitor-initiated eject (commands eject and change).
>> 
>> The event is not suppressed when the tray is already open (previous
>> guest-initiated eject), is it?.  Contradicts spec.
>
> That's a bug.
>
>> The event is suppressed when the tray is empty.
>> 
>> "eject -f" on a non-removable drive does not trigger an event.  Why
>> treat it specially?  I'm not saying you shouldn't, just wondering.
>
> Ejecting a non-removable drive is a qemu bug.

It's clearly intentional, so it's a (mis-)feature, not a bug.

[...]