[Qemu-devel] [PATCH v2 0/4]: QMP: add BLOCK_MEDIUM_EJECT event

[Qemu-devel] [PATCH v2 0/4]: QMP: add BLOCK_MEDIUM_EJECT event

[Luiz Capitulino]

There's been one non-rfc patch and one or two rfc ones. This is v2 of the
non-rfc one.

I think this version does what Kevin and Markus were asking: the event is
emitted whenever the tray moves, be it the guest or HMP/QMP commands.

In a previous email I said that I'd be reviving an old series that breaks the
eject and change commands into multiple tray commands (tray-open/close,
medium-insert/remove etc), but that's not so trivial anymore as the tray state
moved to device models (still possible of course, but requires a bit more
work). So I decided to do it the way patch 4/4 does it.

 QMP/qmp-events.txt |   17 ++++++++++
 block.c            |   84 +++++++++++++++++++++++++++++++++------------------
 block.h            |    8 ++--
 block/raw-posix.c  |    6 ++--
 block/raw.c        |    4 +-
 block_int.h        |    2 +-
 hw/ide/atapi.c     |    2 +-
 hw/ide/core.c      |    8 ++--
 hw/scsi-disk.c     |    8 ++--
 hw/virtio-blk.c    |    6 ++--
 monitor.c          |    3 ++
 monitor.h          |    1 +
 12 files changed, 97 insertions(+), 52 deletions(-)

[Qemu-devel] [PATCH 1/4] block: Rename bdrv_mon_event() & BlockMonEventAction

[Luiz Capitulino]

They are QMP events, not monitor events. Rename them accordingly.

Also, move bdrv_emit_qmp_error_event() up in the file. A new event will
be added soon and it's good to have them next each other.

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 block.c         |   58 +++++++++++++++++++++++++++---------------------------
 block.h         |    6 ++--
 hw/ide/core.c   |    6 ++--
 hw/scsi-disk.c  |    6 ++--
 hw/virtio-blk.c |    6 ++--
 5 files changed, 41 insertions(+), 41 deletions(-)

diff --git a/block.c b/block.c
index 3621d11..d867d0f 100644
--- a/block.c
+++ b/block.c
@@ -941,6 +941,35 @@ void bdrv_set_dev_ops(BlockDriverState *bs, const BlockDevOps *ops,
     }
 }
 
+void bdrv_emit_qmp_error_event(const BlockDriverState *bdrv,
+                               BlockQMPEventAction action, int is_read)
+{
+    QObject *data;
+    const char *action_str;
+
+    switch (action) {
+    case BDRV_ACTION_REPORT:
+        action_str = "report";
+        break;
+    case BDRV_ACTION_IGNORE:
+        action_str = "ignore";
+        break;
+    case BDRV_ACTION_STOP:
+        action_str = "stop";
+        break;
+    default:
+        abort();
+    }
+
+    data = qobject_from_jsonf("{ 'device': %s, 'action': %s, 'operation': %s }",
+                              bdrv->device_name,
+                              action_str,
+                              is_read ? "read" : "write");
+    monitor_protocol_event(QEVENT_BLOCK_IO_ERROR, 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) {
@@ -2244,35 +2273,6 @@ int bdrv_is_allocated(BlockDriverState *bs, int64_t sector_num, int nb_sectors,
     return data.ret;
 }
 
-void bdrv_mon_event(const BlockDriverState *bdrv,
-                    BlockMonEventAction action, int is_read)
-{
-    QObject *data;
-    const char *action_str;
-
-    switch (action) {
-    case BDRV_ACTION_REPORT:
-        action_str = "report";
-        break;
-    case BDRV_ACTION_IGNORE:
-        action_str = "ignore";
-        break;
-    case BDRV_ACTION_STOP:
-        action_str = "stop";
-        break;
-    default:
-        abort();
-    }
-
-    data = qobject_from_jsonf("{ 'device': %s, 'action': %s, 'operation': %s }",
-                              bdrv->device_name,
-                              action_str,
-                              is_read ? "read" : "write");
-    monitor_protocol_event(QEVENT_BLOCK_IO_ERROR, data);
-
-    qobject_decref(data);
-}
-
 BlockInfoList *qmp_query_block(Error **errp)
 {
     BlockInfoList *head = NULL, *cur_item = NULL;
diff --git a/block.h b/block.h
index cae289b..36a53a0 100644
--- a/block.h
+++ b/block.h
@@ -85,15 +85,15 @@ typedef enum {
 
 typedef enum {
     BDRV_ACTION_REPORT, BDRV_ACTION_IGNORE, BDRV_ACTION_STOP
-} BlockMonEventAction;
+} BlockQMPEventAction;
 
 void bdrv_iostatus_enable(BlockDriverState *bs);
 void bdrv_iostatus_reset(BlockDriverState *bs);
 void bdrv_iostatus_disable(BlockDriverState *bs);
 bool bdrv_iostatus_is_enabled(const BlockDriverState *bs);
 void bdrv_iostatus_set_err(BlockDriverState *bs, int error);
-void bdrv_mon_event(const BlockDriverState *bdrv,
-                    BlockMonEventAction action, int is_read);
+void bdrv_emit_qmp_error_event(const BlockDriverState *bdrv,
+                               BlockQMPEventAction action, int is_read);
 void bdrv_info_print(Monitor *mon, const QObject *data);
 void bdrv_info(Monitor *mon, QObject **ret_data);
 void bdrv_stats_print(Monitor *mon, const QObject *data);
diff --git a/hw/ide/core.c b/hw/ide/core.c
index 56b219b..0856385 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -519,7 +519,7 @@ static int ide_handle_rw_error(IDEState *s, int error, int op)
     BlockErrorAction action = bdrv_get_on_error(s->bs, is_read);
 
     if (action == BLOCK_ERR_IGNORE) {
-        bdrv_mon_event(s->bs, BDRV_ACTION_IGNORE, is_read);
+        bdrv_emit_qmp_error_event(s->bs, BDRV_ACTION_IGNORE, is_read);
         return 0;
     }
 
@@ -527,7 +527,7 @@ static int ide_handle_rw_error(IDEState *s, int error, int op)
             || action == BLOCK_ERR_STOP_ANY) {
         s->bus->dma->ops->set_unit(s->bus->dma, s->unit);
         s->bus->error_status = op;
-        bdrv_mon_event(s->bs, BDRV_ACTION_STOP, is_read);
+        bdrv_emit_qmp_error_event(s->bs, BDRV_ACTION_STOP, is_read);
         vm_stop(RUN_STATE_IO_ERROR);
         bdrv_iostatus_set_err(s->bs, error);
     } else {
@@ -537,7 +537,7 @@ static int ide_handle_rw_error(IDEState *s, int error, int op)
         } else {
             ide_rw_error(s);
         }
-        bdrv_mon_event(s->bs, BDRV_ACTION_REPORT, is_read);
+        bdrv_emit_qmp_error_event(s->bs, BDRV_ACTION_REPORT, is_read);
     }
 
     return 1;
diff --git a/hw/scsi-disk.c b/hw/scsi-disk.c
index 399e51e..94f6af6 100644
--- a/hw/scsi-disk.c
+++ b/hw/scsi-disk.c
@@ -233,14 +233,14 @@ static int scsi_handle_rw_error(SCSIDiskReq *r, int error)
     BlockErrorAction action = bdrv_get_on_error(s->qdev.conf.bs, is_read);
 
     if (action == BLOCK_ERR_IGNORE) {
-        bdrv_mon_event(s->qdev.conf.bs, BDRV_ACTION_IGNORE, is_read);
+        bdrv_emit_qmp_error_event(s->qdev.conf.bs, BDRV_ACTION_IGNORE, is_read);
         return 0;
     }
 
     if ((error == ENOSPC && action == BLOCK_ERR_STOP_ENOSPC)
             || action == BLOCK_ERR_STOP_ANY) {
 
-        bdrv_mon_event(s->qdev.conf.bs, BDRV_ACTION_STOP, is_read);
+        bdrv_emit_qmp_error_event(s->qdev.conf.bs, BDRV_ACTION_STOP, is_read);
         vm_stop(RUN_STATE_IO_ERROR);
         bdrv_iostatus_set_err(s->qdev.conf.bs, error);
         scsi_req_retry(&r->req);
@@ -259,7 +259,7 @@ static int scsi_handle_rw_error(SCSIDiskReq *r, int error)
             scsi_check_condition(r, SENSE_CODE(IO_ERROR));
             break;
         }
-        bdrv_mon_event(s->qdev.conf.bs, BDRV_ACTION_REPORT, is_read);
+        bdrv_emit_qmp_error_event(s->qdev.conf.bs, BDRV_ACTION_REPORT, is_read);
     }
     return 1;
 }
