[Qemu-devel] [RFC 0/5]: QMP: Introduce GUEST_MEDIUM_EJECT & BLOCK_MEDIUM_CHANGED

[Qemu-devel] [RFC 0/5]: QMP: Introduce GUEST_MEDIUM_EJECT & BLOCK_MEDIUM_CHANGED

[Luiz Capitulino]

I've tried to implement a BLOCK_MEDIUM_EJECT event that, as we discussed[1],
would be emitted by guest-initiated ejects and by the QMP/HMP eject and change
commands.

However, that turned to be a bit problematic, because the eject and change
commands don't exactly handle tray movements: they actually insert/purge a
medium from from the drive.

Consider this example: you have a medium inserted and locked; a first eject
from HMP will tell the guest to eject the medium; if the guest does eject, a
second eject from HMP will just purge the medium (in which case
BLOCK_MEDIUM_EJECT is a bad event to be emitted).

What we really want to do is to tell mngt that the medium was purged.

The same is valid for the change command: we want to inform mngt if a medium
was inserted or purged and not emulate tray movements with two eject events
as we discussed[1].

So, the solution I came up with is to have two events:

 o GUEST_MEDIUM_EJECTED: emitted when the tray state is changed by the guest
 o BLOCK_MEDIUM_CHANGED: emitted when there's a medium change. This should
   happen when the eject and change QMP/HMP commands are used

 QMP/qmp-events.txt |   38 ++++++++++++++++++++++++++++++++++++++
 block.c            |   33 ++++++++++++++++++++++++++++++---
 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          |    6 ++++++
 monitor.h          |    2 ++
 12 files changed, 98 insertions(+), 25 deletions(-)

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

[Luiz Capitulino]

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

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

diff --git a/block.c b/block.c
index 3621d11..160b9de 100644
--- a/block.c
+++ b/block.c
@@ -2244,8 +2244,8 @@ 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)
+void bdrv_emit_qmp_error_event(const BlockDriverState *bdrv,
+                               BlockQMPEventAction action, int is_read)
 {
     QObject *data;
     const char *action_str;
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/5] 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 160b9de..4042ada 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/5] 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 4042ada..77ace49 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/5] qmp: add the GUEST_MEDIUM_EJECTED event

[Luiz Capitulino]

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 QMP/qmp-events.txt |   21 +++++++++++++++++++++
 block.c            |   15 +++++++++++++++
 monitor.c          |    3 +++
 monitor.h          |    1 +
 4 files changed, 40 insertions(+), 0 deletions(-)

diff --git a/QMP/qmp-events.txt b/QMP/qmp-events.txt
index 06cb404..ab32e62 100644
--- a/QMP/qmp-events.txt
+++ b/QMP/qmp-events.txt
@@ -26,6 +26,27 @@ Example:
 Note: If action is "stop", a STOP event will eventually follow the
 BLOCK_IO_ERROR event.
 
+GUEST_MEDIUM_EJECTED
+--------------------
+
+Emitted when the guest succeeds ejecting a removable medium. If the drive
+has a tray, this is only emitted if the tray opens or closes.
+
+Data:
+
+- "device": device name (json-string)
+- "ejected": true if the tray has been opened or false if it has been closed
+
+Example:
+
+{ "event": "GUEST_MEDIUM_EJECTED",
+    "data": { "device": "ide1-cd1",
+              "ejected": true },
+    "timestamp": { "seconds": 1265044230, "microseconds": 450486 } }
+
+Note: This event does not track all tray changes, as QMP/HMP commands like
+'eject' can also change the tray.
+
 RESET
 -----
 
diff --git a/block.c b/block.c
index 77ace49..7f5b56d 100644
--- a/block.c
+++ b/block.c
@@ -2244,6 +2244,17 @@ int bdrv_is_allocated(BlockDriverState *bs, int64_t sector_num, int nb_sectors,
     return data.ret;
 }
 
