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

[Markus Armbruster]

Luiz Capitulino <address@hidden> writes:

> Conforms to the event specification defined in the
> QMP/qmp-events.txt file.

I'd squash PATCH 2+3.

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

Wouldn't it be easier if we just report tray status change, regardless
of media?

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

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

The event is not suppressed when the tray is empty, is it?  Contradicts
spec.

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

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.

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