diff --git a/hw/virtio-blk.c b/hw/virtio-blk.c
index a5a4396..49990f8 100644
--- a/hw/virtio-blk.c
+++ b/hw/virtio-blk.c
@@ -69,7 +69,7 @@ static int virtio_blk_handle_rw_error(VirtIOBlockReq *req, int error,
     VirtIOBlock *s = req->dev;
 
     if (action == BLOCK_ERR_IGNORE) {
-        bdrv_mon_event(s->bs, BDRV_ACTION_IGNORE, is_read);
+        bdrv_emit_qmp_error_event(s->bs, BDRV_ACTION_IGNORE, is_read);
         return 0;
     }
 
@@ -77,14 +77,14 @@ static int virtio_blk_handle_rw_error(VirtIOBlockReq *req, int error,
             || action == BLOCK_ERR_STOP_ANY) {
         req->next = s->rq;
         s->rq = req;
-        bdrv_mon_event(s->bs, BDRV_ACTION_STOP, is_read);
+        bdrv_emit_qmp_error_event(s->bs, BDRV_ACTION_STOP, is_read);
         vm_stop(RUN_STATE_IO_ERROR);
         bdrv_iostatus_set_err(s->bs, error);
     } else {
         virtio_blk_req_complete(req, VIRTIO_BLK_S_IOERR);
         bdrv_acct_done(s->bs, &req->acct);
         g_free(req);
-        bdrv_mon_event(s->bs, BDRV_ACTION_REPORT, is_read);
+        bdrv_emit_qmp_error_event(s->bs, BDRV_ACTION_REPORT, is_read);
     }
 
     return 1;
-- 
1.7.9.111.gf3fb0.dirty

[Qemu-devel] [PATCH 2/4] block: bdrv_eject(): Make eject_flag a real bool

[Luiz Capitulino]

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 block.c           |    2 +-
 block.h           |    2 +-
 block/raw-posix.c |    6 +++---
 block/raw.c       |    2 +-
 block_int.h       |    2 +-
 5 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/block.c b/block.c
index d867d0f..b5710e1 100644
--- a/block.c
+++ b/block.c
@@ -3560,7 +3560,7 @@ int bdrv_media_changed(BlockDriverState *bs)
 /**
  * If eject_flag is TRUE, eject the media. Otherwise, close the tray
  */
-void bdrv_eject(BlockDriverState *bs, int eject_flag)
+void bdrv_eject(BlockDriverState *bs, bool eject_flag)
 {
     BlockDriver *drv = bs->drv;
 
diff --git a/block.h b/block.h
index 36a53a0..ce8c5ed 100644
--- a/block.h
+++ b/block.h
@@ -257,7 +257,7 @@ int bdrv_enable_write_cache(BlockDriverState *bs);
 int bdrv_is_inserted(BlockDriverState *bs);
 int bdrv_media_changed(BlockDriverState *bs);
 void bdrv_lock_medium(BlockDriverState *bs, bool locked);
-void bdrv_eject(BlockDriverState *bs, int eject_flag);
+void bdrv_eject(BlockDriverState *bs, bool eject_flag);
 void bdrv_get_format(BlockDriverState *bs, char *buf, int buf_size);
 BlockDriverState *bdrv_find(const char *name);
 BlockDriverState *bdrv_next(BlockDriverState *bs);
diff --git a/block/raw-posix.c b/block/raw-posix.c
index 2ee5d69..2d1bc13 100644
--- a/block/raw-posix.c
+++ b/block/raw-posix.c
@@ -994,7 +994,7 @@ static int floppy_media_changed(BlockDriverState *bs)
     return ret;
 }
 
-static void floppy_eject(BlockDriverState *bs, int eject_flag)
+static void floppy_eject(BlockDriverState *bs, bool eject_flag)
 {
     BDRVRawState *s = bs->opaque;
     int fd;
@@ -1084,7 +1084,7 @@ static int cdrom_is_inserted(BlockDriverState *bs)
     return 0;
 }
 
-static void cdrom_eject(BlockDriverState *bs, int eject_flag)
+static void cdrom_eject(BlockDriverState *bs, bool eject_flag)
 {
     BDRVRawState *s = bs->opaque;
 
@@ -1194,7 +1194,7 @@ static int cdrom_is_inserted(BlockDriverState *bs)
     return raw_getlength(bs) > 0;
 }
 
-static void cdrom_eject(BlockDriverState *bs, int eject_flag)
+static void cdrom_eject(BlockDriverState *bs, bool eject_flag)
 {
     BDRVRawState *s = bs->opaque;
 
diff --git a/block/raw.c b/block/raw.c
index 6098070..1cdac0c 100644
--- a/block/raw.c
+++ b/block/raw.c
@@ -61,7 +61,7 @@ static int raw_media_changed(BlockDriverState *bs)
     return bdrv_media_changed(bs->file);
 }
 
-static void raw_eject(BlockDriverState *bs, int eject_flag)
+static void raw_eject(BlockDriverState *bs, bool eject_flag)
 {
     bdrv_eject(bs->file, eject_flag);
 }
diff --git a/block_int.h b/block_int.h
index 7be2988..f954add 100644
--- a/block_int.h
+++ b/block_int.h
@@ -189,7 +189,7 @@ struct BlockDriver {
     /* removable device specific */
     int (*bdrv_is_inserted)(BlockDriverState *bs);
     int (*bdrv_media_changed)(BlockDriverState *bs);
-    void (*bdrv_eject)(BlockDriverState *bs, int eject_flag);
+    void (*bdrv_eject)(BlockDriverState *bs, bool eject_flag);
     void (*bdrv_lock_medium)(BlockDriverState *bs, bool locked);
 
     /* to control generic scsi devices */
-- 
1.7.9.111.gf3fb0.dirty

[Qemu-devel] [PATCH 3/4] block: bdrv_eject(): Add tray_changed parameter

[Luiz Capitulino]

It's true if the tray has changed its state. This is going to be used
by the next commit.

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 block.c        |    2 +-
 block.h        |    2 +-
 block/raw.c    |    2 +-
 hw/ide/atapi.c |    2 +-
 hw/ide/core.c  |    2 +-
 hw/scsi-disk.c |    2 +-
 6 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/block.c b/block.c
index b5710e1..47f1823 100644
--- a/block.c
+++ b/block.c
@@ -3560,7 +3560,7 @@ int bdrv_media_changed(BlockDriverState *bs)
 /**
  * If eject_flag is TRUE, eject the media. Otherwise, close the tray
  */
-void bdrv_eject(BlockDriverState *bs, bool eject_flag)
+void bdrv_eject(BlockDriverState *bs, bool eject_flag, bool tray_changed)
 {
     BlockDriver *drv = bs->drv;
 
diff --git a/block.h b/block.h
index ce8c5ed..1d4cb94 100644
--- a/block.h
+++ b/block.h
@@ -257,7 +257,7 @@ int bdrv_enable_write_cache(BlockDriverState *bs);
 int bdrv_is_inserted(BlockDriverState *bs);
 int bdrv_media_changed(BlockDriverState *bs);
 void bdrv_lock_medium(BlockDriverState *bs, bool locked);
-void bdrv_eject(BlockDriverState *bs, bool eject_flag);
+void bdrv_eject(BlockDriverState *bs, bool eject_flag, bool tray_changed);
 void bdrv_get_format(BlockDriverState *bs, char *buf, int buf_size);
 BlockDriverState *bdrv_find(const char *name);
 BlockDriverState *bdrv_next(BlockDriverState *bs);
diff --git a/block/raw.c b/block/raw.c
index 1cdac0c..8343df4 100644
--- a/block/raw.c
+++ b/block/raw.c
@@ -63,7 +63,7 @@ static int raw_media_changed(BlockDriverState *bs)
 
 static void raw_eject(BlockDriverState *bs, bool eject_flag)
 {
-    bdrv_eject(bs->file, eject_flag);
+    bdrv_eject(bs->file, eject_flag, false);
 }
 
 static void raw_lock_medium(BlockDriverState *bs, bool locked)
diff --git a/hw/ide/atapi.c b/hw/ide/atapi.c
index 0adb27b..d2e2a84 100644
--- a/hw/ide/atapi.c
+++ b/hw/ide/atapi.c
@@ -883,7 +883,7 @@ static void cmd_start_stop_unit(IDEState *s, uint8_t* buf)
             ide_atapi_cmd_error(s, sense, ASC_MEDIA_REMOVAL_PREVENTED);
             return;
         }
-        bdrv_eject(s->bs, !start);
+        bdrv_eject(s->bs, !start, s->tray_open != !start);
         s->tray_open = !start;
     }
 
diff --git a/hw/ide/core.c b/hw/ide/core.c
index 0856385..baac6c8 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -2081,7 +2081,7 @@ static int ide_tray_state_post_load(void *opaque, int version_id)
 {
     IDEState *s = opaque;
 
-    bdrv_eject(s->bs, s->tray_open);
+    bdrv_eject(s->bs, s->tray_open, false);
     bdrv_lock_medium(s->bs, s->tray_locked);
     return 0;
 }
diff --git a/hw/scsi-disk.c b/hw/scsi-disk.c
index 94f6af6..9dad9c1 100644
--- a/hw/scsi-disk.c
+++ b/hw/scsi-disk.c
@@ -1050,7 +1050,7 @@ static int scsi_disk_emulate_start_stop(SCSIDiskReq *r)
                                  : SENSE_CODE(NOT_READY_REMOVAL_PREVENTED));
             return -1;
         }