+static void bdrv_emit_qmp_eject_event(BlockDriverState *bs, bool is_ejected)
+{
+    QObject *data;
+
+    data = qobject_from_jsonf("{ 'device': %s, 'ejected': %i }",
+                              bdrv_get_device_name(bs), is_ejected);
+    monitor_protocol_event(QEVENT_GUEST_MEDIUM_EJECTED, data);
+
+    qobject_decref(data);
+}
+
 void bdrv_emit_qmp_error_event(const BlockDriverState *bdrv,
                                BlockQMPEventAction action, int is_read)
 {
@@ -3567,6 +3578,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 && bs->device_name[0]) {
+        bdrv_emit_qmp_eject_event(bs, eject_flag);
+    }
 }
 
 /**
diff --git a/monitor.c b/monitor.c
index aadbdcb..48b1dea 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_GUEST_MEDIUM_EJECTED:
+            event_name = "GUEST_MEDIUM_EJECTED";
+            break;
         default:
             abort();
             break;
diff --git a/monitor.h b/monitor.h
index b72ea07..06be7bb 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_GUEST_MEDIUM_EJECTED,
     QEVENT_MAX,
 } MonitorEvent;
 
-- 
1.7.9.111.gf3fb0.dirty

[Qemu-devel] [PATCH 5/5] qmp: add the BLOCK_MEDIUM_CHANGED event

[Luiz Capitulino]

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 QMP/qmp-events.txt |   17 +++++++++++++++++
 block.c            |   12 ++++++++++++
 monitor.c          |    3 +++
 monitor.h          |    1 +
 4 files changed, 33 insertions(+), 0 deletions(-)

diff --git a/QMP/qmp-events.txt b/QMP/qmp-events.txt
index ab32e62..a1815b5 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_CHANGED
+--------------------
+
+Emitted when a medium is either inserted or removed from a drive. This should
+happen as a result of the 'eject' and 'change' commands.
+
+Data:
+
+- "device": device name (json-string)
+- "medium-inserted": true if a medium has been inserted, false if it has been
+                     removed from the drive
+
+{ "event": "BLOCK_MEDIUM_CHANGED",
+    "data": { "device": "ide1-cd1",
+              "medium-inserted": true },
+    "timestamp": { "seconds": 1265044230, "microseconds": 450486 } }
+
 GUEST_MEDIUM_EJECTED
 --------------------
 
diff --git a/block.c b/block.c
index 7f5b56d..1d40f82 100644
--- a/block.c
+++ b/block.c
@@ -941,10 +941,22 @@ void bdrv_set_dev_ops(BlockDriverState *bs, const BlockDevOps *ops,
     }
 }
 
+static void bdrv_emit_qmp_medium_changed_event(BlockDriverState *bs, bool load)
+{
+    QObject *data;
+
+    data = qobject_from_jsonf("{ 'device': %s, 'medium-inserted': %i }",
+                              bdrv_get_device_name(bs), load);
+    monitor_protocol_event(QEVENT_BLOCK_MEDIUM_CHANGED, 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) {
         bs->dev_ops->change_media_cb(bs->dev_opaque, load);
+        bdrv_emit_qmp_medium_changed_event(bs, load);
     }
 }
 
diff --git a/monitor.c b/monitor.c
index 48b1dea..8701ec1 100644
--- a/monitor.c
+++ b/monitor.c
@@ -488,6 +488,9 @@ void monitor_protocol_event(MonitorEvent event, QObject *data)
         case QEVENT_GUEST_MEDIUM_EJECTED:
             event_name = "GUEST_MEDIUM_EJECTED";
             break;
+        case QEVENT_BLOCK_MEDIUM_CHANGED:
+            event_name = "BLOCK_MEDIUM_CHANGED";
+            break;
         default:
             abort();
             break;
diff --git a/monitor.h b/monitor.h
index 06be7bb..4f9e6f7 100644
--- a/monitor.h
+++ b/monitor.h
@@ -39,6 +39,7 @@ typedef enum MonitorEvent {
     QEVENT_BLOCK_JOB_COMPLETED,
     QEVENT_BLOCK_JOB_CANCELLED,
     QEVENT_GUEST_MEDIUM_EJECTED,
+    QEVENT_BLOCK_MEDIUM_CHANGED,
     QEVENT_MAX,
 } MonitorEvent;
 
-- 
1.7.9.111.gf3fb0.dirty

Re: [Qemu-devel] [RFC 0/5]: QMP: Introduce GUEST_MEDIUM_EJECT & BLOCK_MEDIUM_CHANGED

[Markus Armbruster]

Luiz Capitulino <lcapitulino@redhat.com> writes:

> I've tried to implement a BLOCK_MEDIUM_EJECT event that, as we discussed[1],
> would be emitted by guest-initiated ejects and by the QMP/HMP eject and change
> commands.
>
> However, that turned to be a bit problematic, because the eject and change
> commands don't exactly handle tray movements: they actually insert/purge a
> medium from from the drive.

The monitor commands are complex, because they do several things, and
the exact things they do depends on circumstances.  In my experience,
it's best to think in basic operations until you understand the problem,
then bring in the complex commands.  If you bring them in any earlier,
you'll get lost in detail and confused.

> Consider this example: you have a medium inserted and locked; a first eject
> from HMP will tell the guest to eject the medium; if the guest does eject, a
> second eject from HMP will just purge the medium (in which case
> BLOCK_MEDIUM_EJECT is a bad event to be emitted).
>
> What we really want to do is to tell mngt that the medium was purged.
>
> The same is valid for the change command: we want to inform mngt if a medium
> was inserted or purged and not emulate tray movements with two eject events
> as we discussed[1].
>
> So, the solution I came up with is to have two events:
>
>  o GUEST_MEDIUM_EJECTED: emitted when the tray state is changed by the guest
>  o BLOCK_MEDIUM_CHANGED: emitted when there's a medium change. This should
>    happen when the eject and change QMP/HMP commands are used

I think what I got in mind is close to your proposal, but the thinking
that gets me there is different.  Let me explain it.

The tray can be modelled as a simple state machine.  Our problem "notify
management app of tray-related events" then becomes "notify on state
transition".

Tray state is just three bits: open/closed, locked/unlocked,
medium/empty.  A state transition changes one bit, i.e. we have three
pairs of them.  There are a few constraints, all obvious: closed -> open
requires unlocked, and empty <-> medium require open.

The three pairs of state transitions correspond to three pairs of QMP
events, each with a boolean argument[*]: one for open <-> closed, one for
locked <-> unlocked, and one for medium <-> empty.

These events report tray state transitions fully by construction.
That's a feature.

The medium/empty bit belongs to the block layer.  So the block layer
should emit the corresponding event.

The open/closed bit and the locked/unlocked bit belong to the device
model.  So the device model should emit the event when it changes the
bit.  When device models need to call into the block layer whenever they
change such a bit, then its fine (even desirable) to put the even
emission into the block layer.

Note there is no mention whatsoever of what caused the state transition.
That's a feature.


Now let's compare your proposal to my ideas.  Your BLOCK_MEDIUM_CHANGED
appears to track my medium <-> empty.  You emit it from the block
layer's bdrv_dev_change_media_cb().  Called to notify device models
whenever the block layer changes media.  The "whenever it changes media"
part makes it a convenient choke point.

Your GUEST_MEDIUM_EJECTED does *not* track my open <-> closed.  I think
it's more complex than a straight open <-> closed event.  Evidence: your
event documentation in qmp-events.txt needs an extra note to clarify
when exactly the event is emitted.

You don't have an event for my locked <-> unlocked, but that's fine.  We
can always start with a subset of the known-complete set of events,
because moving from there to the complete set only involves adding
events (easy), not changing events (messy).


This is just my analysis of the problem.  If folks think your proposal
serves their needs, and Kevin is happy with it, I won't object.


[*] Or six events without arguments, I don't care.

Re: [Qemu-devel] [RFC 0/5]: QMP: Introduce GUEST_MEDIUM_EJECT & BLOCK_MEDIUM_CHANGED

[Luiz Capitulino]

On Thu, 09 Feb 2012 16:01:21 +0100
Markus Armbruster <armbru@redhat.com> wrote:

> Luiz Capitulino <lcapitulino@redhat.com> writes:
> 
> > I've tried to implement a BLOCK_MEDIUM_EJECT event that, as we discussed[1],
> > would be emitted by guest-initiated ejects and by the QMP/HMP eject and change
> > commands.
> >
> > However, that turned to be a bit problematic, because the eject and change
> > commands don't exactly handle tray movements: they actually insert/purge a
> > medium from from the drive.
> 
> The monitor commands are complex, because they do several things, and
> the exact things they do depends on circumstances.  In my experience,
> it's best to think in basic operations until you understand the problem,
> then bring in the complex commands.  If you bring them in any earlier,
> you'll get lost in detail and confused.
> 
> > Consider this example: you have a medium inserted and locked; a first eject
> > from HMP will tell the guest to eject the medium; if the guest does eject, a
> > second eject from HMP will just purge the medium (in which case
> > BLOCK_MEDIUM_EJECT is a bad event to be emitted).
> >
> > What we really want to do is to tell mngt that the medium was purged.
> >
> > The same is valid for the change command: we want to inform mngt if a medium
> > was inserted or purged and not emulate tray movements with two eject events
> > as we discussed[1].
> >
> > So, the solution I came up with is to have two events:
> >
> >  o GUEST_MEDIUM_EJECTED: emitted when the tray state is changed by the guest
> >  o BLOCK_MEDIUM_CHANGED: emitted when there's a medium change. This should
> >    happen when the eject and change QMP/HMP commands are used
> 
> I think what I got in mind is close to your proposal, but the thinking
> that gets me there is different.  Let me explain it.
> 
> The tray can be modelled as a simple state machine.  Our problem "notify
> management app of tray-related events" then becomes "notify on state
> transition".
> 
> Tray state is just three bits: open/closed, locked/unlocked,
> medium/empty.  A state transition changes one bit, i.e. we have three
> pairs of them.  There are a few constraints, all obvious: closed -> open
> requires unlocked, and empty <-> medium require open.
> 
> The three pairs of state transitions correspond to three pairs of QMP
> events, each with a boolean argument[*]: one for open <-> closed, one for
> locked <-> unlocked, and one for medium <-> empty.
> 
> These events report tray state transitions fully by construction.
> That's a feature.
> 
> The medium/empty bit belongs to the block layer.  So the block layer
> should emit the corresponding event.
> 
> The open/closed bit and the locked/unlocked bit belong to the device
> model.  So the device model should emit the event when it changes the
> bit.  When device models need to call into the block layer whenever they
> change such a bit, then its fine (even desirable) to put the even
> emission into the block layer.
> 
> Note there is no mention whatsoever of what caused the state transition.
> That's a feature.
> 
> 
> Now let's compare your proposal to my ideas.  Your BLOCK_MEDIUM_CHANGED
> appears to track my medium <-> empty.  You emit it from the block
> layer's bdrv_dev_change_media_cb().  Called to notify device models
> whenever the block layer changes media.  The "whenever it changes media"
> part makes it a convenient choke point.
> 
> Your GUEST_MEDIUM_EJECTED does *not* track my open <-> closed.  I think
> it's more complex than a straight open <-> closed event.  Evidence: your
> event documentation in qmp-events.txt needs an extra note to clarify
> when exactly the event is emitted.

The purpose of the note is not to clarify, but to emphasize that the event
is about guest initiated ejects. Also, I disagree it's more complex, actually
I think it's quite simple vs. emitting the eject event from the eject and
change commands, as this can be messy because of their complicated semantics.

> This is just my analysis of the problem.  If folks think your proposal
> serves their needs, and Kevin is happy with it, I won't object.

No feedback so far.

Re: [Qemu-devel] [RFC 0/5]: QMP: Introduce GUEST_MEDIUM_EJECT & BLOCK_MEDIUM_CHANGED

[Markus Armbruster]

Luiz Capitulino <lcapitulino@redhat.com> writes:

> On Thu, 09 Feb 2012 16:01:21 +0100
> Markus Armbruster <armbru@redhat.com> wrote:
>
>> Luiz Capitulino <lcapitulino@redhat.com> writes:
>> 
>> > I've tried to implement a BLOCK_MEDIUM_EJECT event that, as we discussed[1],
>> > would be emitted by guest-initiated ejects and by the QMP/HMP eject and change
>> > commands.
>> >
>> > However, that turned to be a bit problematic, because the eject and change
>> > commands don't exactly handle tray movements: they actually insert/purge a
>> > medium from from the drive.
>> 
>> The monitor commands are complex, because they do several things, and
>> the exact things they do depends on circumstances.  In my experience,
>> it's best to think in basic operations until you understand the problem,
>> then bring in the complex commands.  If you bring them in any earlier,
>> you'll get lost in detail and confused.
[...]
>> Now let's compare your proposal to my ideas.  Your BLOCK_MEDIUM_CHANGED
>> appears to track my medium <-> empty.  You emit it from the block
>> layer's bdrv_dev_change_media_cb().  Called to notify device models
>> whenever the block layer changes media.  The "whenever it changes media"
>> part makes it a convenient choke point.
>> 
>> Your GUEST_MEDIUM_EJECTED does *not* track my open <-> closed.  I think
>> it's more complex than a straight open <-> closed event.  Evidence: your
>> event documentation in qmp-events.txt needs an extra note to clarify
>> when exactly the event is emitted.
>
> The purpose of the note is not to clarify, but to emphasize that the event
> is about guest initiated ejects. Also, I disagree it's more complex, actually
> I think it's quite simple vs. emitting the eject event from the eject and
> change commands, as this can be messy because of their complicated semantics.

What's complex about an event that fires when the tray moves?  Isn't
that's as simple as it gets?  The complexity is purely in the two
monitor commands.  Adding events doesn't increase that complexity, it
merely makes it easier to observe.

I feel strongly that we shouldn't design events around monitor
commands[*].  Especially not complex, messy monitor commands that'll
have to be broken up for QMP eventually.  Events should be about
noteworthy state transitions.

Fundamental state machines such as the one modelling tray behavior don't
change much.  But triggers for their state transitions are less stable.
I expect an event that just reports a state transition to age more
gracefully than one that is also tied to triggers, such as "the guest
did it".

>> This is just my analysis of the problem.  If folks think your proposal
>> serves their needs, and Kevin is happy with it, I won't object.
>
> No feedback so far.


[*] I'm oversimplifying.  Events are *usually* about noteworthy state
transitions.  But we sometimes use the event mechanism for asynchronous
replies to synchronous monitor commands, like in your "QMP: add
balloon-get-memory-stats command" series.  That's fine.

Re: [Qemu-devel] [RFC 0/5]: QMP: Introduce GUEST_MEDIUM_EJECT & BLOCK_MEDIUM_CHANGED

[Luiz Capitulino]

On Fri, 10 Feb 2012 10:27:21 +0100
Markus Armbruster <armbru@redhat.com> wrote:

> Luiz Capitulino <lcapitulino@redhat.com> writes:
> 
> > On Thu, 09 Feb 2012 16:01:21 +0100
> > Markus Armbruster <armbru@redhat.com> wrote:
> >
> >> Luiz Capitulino <lcapitulino@redhat.com> writes:
> >> 
> >> > I've tried to implement a BLOCK_MEDIUM_EJECT event that, as we discussed[1],
> >> > would be emitted by guest-initiated ejects and by the QMP/HMP eject and change
> >> > commands.
> >> >
> >> > However, that turned to be a bit problematic, because the eject and change
> >> > commands don't exactly handle tray movements: they actually insert/purge a
> >> > medium from from the drive.
> >> 
> >> The monitor commands are complex, because they do several things, and
> >> the exact things they do depends on circumstances.  In my experience,
> >> it's best to think in basic operations until you understand the problem,
> >> then bring in the complex commands.  If you bring them in any earlier,
> >> you'll get lost in detail and confused.
> [...]
> >> Now let's compare your proposal to my ideas.  Your BLOCK_MEDIUM_CHANGED
> >> appears to track my medium <-> empty.  You emit it from the block
> >> layer's bdrv_dev_change_media_cb().  Called to notify device models
> >> whenever the block layer changes media.  The "whenever it changes media"
> >> part makes it a convenient choke point.
> >> 
> >> Your GUEST_MEDIUM_EJECTED does *not* track my open <-> closed.  I think
> >> it's more complex than a straight open <-> closed event.  Evidence: your
> >> event documentation in qmp-events.txt needs an extra note to clarify
> >> when exactly the event is emitted.
> >
> > The purpose of the note is not to clarify, but to emphasize that the event
> > is about guest initiated ejects. Also, I disagree it's more complex, actually
> > I think it's quite simple vs. emitting the eject event from the eject and
> > change commands, as this can be messy because of their complicated semantics.
> 
> What's complex about an event that fires when the tray moves?  Isn't
> that's as simple as it gets?  The complexity is purely in the two
> monitor commands.  Adding events doesn't increase that complexity, it
> merely makes it easier to observe.

In principle this is completely right, but in practice I see a few problems
because the change and eject commands require the event to be emitted as
special cases (as those commands don't always move the tray).

But note that I'm not saying this is undoable, just saying that I don't
like as much as I like emitting the event only when the guest moves the tray.

> I feel strongly that we shouldn't design events around monitor
> commands[*].  Especially not complex, messy monitor commands that'll
> have to be broken up for QMP eventually.  Events should be about
> noteworthy state transitions.

Well, if we break eject/change into multiple commands and each command does
a single state transition, then we end up doing what you're saying, except
that no explicit state machine code exists (like the RunState code).

> Fundamental state machines such as the one modelling tray behavior don't
> change much.  But triggers for their state transitions are less stable.
> I expect an event that just reports a state transition to age more
> gracefully than one that is also tied to triggers, such as "the guest
> did it".
> 
> >> This is just my analysis of the problem.  If folks think your proposal
> >> serves their needs, and Kevin is happy with it, I won't object.
> >
> > No feedback so far.
> 
> 
> [*] I'm oversimplifying.  Events are *usually* about noteworthy state
> transitions.  But we sometimes use the event mechanism for asynchronous
> replies to synchronous monitor commands, like in your "QMP: add
> balloon-get-memory-stats command" series.  That's fine.
> 

Re: [Qemu-devel] [RFC 0/5]: QMP: Introduce GUEST_MEDIUM_EJECT & BLOCK_MEDIUM_CHANGED

[Paolo Bonzini]

On 02/09/2012 04:01 PM, Markus Armbruster wrote:
> Your GUEST_MEDIUM_EJECTED does*not*  track my open<->  closed.  I think
> it's more complex than a straight open<->  closed event.  Evidence: your
> event documentation in qmp-events.txt needs an extra note to clarify
> when exactly the event is emitted.

I think I agree at this point that always generating an event for open 
<-> closed would make sense.

However, we need to write a proper state machine rather than keeping it 
implicit.  Events would be generated in the state machine rather than 
magically in bdrv_eject/bdrv_close.  We could also take the occasion to 
move all this out of block.c which is becoming huge.  So we would have:

guest eject, tray locked:
     nothing

guest eject, tray unlocked:
     BLOCK_MEDIUM_EJECT
     empty/full not affected

guest eject, tray open:
     BLOCK_MEDIUM_EJECT
     empty/full not affected

eject, tray locked:
     eject request sent to guest
     guest responds to eject request as above

eject, tray unlocked and full:
     BLOCK_MEDIUM_EJECT
     BLOCK_MEDIUM_CHANGED

eject, tray unlocked and empty:
     BLOCK_MEDIUM_EJECT

eject, tray open and full:
     BLOCK_MEDIUM_CHANGED

eject, tray open and empty:
     no event

change, tray locked:
     eject request sent to guest
     guest responds to eject request as above

change, tray unlocked and full:
     BLOCK_MEDIUM_EJECT (to open)
     BLOCK_MEDIUM_CHANGED (perhaps twice? full -> empty -> full)
     BLOCK_MEDIUM_EJECT (to close)

change, tray unlocked and empty:
     BLOCK_MEDIUM_EJECT (to open)
     BLOCK_MEDIUM_CHANGED
     BLOCK_MEDIUM_EJECT (to close)

change, tray open and full:
     BLOCK_MEDIUM_CHANGED (perhaps twice?)
     BLOCK_MEDIUM_EJECT (to close)

change, tray open and empty:
     BLOCK_MEDIUM_CHANGED
     BLOCK_MEDIUM_EJECT (to close)

Luiz, can you try making a proof of concept of this state machine? 
Events then would hopefully come natural.

Paolo

Re: [Qemu-devel] [RFC 0/5]: QMP: Introduce GUEST_MEDIUM_EJECT & BLOCK_MEDIUM_CHANGED

[Markus Armbruster]

Paolo Bonzini <pbonzini@redhat.com> writes:

> On 02/09/2012 04:01 PM, Markus Armbruster wrote:
>> Your GUEST_MEDIUM_EJECTED does*not*  track my open<->  closed.  I think
>> it's more complex than a straight open<->  closed event.  Evidence: your
>> event documentation in qmp-events.txt needs an extra note to clarify
>> when exactly the event is emitted.
>
> I think I agree at this point that always generating an event for open
> <-> closed would make sense.
>
> However, we need to write a proper state machine rather than keeping
> it implicit.  Events would be generated in the state machine rather
> than magically in bdrv_eject/bdrv_close.  We could also take the
> occasion to move all this out of block.c which is becoming huge.  So
> we would have:
>
> guest eject, tray locked:
>     nothing
>
> guest eject, tray unlocked:
>     BLOCK_MEDIUM_EJECT
>     empty/full not affected
>
> guest eject, tray open:
>     BLOCK_MEDIUM_EJECT
>     empty/full not affected
>
> eject, tray locked:
>     eject request sent to guest
>     guest responds to eject request as above
>
> eject, tray unlocked and full:
>     BLOCK_MEDIUM_EJECT
>     BLOCK_MEDIUM_CHANGED
>
> eject, tray unlocked and empty:
>     BLOCK_MEDIUM_EJECT
>
> eject, tray open and full:
>     BLOCK_MEDIUM_CHANGED
>
> eject, tray open and empty:
>     no event
>
> change, tray locked:
>     eject request sent to guest
>     guest responds to eject request as above
>
> change, tray unlocked and full:
>     BLOCK_MEDIUM_EJECT (to open)
>     BLOCK_MEDIUM_CHANGED (perhaps twice? full -> empty -> full)
>     BLOCK_MEDIUM_EJECT (to close)
>
> change, tray unlocked and empty:
>     BLOCK_MEDIUM_EJECT (to open)
>     BLOCK_MEDIUM_CHANGED
>     BLOCK_MEDIUM_EJECT (to close)
>
> change, tray open and full:
>     BLOCK_MEDIUM_CHANGED (perhaps twice?)
>     BLOCK_MEDIUM_EJECT (to close)
>
> change, tray open and empty:
>     BLOCK_MEDIUM_CHANGED
>     BLOCK_MEDIUM_EJECT (to close)
>
> Luiz, can you try making a proof of concept of this state machine?
> Events then would hopefully come natural.

Making the tray state machine explicit may make sense.  But we also need
to preserve the sane guest / host split: tray movement and locking is
guest matter, handling media in an open tray is host matter.

Moreover, let's not think "eject" and "change".  These are complex
actions that should be built from basic parts.  The verbs I want used
are open, close, lock, unlock, insert, remove.

Eject becomes something like open (if not already open) + remove (if
open and not empty).

Change becomes something like open (if not already open) + remove (if
open and not empty) + insert (if empty) + close (if open).

Re: [Qemu-devel] [RFC 0/5]: QMP: Introduce GUEST_MEDIUM_EJECT & BLOCK_MEDIUM_CHANGED

[Luiz Capitulino]

On Fri, 10 Feb 2012 10:36:06 +0100
Markus Armbruster <armbru@redhat.com> wrote:

> Paolo Bonzini <pbonzini@redhat.com> writes:
> 
> > On 02/09/2012 04:01 PM, Markus Armbruster wrote:
> >> Your GUEST_MEDIUM_EJECTED does*not*  track my open<->  closed.  I think
> >> it's more complex than a straight open<->  closed event.  Evidence: your
> >> event documentation in qmp-events.txt needs an extra note to clarify
> >> when exactly the event is emitted.
> >
> > I think I agree at this point that always generating an event for open
> > <-> closed would make sense.
> >
> > However, we need to write a proper state machine rather than keeping
> > it implicit.  Events would be generated in the state machine rather
> > than magically in bdrv_eject/bdrv_close.  We could also take the
> > occasion to move all this out of block.c which is becoming huge.  So
> > we would have:
> >
> > guest eject, tray locked:
> >     nothing
> >
> > guest eject, tray unlocked:
> >     BLOCK_MEDIUM_EJECT
> >     empty/full not affected
> >
> > guest eject, tray open:
> >     BLOCK_MEDIUM_EJECT
> >     empty/full not affected

I think we should only emit the event when the tray actually moves, that's what
mngt is interested in.

> > eject, tray locked:
> >     eject request sent to guest
> >     guest responds to eject request as above
> >
> > eject, tray unlocked and full:
> >     BLOCK_MEDIUM_EJECT
> >     BLOCK_MEDIUM_CHANGED

I don't think BLOCK_MEDIUM_EJECT should be emitted if the tray is already open.

> > eject, tray unlocked and empty:
> >     BLOCK_MEDIUM_EJECT

And closed...

> > eject, tray open and full:
> >     BLOCK_MEDIUM_CHANGED
> >
> > eject, tray open and empty:
> >     no event

Yes.

> >
> > change, tray locked:
> >     eject request sent to guest
> >     guest responds to eject request as above
> >
> > change, tray unlocked and full:
> >     BLOCK_MEDIUM_EJECT (to open)
> >     BLOCK_MEDIUM_CHANGED (perhaps twice? full -> empty -> full)
> >     BLOCK_MEDIUM_EJECT (to close)
> >
> > change, tray unlocked and empty:
> >     BLOCK_MEDIUM_EJECT (to open)
> >     BLOCK_MEDIUM_CHANGED
> >     BLOCK_MEDIUM_EJECT (to close)
> >
> > change, tray open and full:
> >     BLOCK_MEDIUM_CHANGED (perhaps twice?)
> >     BLOCK_MEDIUM_EJECT (to close)
> >
> > change, tray open and empty:
> >     BLOCK_MEDIUM_CHANGED
> >     BLOCK_MEDIUM_EJECT (to close)
> >
> > Luiz, can you try making a proof of concept of this state machine?
> >
> > Events then would hopefully come natural.
> 
> Making the tray state machine explicit may make sense.  But we also need
> to preserve the sane guest / host split: tray movement and locking is
> guest matter, handling media in an open tray is host matter.
> 
> Moreover, let's not think "eject" and "change".  These are complex
> actions that should be built from basic parts.  The verbs I want used
> are open, close, lock, unlock, insert, remove.
> 
> Eject becomes something like open (if not already open) + remove (if
> open and not empty).
> 
> Change becomes something like open (if not already open) + remove (if
> open and not empty) + insert (if empty) + close (if open).

This reminds me about an earlier try where I did the following, iirc:

 1. added commands blockdev-tray-open, blockdev-tray-close, blockdev-medium-insert,
    blockdev-medium-remove
 2. added the events: BLOCK_TRAY_OPEN, BLOCK_TRAY_CLOSE, BLOCK_MEDIUM_INSERTED
    BLOCK_MEDIUM_REMOVED, which would be emitted when the relating command is issued
    (maybe the events could just be BLOCK_TRAY_CHANGED & BLOCK_MEDIUM_CHANGED)
 3. re-wrote eject and change in terms of the new commands, note that you get the
    events for free

Now, maybe the guest eject could also emit BLOCK_TRAY_OPEN & BLOCK_TRAY_CLOSE. Then
I think this is a complete solution.

Do you guys agree?

Re: [Qemu-devel] [RFC 0/5]: QMP: Introduce GUEST_MEDIUM_EJECT & BLOCK_MEDIUM_CHANGED

[Kevin Wolf]

Am 10.02.2012 18:04, schrieb Luiz Capitulino:
> This reminds me about an earlier try where I did the following, iirc:
> 
>  1. added commands blockdev-tray-open, blockdev-tray-close, blockdev-medium-insert,
>     blockdev-medium-remove
>  2. added the events: BLOCK_TRAY_OPEN, BLOCK_TRAY_CLOSE, BLOCK_MEDIUM_INSERTED
>     BLOCK_MEDIUM_REMOVED, which would be emitted when the relating command is issued
>     (maybe the events could just be BLOCK_TRAY_CHANGED & BLOCK_MEDIUM_CHANGED)
>  3. re-wrote eject and change in terms of the new commands, note that you get the
>     events for free
> 
> Now, maybe the guest eject could also emit BLOCK_TRAY_OPEN & BLOCK_TRAY_CLOSE. Then
> I think this is a complete solution.
> 
> Do you guys agree?

Looks good to me in general. I'm not sure how you're imagining to
implement this, I would prefer not to emit events from the device code,
but only from block.c.

Another interesting point is what to do with host CD-ROM passthrough. I
think the TRAY_OPEN/CLOSE part is doable (do Paolo's patches actually do
that, so that we just need to add an event?). We would have to fake
MEDIUM_REMOVED/INSERTED immediately before a TRAY_CLOSE if the medium
has changed. LOCK/UNLOCK (which you forgot in your list) is only
initiated by the guest or monitor (eject -f), so there's nothing special
with passthrough.

Kevin

Re: [Qemu-devel] [RFC 0/5]: QMP: Introduce GUEST_MEDIUM_EJECT & BLOCK_MEDIUM_CHANGED

[Paolo Bonzini]

On 02/10/2012 06:55 PM, Kevin Wolf wrote:
> Am 10.02.2012 18:04, schrieb Luiz Capitulino:
>> This reminds me about an earlier try where I did the following, iirc:
>>
>>  1. added commands blockdev-tray-open, blockdev-tray-close, blockdev-medium-insert,
>>     blockdev-medium-remove

I think this slightly overengineering.  eject and change work well 
enough, we do not need blockdev-medium-insert and blockdev-medium-remove 
(yet).  Of course there can be a new API, just nothing user-visible.

>>  2. added the events: BLOCK_TRAY_OPEN, BLOCK_TRAY_CLOSE, BLOCK_MEDIUM_INSERTED
>>     BLOCK_MEDIUM_REMOVED, which would be emitted when the relating command is issued
>>     (maybe the events could just be BLOCK_TRAY_CHANGED & BLOCK_MEDIUM_CHANGED)

Or even just one event with two boolean arguments.

Looks slightly less clean, but it has an advantage: a guest that sends 
"eject" can wait for an event and will know whether the eject command 
was really executed (tray = open, medium = none) or just an eject 
request was obeyed by the guest (tray = open, medium = present).

>> Now, maybe the guest eject could also emit BLOCK_TRAY_OPEN & BLOCK_TRAY_CLOSE. Then
>> I think this is a complete solution.

Yes.

> Looks good to me in general. I'm not sure how you're imagining to
> implement this, I would prefer not to emit events from the device code,
> but only from block.c.

... and yes. :)

> Another interesting point is what to do with host CD-ROM passthrough. I
> think the TRAY_OPEN/CLOSE part is doable (do Paolo's patches actually do
> that, so that we just need to add an event?).

It is in the part that I haven't posted yet.

> We would have to fake
> MEDIUM_REMOVED/INSERTED immediately before a TRAY_CLOSE if the medium
> has changed.

Not sure about this, the "medium" hasn't changed in the sense that the 
backend is still the same.

With passthrough, eject could become a synonym of tray-open.  It is very 
unintuitive that the device is completely disconnected by the backend. 
And with passthrough, as soon as the guest ejects my CD I know that I 
have to remove the medium before the guest reboots.  In other words we 
can expect some kind of collaboration from the user.

> LOCK/UNLOCK (which you forgot in your list) is only
> initiated by the guest or monitor (eject -f), so there's nothing special
> with passthrough.

Well, there are a couple of places where we unlock without calling 
bdrv_lock_medium, those should be fixed so that in the passthrough case 
we force-unlock the host CD too.

Paolo

Re: [Qemu-devel] [RFC 0/5]: QMP: Introduce GUEST_MEDIUM_EJECT & BLOCK_MEDIUM_CHANGED

[Paolo Bonzini]

On 02/10/2012 08:39 PM, Paolo Bonzini wrote:
>>>
>>>  1. added commands blockdev-tray-open, blockdev-tray-close,
>>> blockdev-medium-insert,
>>>     blockdev-medium-remove
>
> I think this slightly overengineering.  eject and change work well
> enough, we do not need blockdev-medium-insert and blockdev-medium-remove
> (yet).  Of course there can be a new API, just nothing user-visible.
>
>>>  2. added the events: BLOCK_TRAY_OPEN, BLOCK_TRAY_CLOSE,
>>> BLOCK_MEDIUM_INSERTED
>>>     BLOCK_MEDIUM_REMOVED, which would be emitted when the relating
>>> command is issued
>>>     (maybe the events could just be BLOCK_TRAY_CHANGED &
>>> BLOCK_MEDIUM_CHANGED)
>
> Or even just one event with two boolean arguments.
>
> Looks slightly less clean, but it has an advantage: a guest that sends
> "eject" can wait for an event and will know whether the eject command
> was really executed (tray = open, medium = none) or just an eject
> request was obeyed by the guest (tray = open, medium = present).
>
>>> Now, maybe the guest eject could also emit BLOCK_TRAY_OPEN &
>>> BLOCK_TRAY_CLOSE. Then
>>> I think this is a complete solution.
>
> Yes.

Hmm... I don't know...

The more I think about it, the more I like Luiz's original proposal of 
only signaling guest-initiated ejects.  I won't hold up any other patch 
that works, but I cannot think of something that is as easily usable by 
management in a non-racy way.

Paolo

[Qemu-devel] [PATCH v3 0/5]: QMP: add DEVICE_TRAY_MOVED event

[Luiz Capitulino]

The event name changed, which caused the subject to change too, hope this
won't cause confusion.

v3

o Rename the event to DEVICE_TRAY_MOVED
o Rename the 'ejected' event data to 'tray-open'
o Only call bdrv_eject() if the tray state changed
o Drop ide_tray_state_post_load()

 QMP/qmp-events.txt |   18 +++++++++++
 block.c            |   84 +++++++++++++++++++++++++++++++++------------------
 block.h            |    8 ++--
 block/raw-posix.c  |    6 ++--
 block/raw.c        |    2 +-
 block_int.h        |    2 +-
 hw/ide/atapi.c     |    7 +++-
 hw/ide/core.c      |   16 ++--------
 hw/scsi-disk.c     |   13 +++++---
 hw/virtio-blk.c    |    6 ++--
 monitor.c          |    3 ++
 monitor.h          |    1 +
 12 files changed, 104 insertions(+), 62 deletions(-)

[Qemu-devel] [PATCH 2/5] 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 8d4cfea..bfb0dec 100644
--- a/block.c
+++ b/block.c
@@ -3609,7 +3609,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 ec0a6c8..49bca5a 100644
--- a/block.h
+++ b/block.h
@@ -265,7 +265,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 7946cf6..04f4b83 100644
--- a/block_int.h
+++ b/block_int.h
@@ -197,7 +197,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] [PULL 0/5]: QMP queue

[Luiz Capitulino]

Contains only the DEVICE_TRAY_MOVED event series.

The changes (since 235fe3bfd46b1104575b540d0bc3fdf584030b99) are available
in the following repository:

    git://repo.or.cz/qemu/qmp-unstable.git queue/qmp

Luiz Capitulino (5):
      block: Rename bdrv_mon_event() & BlockMonEventAction
      block: bdrv_eject(): Make eject_flag a real bool
      block: Don't call bdrv_eject() if the tray state didn't change
      ide: drop ide_tray_state_post_load()
      qmp: add DEVICE_TRAY_MOVED event

 QMP/qmp-events.txt |   18 +++++++++++
 block.c            |   84 +++++++++++++++++++++++++++++++++------------------
 block.h            |    8 ++--
 block/raw-posix.c  |    6 ++--
 block/raw.c        |    2 +-
 block_int.h        |    2 +-
 hw/ide/atapi.c     |    7 +++-
 hw/ide/core.c      |   16 ++--------
 hw/scsi-disk.c     |   13 +++++---
 hw/virtio-blk.c    |    6 ++--
 monitor.c          |    3 ++
 monitor.h          |    1 +
 12 files changed, 104 insertions(+), 62 deletions(-)

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

[Luiz Capitulino]

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
Reviewed-by: Markus Armbruster <armbru@redhat.com>
Acked-by: Kevin Wolf <kwolf@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 8d4cfea..bfb0dec 100644
--- a/block.c
+++ b/block.c
@@ -3609,7 +3609,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 ec0a6c8..49bca5a 100644
--- a/block.h
+++ b/block.h
@@ -265,7 +265,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 7946cf6..04f4b83 100644
--- a/block_int.h
+++ b/block_int.h
@@ -197,7 +197,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