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

[Luiz Capitulino]

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.

> 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.
But this is probably not going to be an issue when we implement the more
general BLOCK_TRAY_STATUS (or something like it).

> > 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.

> > diff --git a/monitor.c b/monitor.c
> > index f63cce0..4a81467 100644
> > --- a/monitor.c
> > +++ b/monitor.c
> > @@ -450,6 +450,9 @@ void monitor_protocol_event(MonitorEvent event, QObject 
> > *data)
> >          case QEVENT_VNC_DISCONNECTED:
> >              event_name = "VNC_DISCONNECTED";
> >              break;
> > +        case QEVENT_BLOCK_MEDIA_EJECT:
> > +            event_name = "BLOCK_MEDIA_EJECT";
> > +            break;
> >          case QEVENT_BLOCK_IO_ERROR:
> >              event_name = "BLOCK_IO_ERROR";
> >              break;
> > diff --git a/monitor.h b/monitor.h
> > index 4f2d328..7a04137 100644
> > --- a/monitor.h
> > +++ b/monitor.h
> > @@ -29,6 +29,7 @@ typedef enum MonitorEvent {
> >      QEVENT_VNC_CONNECTED,
> >      QEVENT_VNC_INITIALIZED,
> >      QEVENT_VNC_DISCONNECTED,
> > +    QEVENT_BLOCK_MEDIA_EJECT,
> >      QEVENT_BLOCK_IO_ERROR,
> >      QEVENT_RTC_CHANGE,
> >      QEVENT_WATCHDOG,
>