-        bdrv_eject(s->qdev.conf.bs, !start);
+        bdrv_eject(s->qdev.conf.bs, !start, s->tray_open != !start);
         s->tray_open = !start;
     }
     return 0;
-- 
1.7.9.111.gf3fb0.dirty

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

[Luiz Capitulino]

It's emitted whenever the tray is moved by the guest or by HMP/QMP
commands.

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 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
+
+{ "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);
+        }
+        if (load) {
+            /* tray close */
+            bdrv_emit_qmp_eject_event(bs, false);
+        }
     }
 }
 
@@ -3567,6 +3587,10 @@ void bdrv_eject(BlockDriverState *bs, bool eject_flag, bool tray_changed)
     if (drv && drv->bdrv_eject) {
         drv->bdrv_eject(bs, eject_flag);
     }
+
+    if (tray_changed) {
+        bdrv_emit_qmp_eject_event(bs, eject_flag);
+    }
 }
 
 /**
diff --git a/monitor.c b/monitor.c
index aadbdcb..1758f03 100644
--- a/monitor.c
+++ b/monitor.c
@@ -485,6 +485,9 @@ void monitor_protocol_event(MonitorEvent event, QObject *data)
         case QEVENT_BLOCK_JOB_CANCELLED:
             event_name = "BLOCK_JOB_CANCELLED";
             break;
+        case QEVENT_BLOCK_MEDIUM_EJECT:
+             event_name = "BLOCK_MEDIUM_EJECT";
+            break;
         default:
             abort();
             break;
diff --git a/monitor.h b/monitor.h
index b72ea07..a2555e5 100644
--- a/monitor.h
+++ b/monitor.h
@@ -38,6 +38,7 @@ typedef enum MonitorEvent {
     QEVENT_SPICE_DISCONNECTED,
     QEVENT_BLOCK_JOB_COMPLETED,
     QEVENT_BLOCK_JOB_CANCELLED,
+    QEVENT_BLOCK_MEDIUM_EJECT,
     QEVENT_MAX,
 } MonitorEvent;
 
-- 
1.7.9.111.gf3fb0.dirty

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

[Markus Armbruster]

Luiz Capitulino <lcapitulino@redhat.com> 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 <lcapitulino@redhat.com>
> ---
>  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.

> +
> +{ "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?

> +        }
> +        if (load) {
> +            /* tray close */
> +            bdrv_emit_qmp_eject_event(bs, false);

Likewise (!tray_was_closed && load)?

> +        }
>      }
>  }
>  
> @@ -3567,6 +3587,10 @@ void bdrv_eject(BlockDriverState *bs, bool eject_flag, bool tray_changed)
>      if (drv && drv->bdrv_eject) {
>          drv->bdrv_eject(bs, eject_flag);
>      }
> +
> +    if (tray_changed) {
> +        bdrv_emit_qmp_eject_event(bs, eject_flag);
> +    }

I wonder why we bother to call bdrv_eject() with false tray_changed at
all.  If there's no good reason for that, we could replace PATCH 3/4 by
one that simply skips the call when the virtual tray stays put.

>  }
>  
>  /**
> diff --git a/monitor.c b/monitor.c
> index aadbdcb..1758f03 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -485,6 +485,9 @@ void monitor_protocol_event(MonitorEvent event, QObject *data)
>          case QEVENT_BLOCK_JOB_CANCELLED:
>              event_name = "BLOCK_JOB_CANCELLED";
>              break;
> +        case QEVENT_BLOCK_MEDIUM_EJECT:
> +             event_name = "BLOCK_MEDIUM_EJECT";
> +            break;
>          default:
>              abort();
>              break;
> diff --git a/monitor.h b/monitor.h
> index b72ea07..a2555e5 100644
> --- a/monitor.h
> +++ b/monitor.h
> @@ -38,6 +38,7 @@ typedef enum MonitorEvent {
>      QEVENT_SPICE_DISCONNECTED,
>      QEVENT_BLOCK_JOB_COMPLETED,
>      QEVENT_BLOCK_JOB_CANCELLED,
> +    QEVENT_BLOCK_MEDIUM_EJECT,
>      QEVENT_MAX,
>  } MonitorEvent;

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

[Luiz Capitulino]

On Thu, 16 Feb 2012 10:25:43 +0100
Markus Armbruster <armbru@redhat.com> wrote:

> Luiz Capitulino <lcapitulino@redhat.com> 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 <lcapitulino@redhat.com>
> > ---
> >  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.

> > +
> > +{ "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).

> 
> > +        }
> > +        if (load) {
> > +            /* tray close */
> > +            bdrv_emit_qmp_eject_event(bs, false);
> 
> Likewise (!tray_was_closed && load)?
> 
> > +        }
> >      }
> >  }
> >  
> > @@ -3567,6 +3587,10 @@ void bdrv_eject(BlockDriverState *bs, bool eject_flag, bool tray_changed)
> >      if (drv && drv->bdrv_eject) {
> >          drv->bdrv_eject(bs, eject_flag);
> >      }
> > +
> > +    if (tray_changed) {
> > +        bdrv_emit_qmp_eject_event(bs, eject_flag);
> > +    }
> 
> I wonder why we bother to call bdrv_eject() with false tray_changed at
> all.  If there's no good reason for that, we could replace PATCH 3/4 by
> one that simply skips the call when the virtual tray stays put.

My understanding is that this is used for passthrough: we forward ejects,
independent of the state of the tray. Seems correct to me, but I'm not
against changing it.

> 
> >  }
> >  
> >  /**
> > diff --git a/monitor.c b/monitor.c
> > index aadbdcb..1758f03 100644
> > --- a/monitor.c
> > +++ b/monitor.c
> > @@ -485,6 +485,9 @@ void monitor_protocol_event(MonitorEvent event, QObject *data)
> >          case QEVENT_BLOCK_JOB_CANCELLED:
> >              event_name = "BLOCK_JOB_CANCELLED";
> >              break;
> > +        case QEVENT_BLOCK_MEDIUM_EJECT:
> > +             event_name = "BLOCK_MEDIUM_EJECT";
> > +            break;
> >          default:
> >              abort();
> >              break;
> > diff --git a/monitor.h b/monitor.h
> > index b72ea07..a2555e5 100644
> > --- a/monitor.h
> > +++ b/monitor.h
> > @@ -38,6 +38,7 @@ typedef enum MonitorEvent {
> >      QEVENT_SPICE_DISCONNECTED,
> >      QEVENT_BLOCK_JOB_COMPLETED,
> >      QEVENT_BLOCK_JOB_CANCELLED,
> > +    QEVENT_BLOCK_MEDIUM_EJECT,
> >      QEVENT_MAX,
> >  } MonitorEvent;
> 

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

[Luiz Capitulino]

On Thu, 16 Feb 2012 11:14:47 -0200
Luiz Capitulino <lcapitulino@redhat.com> wrote:

> On Thu, 16 Feb 2012 10:25:43 +0100
> Markus Armbruster <armbru@redhat.com> wrote:
> 
> > Luiz Capitulino <lcapitulino@redhat.com> 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 <lcapitulino@redhat.com>
> > > ---
> > >  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.
> 
> > > +
> > > +{ "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).

Forgot to say that I'm fine with either way, just picked the one that made
more sense to me...

> 
> > 
> > > +        }
> > > +        if (load) {
> > > +            /* tray close */
> > > +            bdrv_emit_qmp_eject_event(bs, false);
> > 
> > Likewise (!tray_was_closed && load)?
> > 
> > > +        }
> > >      }
> > >  }
> > >  
> > > @@ -3567,6 +3587,10 @@ void bdrv_eject(BlockDriverState *bs, bool eject_flag, bool tray_changed)
> > >      if (drv && drv->bdrv_eject) {
> > >          drv->bdrv_eject(bs, eject_flag);
> > >      }
> > > +
> > > +    if (tray_changed) {
> > > +        bdrv_emit_qmp_eject_event(bs, eject_flag);
> > > +    }
> > 
> > I wonder why we bother to call bdrv_eject() with false tray_changed at
> > all.  If there's no good reason for that, we could replace PATCH 3/4 by
> > one that simply skips the call when the virtual tray stays put.
> 
> My understanding is that this is used for passthrough: we forward ejects,
> independent of the state of the tray. Seems correct to me, but I'm not
> against changing it.
> 
> > 
> > >  }
> > >  
> > >  /**
> > > diff --git a/monitor.c b/monitor.c
> > > index aadbdcb..1758f03 100644
> > > --- a/monitor.c
> > > +++ b/monitor.c
> > > @@ -485,6 +485,9 @@ void monitor_protocol_event(MonitorEvent event, QObject *data)
> > >          case QEVENT_BLOCK_JOB_CANCELLED:
> > >              event_name = "BLOCK_JOB_CANCELLED";
> > >              break;
> > > +        case QEVENT_BLOCK_MEDIUM_EJECT:
> > > +             event_name = "BLOCK_MEDIUM_EJECT";
> > > +            break;
> > >          default:
> > >              abort();
> > >              break;
> > > diff --git a/monitor.h b/monitor.h
> > > index b72ea07..a2555e5 100644
> > > --- a/monitor.h
> > > +++ b/monitor.h
> > > @@ -38,6 +38,7 @@ typedef enum MonitorEvent {
> > >      QEVENT_SPICE_DISCONNECTED,
> > >      QEVENT_BLOCK_JOB_COMPLETED,
> > >      QEVENT_BLOCK_JOB_CANCELLED,
> > > +    QEVENT_BLOCK_MEDIUM_EJECT,
> > >      QEVENT_MAX,
> > >  } MonitorEvent;
> > 
> 

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 <armbru@redhat.com> wrote:
> 
>> Luiz Capitulino <lcapitulino@redhat.com> 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 <lcapitulino@redhat.com>
>>> ---
>>>  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

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 <kwolf@redhat.com> wrote:

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

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

[Paolo Bonzini]

On 02/16/2012 03:10 PM, Luiz Capitulino wrote:
> 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.

The guest is seeing each step separately, but that is managed by the
device model (which sends two separate error codes: a NOT READY for
opening the tray, and a UNIT ATTENTION for closing the tray and changing
medium) rather than by the monitor.

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

change_media_cb is really a move_tray_cb and we could rename it.
Calling it twice from the change command should work.

Paolo

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

[Kevin Wolf]

Am 17.02.2012 11:32, schrieb Paolo Bonzini:
> On 02/16/2012 03:10 PM, Luiz Capitulino wrote:
>> 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.
> 
> The guest is seeing each step separately, but that is managed by the
> device model (which sends two separate error codes: a NOT READY for
> opening the tray, and a UNIT ATTENTION for closing the tray and changing
> medium) rather than by the monitor.

But this separation isn't set in stone. Maybe it would make sense to
move the logic into the block layer. Of course, then it would have to
meet the needs of floppies as well.

Kevin

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

[Markus Armbruster]

Kevin Wolf <kwolf@redhat.com> writes:

> Am 17.02.2012 11:32, schrieb Paolo Bonzini:
>> On 02/16/2012 03:10 PM, Luiz Capitulino wrote:
>>> 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.
>> 
>> The guest is seeing each step separately, but that is managed by the
>> device model (which sends two separate error codes: a NOT READY for
>> opening the tray, and a UNIT ATTENTION for closing the tray and changing
>> medium) rather than by the monitor.
>
> But this separation isn't set in stone. Maybe it would make sense to
> move the logic into the block layer. Of course, then it would have to
> meet the needs of floppies as well.

And whatever other device with removable media comes up.

We should not move device-specific stuff from device models into the
block layer.  The block layer is fat enough as it is.  And it's not
meant for sharing code among device models.  There are better ways to do
that.

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

[Kevin Wolf]

Am 17.02.2012 12:48, schrieb Markus Armbruster:
> Kevin Wolf <kwolf@redhat.com> writes:
> 
>> Am 17.02.2012 11:32, schrieb Paolo Bonzini:
>>> On 02/16/2012 03:10 PM, Luiz Capitulino wrote:
>>>> 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.
>>>
>>> The guest is seeing each step separately, but that is managed by the
>>> device model (which sends two separate error codes: a NOT READY for
>>> opening the tray, and a UNIT ATTENTION for closing the tray and changing
>>> medium) rather than by the monitor.
>>
>> But this separation isn't set in stone. Maybe it would make sense to
>> move the logic into the block layer. Of course, then it would have to
>> meet the needs of floppies as well.
> 
> And whatever other device with removable media comes up.
> 
> We should not move device-specific stuff from device models into the
> block layer.  The block layer is fat enough as it is.  And it's not
> meant for sharing code among device models.  There are better ways to do
> that.

My motivation is not sharing code but getting the model right.

As Paolo says, currently we only tell the device that the medium has
changed. It is the device's responsibility to fake separate tray
open/close events.

So my question is just if the monitor/block layer shouldn't really be
responsible for opening the tray before they change the medium and close
the tray again. This doesn't look device-specific to me at all.

But even though I think that it would be conceptionally cleaner to do it
this way, it would probably be a bad idea as faking the states requires
knowledge of when the guest has read out the intermediate status. and
only devices have this knowledge.

Kevin

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

[Markus Armbruster]

Kevin Wolf <kwolf@redhat.com> writes:

> Am 17.02.2012 12:48, schrieb Markus Armbruster:
>> Kevin Wolf <kwolf@redhat.com> writes:
>> 
>>> Am 17.02.2012 11:32, schrieb Paolo Bonzini:
>>>> On 02/16/2012 03:10 PM, Luiz Capitulino wrote:
>>>>> 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.
>>>>
>>>> The guest is seeing each step separately, but that is managed by the
>>>> device model (which sends two separate error codes: a NOT READY for
>>>> opening the tray, and a UNIT ATTENTION for closing the tray and changing
>>>> medium) rather than by the monitor.
>>>
>>> But this separation isn't set in stone. Maybe it would make sense to
>>> move the logic into the block layer. Of course, then it would have to
>>> meet the needs of floppies as well.
>> 
>> And whatever other device with removable media comes up.
>> 
>> We should not move device-specific stuff from device models into the
>> block layer.  The block layer is fat enough as it is.  And it's not
>> meant for sharing code among device models.  There are better ways to do
>> that.
>
> My motivation is not sharing code but getting the model right.
>
> As Paolo says, currently we only tell the device that the medium has
> changed. It is the device's responsibility to fake separate tray
> open/close events.
>
> So my question is just if the monitor/block layer shouldn't really be
> responsible for opening the tray before they change the medium and close
> the tray again. This doesn't look device-specific to me at all.

Looks like I misunderstood you :)

I think BlockDevOps method(s) to move the tray are okay.  If you want
more generality there, call it "prepare for media change" (called right
before media goes out) and "media changed" (called right after media
went in).

> But even though I think that it would be conceptionally cleaner to do it
> this way, it would probably be a bad idea as faking the states requires
> knowledge of when the guest has read out the intermediate status. and
> only devices have this knowledge.

I'm not sure I got the last paragraph.

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

[Markus Armbruster]

Kevin Wolf <kwolf@redhat.com> writes:

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

Agreed.

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

Agreed again.

>                     We're probably taking shortcuts so that this
> function really sees a closed -> closed transition when we really have
> closed -> open -> closed.

Let's figure out how this stuff really works.

1. Guest load/eject

Guest commands load or eject.  Device model updates its state of virtual
tray (e.g. IDEState member tray_open) unless locked, then calls
bdrv_eject().

bdrv_eject() is a no-op except for pass-through backends such as
host_cdrom.

Note: device models call bdrv_eject() whether the state changed or not.
The only use for that I can see is syncing a wayward physical tray to
the virtual one.  Shouldn't be necessary with Paolo's recent work,
should it?

Luiz's patch adds event emission to bdrv_eject().  Makes sense, except
when it's called even though the tray didn't move.  Patch adds a
parameter to suppress the event then.  I'd prefer to suppress the call
entirely then, if that's possible.

2. Monitor commands

2a. eject

run bdrv_close() if eject is permitted right now (not in use by
long-running block operations such as block migration and image
streaming, and force or not locked).

bdrv_close(bs) calls bdrv_dev_change_media_cb(bs, false).

bdrv_dev_change_media_cb() tells the device model the media went away.
Device model responds by opening its virtual tray, unless it's already
open.

Luiz's patch adds event emission to bdrv_dev_change_media_cb().  It
emits an "open" event when the tray is closed before telling the device
model.  We assume the device model always opens the tray then, so this
emits the event only when the tray actually moves.  A bit subtle, but
works.

Wart: bdrv_close(bs) is a no-op if bs isn't open.  "bs not open"
represents "no media".  Therefore, eject without media does nothing.  In
particular, the virtual tray doesn't move, and no event is emitted.
Works.

2b. change

First do 2a. eject.  If we had media, the virtual tray is now open.
Else, it could be open or closed, because of the wart.

Then open a new block backend.  bdrv_dev_change_media_cb(bs, true) gets
called either right away by bdrv_open(), or later by bdrv_set_key().

bdrv_dev_change_media_cb() tells the device model new media appeared.
Device model responds by closing its virtual tray, unless it's already
closed[*].

Luiz's patch adds event emission to bdrv_dev_change_media_cb().  It
emits an "open" event when the tray is closed before telling the device
model, and a "closed" event unconditionally.  As above, subtle, but
works,

3. Physical load/eject with pass-through device

The host_cdrom backend's tray handling is rudimentary.  But let me
sketch how a real implementation could work.

3a. User closes tray, or opens unlocked tray

Backend gets notified, calls into block layer.  Block layer notifies
device model.  Device model notifies guest OS.

3b. User presses button while tray is closed and locked

Backend gets notified, calls into block layer.  Block layer notifies
device model.  Device model notifies guest OS.  Guest OS may command
tray open.  Goto 1.

We should be able to emit suitable events in the block layer.

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

Your suggestion makes sense to me.

I figure Luiz's patch works.  But maybe it could be simplified some by
replacing bdrv_dev_change_media_cb() by a "open/close tray" callback
that returns whether it moved.  bdrv_dev_change_media_cb() would then
simply open the tray, emit event if it moved, close the tray, emit event
if it moved.

I believe that should also do fine for my case 3.


[*] Can happen only in the warty case.  Or when the guest closes the
tray before us.  I'm not sure that works.  The device model sees media
(because bs is open) even though we don't have the key, yet.  No idea
what happens when it reads or writes it.

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

[Luiz Capitulino]

On Thu, 16 Feb 2012 19:30:13 +0100
Markus Armbruster <armbru@redhat.com> wrote:

> >                     We're probably taking shortcuts so that this
> > function really sees a closed -> closed transition when we really have
> > closed -> open -> closed.
> 
> Let's figure out how this stuff really works.
> 
> 1. Guest load/eject
> 
> Guest commands load or eject.  Device model updates its state of virtual
> tray (e.g. IDEState member tray_open) unless locked, then calls
> bdrv_eject().
> 
> bdrv_eject() is a no-op except for pass-through backends such as
> host_cdrom.
> 
> Note: device models call bdrv_eject() whether the state changed or not.
> The only use for that I can see is syncing a wayward physical tray to
> the virtual one.  Shouldn't be necessary with Paolo's recent work,
> should it?
> 
> Luiz's patch adds event emission to bdrv_eject().  Makes sense, except
> when it's called even though the tray didn't move.  Patch adds a
> parameter to suppress the event then.  I'd prefer to suppress the call
> entirely then, if that's possible.

I can do that. I can't also think of any problem that could arise from
that, except that we will be deviating slightly on how this works in
non-virtual life when using passthrough (ie. all ejects reach the driver and
possibly the drive).

Again, either way is fine with me. I just tried to preserve what the current
code does.

> 2. Monitor commands
> 
> 2a. eject
> 
> run bdrv_close() if eject is permitted right now (not in use by
> long-running block operations such as block migration and image
> streaming, and force or not locked).
> 
> bdrv_close(bs) calls bdrv_dev_change_media_cb(bs, false).
> 
> bdrv_dev_change_media_cb() tells the device model the media went away.
> Device model responds by opening its virtual tray, unless it's already
> open.
> 
> Luiz's patch adds event emission to bdrv_dev_change_media_cb().  It
> emits an "open" event when the tray is closed before telling the device
> model. 

Both events are called after dev_ops->change_media_cb() is called, so this
is not true (unless I got you wrong here).

> We assume the device model always opens the tray then, so this
> emits the event only when the tray actually moves.  A bit subtle, but
> works.

Yes, having QMP events in the block layer this way ends up being subtle. That's
why I wanted to have only GUEST_MEDIUM_EJECT, this would allow us to cut
a lot on the subtle factor...

Now, one way of improving this would be to add an interface to the block layer
which allows for other subsystems to register for block layer events. Then we
should trigger those events in well specified block layer "actions", like
"tray moved" or "a block I/O error happened".

> Wart: bdrv_close(bs) is a no-op if bs isn't open.  "bs not open"
> represents "no media".  Therefore, eject without media does nothing.  In
> particular, the virtual tray doesn't move, and no event is emitted.
> Works.
> 
> 2b. change
> 
> First do 2a. eject.  If we had media, the virtual tray is now open.
> Else, it could be open or closed, because of the wart.
> 
> Then open a new block backend.  bdrv_dev_change_media_cb(bs, true) gets
> called either right away by bdrv_open(), or later by bdrv_set_key().
> 
> bdrv_dev_change_media_cb() tells the device model new media appeared.
> Device model responds by closing its virtual tray, unless it's already
> closed[*].
> 
> Luiz's patch adds event emission to bdrv_dev_change_media_cb().  It
> emits an "open" event when the tray is closed before telling the device
> model, and a "closed" event unconditionally.  As above, subtle, but
> works,
> 
> 3. Physical load/eject with pass-through device
> 
> The host_cdrom backend's tray handling is rudimentary.  But let me
> sketch how a real implementation could work.
> 
> 3a. User closes tray, or opens unlocked tray
> 
> Backend gets notified, calls into block layer.  Block layer notifies
> device model.  Device model notifies guest OS.
> 
> 3b. User presses button while tray is closed and locked
> 
> Backend gets notified, calls into block layer.  Block layer notifies
> device model.  Device model notifies guest OS.  Guest OS may command
> tray open.  Goto 1.
> 
> We should be able to emit suitable events in the block layer.
> 
> > 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.
> 
> Your suggestion makes sense to me.
> 
> I figure Luiz's patch works.  But maybe it could be simplified some by
> replacing bdrv_dev_change_media_cb() by a "open/close tray" callback
> that returns whether it moved.  bdrv_dev_change_media_cb() would then
> simply open the tray, emit event if it moved, close the tray, emit event
> if it moved.

I can work on this refactoring, but I'd like to defer it for now.

> 
> I believe that should also do fine for my case 3.
> 
> 
> [*] Can happen only in the warty case.  Or when the guest closes the
> tray before us.  I'm not sure that works.  The device model sees media
> (because bs is open) even though we don't have the key, yet.  No idea
> what happens when it reads or writes it.
> 

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

[Kevin Wolf]

Am 16.02.2012 19:30, schrieb Markus Armbruster:
> Let's figure out how this stuff really works.
> 
> 1. Guest load/eject
> 
> Guest commands load or eject.  Device model updates its state of virtual
> tray (e.g. IDEState member tray_open) unless locked, then calls
> bdrv_eject().
> 
> bdrv_eject() is a no-op except for pass-through backends such as
> host_cdrom.
> 
> Note: device models call bdrv_eject() whether the state changed or not.
> The only use for that I can see is syncing a wayward physical tray to
> the virtual one.  Shouldn't be necessary with Paolo's recent work,
> should it?

The one case that I'm unsure about is migration. I thought we sync the
physical tray with the virtual one then (or at least we discussed it),
but I don't really know what we do today, or even what the right thing
to do is in some cases.

> I figure Luiz's patch works.  But maybe it could be simplified some by
> replacing bdrv_dev_change_media_cb() by a "open/close tray" callback
> that returns whether it moved.  bdrv_dev_change_media_cb() would then
> simply open the tray, emit event if it moved, close the tray, emit event
> if it moved.

Yes, I had the same impression after reading the first few paragraphs of
your analysis (which looks right to me).

Kevin

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

[Paolo Bonzini]

On 02/17/2012 10:53 AM, Kevin Wolf wrote:
> > Note: device models call bdrv_eject() whether the state changed or not.
> > The only use for that I can see is syncing a wayward physical tray to
> > the virtual one.  Shouldn't be necessary with Paolo's recent work,
> > should it?
> 
> The one case that I'm unsure about is migration. I thought we sync the
> physical tray with the virtual one then (or at least we discussed it),
> but I don't really know what we do today, or even what the right thing
> to do is in some cases.

Migration with passthrough doesn't make sense.  The only way it could
work (untested) is if the destination connected via iSCSI to the
source's CD-ROM.  In which case the physical and virtual tray would be
synced already.

Paolo

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

[Markus Armbruster]

Paolo Bonzini <pbonzini@redhat.com> writes:

> On 02/17/2012 10:53 AM, Kevin Wolf wrote:
>> > Note: device models call bdrv_eject() whether the state changed or not.
>> > The only use for that I can see is syncing a wayward physical tray to
>> > the virtual one.  Shouldn't be necessary with Paolo's recent work,
>> > should it?
>> 
>> The one case that I'm unsure about is migration. I thought we sync the
>> physical tray with the virtual one then (or at least we discussed it),
>> but I don't really know what we do today, or even what the right thing
>> to do is in some cases.
>
> Migration with passthrough doesn't make sense.  The only way it could
> work (untested) is if the destination connected via iSCSI to the
> source's CD-ROM.  In which case the physical and virtual tray would be
> synced already.

Maybe we should prevent migration with CD-ROM pass-through just like we
do for PCI pass-through.

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

[Markus Armbruster]

Kevin Wolf <kwolf@redhat.com> writes:

> Am 16.02.2012 19:30, schrieb Markus Armbruster:
>> Let's figure out how this stuff really works.
>> 
>> 1. Guest load/eject
>> 
>> Guest commands load or eject.  Device model updates its state of virtual
>> tray (e.g. IDEState member tray_open) unless locked, then calls
>> bdrv_eject().
>> 
>> bdrv_eject() is a no-op except for pass-through backends such as
>> host_cdrom.
>> 
>> Note: device models call bdrv_eject() whether the state changed or not.
>> The only use for that I can see is syncing a wayward physical tray to
>> the virtual one.  Shouldn't be necessary with Paolo's recent work,
>> should it?
>
> The one case that I'm unsure about is migration. I thought we sync the
> physical tray with the virtual one then (or at least we discussed it),
> but I don't really know what we do today, or even what the right thing
> to do is in some cases.

I looked into this back when I moved tray stuff into device models.  I
believe it needs fixing, but my patch was flawed[*], so I shelved it.

Whether migration makes sense while the host CD-ROM is passed through is
a valid question.

>> I figure Luiz's patch works.  But maybe it could be simplified some by
>> replacing bdrv_dev_change_media_cb() by a "open/close tray" callback
>> that returns whether it moved.  bdrv_dev_change_media_cb() would then
>> simply open the tray, emit event if it moved, close the tray, emit event
>> if it moved.
>
> Yes, I had the same impression after reading the first few paragraphs of
> your analysis (which looks right to me).

If I understand Luiz correctly, he'd like to do that in a follow-up
patch.


[*] http://lists.nongnu.org/archive/html/qemu-devel/2011-09/msg00324.html

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

[Luiz Capitulino]

On Fri, 17 Feb 2012 12:44:28 +0100
Markus Armbruster <armbru@redhat.com> wrote:

> >> I figure Luiz's patch works.  But maybe it could be simplified some by
> >> replacing bdrv_dev_change_media_cb() by a "open/close tray" callback
> >> that returns whether it moved.  bdrv_dev_change_media_cb() would then
> >> simply open the tray, emit event if it moved, close the tray, emit event
> >> if it moved.
> >
> > Yes, I had the same impression after reading the first few paragraphs of
> > your analysis (which looks right to me).
> 
> If I understand Luiz correctly, he'd like to do that in a follow-up
> patch.

Yes, but as I said I'm not completely sure what's the best thing to do here,
and this may take some time, but I'll do it.

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

[Paolo Bonzini]

On 02/16/2012 07:30 PM, Markus Armbruster wrote:
> 3. Physical load/eject with pass-through device
> 
> The host_cdrom backend's tray handling is rudimentary.  But let me
> sketch how a real implementation could work.
> 
> 3a. User closes tray, or opens unlocked tray
> 
> Backend gets notified, calls into block layer.  Block layer notifies
> device model.  Device model notifies guest OS.
> 
> 3b. User presses button while tray is closed and locked
> 
> Backend gets notified, calls into block layer.  Block layer notifies
> device model.  Device model notifies guest OS.  Guest OS may command
> tray open.  Goto 1.
> 
> We should be able to emit suitable events in the block layer.

Here is how I had implemented it; it is similar enough:

3a. User closes tray, or opens unlocked tray

The device model polls the block layer that in turn asks the backend.
Device model notifies guest OS.

The block layer could look at the results of polling before passing them
to the device model and emit events.

3b. User presses button while tray is closed and locked

If the guest doesn't poll the device model, there is nothing to do.

If the guest polls the device model, the device model passes the request
to the block layer that in turn asks the backend.  But this doesn't
generate any event unless the guest OS commands tray open.  If it does,
goto 1.

> I figure Luiz's patch works.  But maybe it could be simplified some by
> replacing bdrv_dev_change_media_cb() by a "open/close tray" callback
> that returns whether it moved.  bdrv_dev_change_media_cb() would then
> simply open the tray, emit event if it moved, close the tray, emit event
> if it moved.

The device models' change_media_cb is already effectively an open/close
tray callback.  You do not need a check in the callback, you can simply
call the is_tray_open callback.  bdrv_dev_change_media_cb can stop
calling the callback unless the tray moved, and
eject_device+qmp_bdrv_open_encrypted can call bdrv_dev_change_media_cb
respectively to open and close the tray.  This would also fix the wart
with no medium, IIUC.

> [*] Can happen only in the warty case.  Or when the guest closes the
> tray before us.  I'm not sure that works.

The guest cannot beat us to this race because the monitor and MMIO are
both protected by the global mutex.

Paolo

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

[Markus Armbruster]

Paolo Bonzini <pbonzini@redhat.com> writes:

> On 02/16/2012 07:30 PM, Markus Armbruster wrote:
>> 3. Physical load/eject with pass-through device
>> 
>> The host_cdrom backend's tray handling is rudimentary.  But let me
>> sketch how a real implementation could work.
>> 
>> 3a. User closes tray, or opens unlocked tray
>> 
>> Backend gets notified, calls into block layer.  Block layer notifies
>> device model.  Device model notifies guest OS.
>> 
>> 3b. User presses button while tray is closed and locked
>> 
>> Backend gets notified, calls into block layer.  Block layer notifies
>> device model.  Device model notifies guest OS.  Guest OS may command
>> tray open.  Goto 1.
>> 
>> We should be able to emit suitable events in the block layer.
>
> Here is how I had implemented it; it is similar enough:
>
> 3a. User closes tray, or opens unlocked tray
>
> The device model polls the block layer that in turn asks the backend.
> Device model notifies guest OS.
>
> The block layer could look at the results of polling before passing them
> to the device model and emit events.
>
> 3b. User presses button while tray is closed and locked
>
> If the guest doesn't poll the device model, there is nothing to do.
>
> If the guest polls the device model, the device model passes the request
> to the block layer that in turn asks the backend.  But this doesn't
> generate any event unless the guest OS commands tray open.  If it does,
> goto 1.

Yes, that's just active polling instead of passive event reception,
i.e. similar enough.

For device models that can signal the guest (usually IRQ), polling
introduces latency, though.  Polling frequently enough to hide the
latency can waste a bit of power.  Anyway, no need to worry about it as
long as it doesn't bother users.

>> I figure Luiz's patch works.  But maybe it could be simplified some by
>> replacing bdrv_dev_change_media_cb() by a "open/close tray" callback
>> that returns whether it moved.  bdrv_dev_change_media_cb() would then
>> simply open the tray, emit event if it moved, close the tray, emit event
>> if it moved.
>
> The device models' change_media_cb is already effectively an open/close
> tray callback.  You do not need a check in the callback, you can simply
> call the is_tray_open callback.  bdrv_dev_change_media_cb can stop
> calling the callback unless the tray moved, and
> eject_device+qmp_bdrv_open_encrypted can call bdrv_dev_change_media_cb
> respectively to open and close the tray.  This would also fix the wart
> with no medium, IIUC.

I'd like to see this change, too.  Follow-up patch would be fine with
me.

>> [*] Can happen only in the warty case.  Or when the guest closes the
>> tray before us.  I'm not sure that works.
>
> The guest cannot beat us to this race because the monitor and MMIO are
> both protected by the global mutex.

Good to know, thanks!

Re: [Qemu-devel] [PATCH v2 0/4]: QMP: add BLOCK_MEDIUM_EJECT event

[Paolo Bonzini]

On 02/15/2012 07:42 PM, Luiz Capitulino wrote:
> There's been one non-rfc patch and one or two rfc ones. This is v2 of the
> non-rfc one.
> 
> I think this version does what Kevin and Markus were asking: the event is
> emitted whenever the tray moves, be it the guest or HMP/QMP commands.
> 
> In a previous email I said that I'd be reviving an old series that breaks the
> eject and change commands into multiple tray commands (tray-open/close,
> medium-insert/remove etc), but that's not so trivial anymore as the tray state
> moved to device models (still possible of course, but requires a bit more
> work). So I decided to do it the way patch 4/4 does it.
> 
>  QMP/qmp-events.txt |   17 ++++++++++
>  block.c            |   84 +++++++++++++++++++++++++++++++++------------------
>  block.h            |    8 ++--
>  block/raw-posix.c  |    6 ++--
>  block/raw.c        |    4 +-
>  block_int.h        |    2 +-
>  hw/ide/atapi.c     |    2 +-
>  hw/ide/core.c      |    8 ++--
>  hw/scsi-disk.c     |    8 ++--
>  hw/virtio-blk.c    |    6 ++--
>  monitor.c          |    3 ++
>  monitor.h          |    1 +
>  12 files changed, 97 insertions(+), 52 deletions(-)

We can seek perfection, but we can also do that incrementally.  We're
discussing the code more than the actual behavior, so

Acked-by: Paolo Bonzini <pbonzini@redhat.com>

Paolo

Re: [Qemu-devel] [PATCH v2 0/4]: QMP: add BLOCK_MEDIUM_EJECT event

[Luiz Capitulino]

On Fri, 17 Feb 2012 11:49:21 +0100
Paolo Bonzini <pbonzini@redhat.com> wrote:

> On 02/15/2012 07:42 PM, Luiz Capitulino wrote:
> > There's been one non-rfc patch and one or two rfc ones. This is v2 of the
> > non-rfc one.
> > 
> > I think this version does what Kevin and Markus were asking: the event is
> > emitted whenever the tray moves, be it the guest or HMP/QMP commands.
> > 
> > In a previous email I said that I'd be reviving an old series that breaks the
> > eject and change commands into multiple tray commands (tray-open/close,
> > medium-insert/remove etc), but that's not so trivial anymore as the tray state
> > moved to device models (still possible of course, but requires a bit more
> > work). So I decided to do it the way patch 4/4 does it.
> > 
> >  QMP/qmp-events.txt |   17 ++++++++++
> >  block.c            |   84 +++++++++++++++++++++++++++++++++------------------
> >  block.h            |    8 ++--
> >  block/raw-posix.c  |    6 ++--
> >  block/raw.c        |    4 +-
> >  block_int.h        |    2 +-
> >  hw/ide/atapi.c     |    2 +-
> >  hw/ide/core.c      |    8 ++--
> >  hw/scsi-disk.c     |    8 ++--
> >  hw/virtio-blk.c    |    6 ++--
> >  monitor.c          |    3 ++
> >  monitor.h          |    1 +
> >  12 files changed, 97 insertions(+), 52 deletions(-)
> 
> We can seek perfection, but we can also do that incrementally.  We're
> discussing the code more than the actual behavior, so
> 
> Acked-by: Paolo Bonzini <pbonzini@redhat.com>

I'll submit v3 with a few renames and the change proposed by Markus, which is
not to call bdrv_eject() in the device models if the tray didn't move.