[Qemu-devel] [RFC 00/10]: QMP/HMP: Introduce tray handling commands

[Qemu-devel] [RFC 00/10]: QMP/HMP: Introduce tray handling commands

[Luiz Capitulino]

In a recent discussion on the mailing list regarding the introduction of
the BLOCK_TRAY_OPEN and BLOCK_TRAY_CLOSE events[1], it was mentioned that
we need to fix the eject command and maybe introduce new commands first.

Here's a my proposal.

This series introduces three new commands:

 o blockdev-tray-open: opens the drive tray. Also Supports removing the inserted
   media. The BLOCK_TRAY_OPEN event is emitted if this command succeeds.
 o blockdev-tray-close: closes a drive tray. The BLOCK_TRAY_CLOSE event is
   emitted.
 o blockdev-media-insert: Inserts a media in the tray. The tray must empty
   and already opened. No event is emitted.

The existing 'eject' and 'change' commands are completely rewriten in terms
of the new commands. Besides fixing some bad behaviors, this also makes it
possible for the events to be automtically emitted.

Ejecting a device inside the guest (or closing its tray) also causes the
events to be emitted.

Everything which applies for the virtual tray seems to work as expected for
the host's tray. But more testing is needed.

 QMP/qmp-events.txt |   30 ++++++++++
 block.c            |   36 +++++++++++-
 block.h            |    6 +-
 block/raw.c        |    2 +-
 blockdev.c         |  157 ++++++++++++++++++++++++++++++++++++++++------------
 blockdev.h         |    3 +
 hw/ide/atapi.c     |    2 +-
 hw/ide/core.c      |    6 +-
 hw/scsi-disk.c     |    8 +-
 hw/virtio-blk.c    |    6 +-
 monitor.c          |    6 ++
 monitor.h          |    2 +
 qmp-commands.hx    |   84 ++++++++++++++++++++++++++++
 13 files changed, 294 insertions(+), 54 deletions(-)

[Qemu-devel] [RFC 01/10] block: bdrv_eject(): Add 'force' parameter

[Luiz Capitulino]

It's purpose is to skip the media locked test. This is going to be used
by the blockdev-tray-open QMP command.

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

diff --git a/block.c b/block.c
index effa86f..a3d0556 100644
--- a/block.c
+++ b/block.c
@@ -2752,12 +2752,12 @@ int bdrv_media_changed(BlockDriverState *bs)
 /**
  * If eject_flag is TRUE, eject the media. Otherwise, close the tray
  */
-int bdrv_eject(BlockDriverState *bs, int eject_flag)
+int bdrv_eject(BlockDriverState *bs, int eject_flag, int force)
 {
     BlockDriver *drv = bs->drv;
     int ret;
 
-    if (bs->locked) {
+    if (bs->locked && !force) {
         return -EBUSY;
     }
 
diff --git a/block.h b/block.h
index da7d39c..131812c 100644
--- a/block.h
+++ b/block.h
@@ -186,7 +186,7 @@ int bdrv_is_inserted(BlockDriverState *bs);
 int bdrv_media_changed(BlockDriverState *bs);
 int bdrv_is_locked(BlockDriverState *bs);
 void bdrv_set_locked(BlockDriverState *bs, int locked);
-int bdrv_eject(BlockDriverState *bs, int eject_flag);
+int bdrv_eject(BlockDriverState *bs, int eject_flag, int force);
 void bdrv_set_change_cb(BlockDriverState *bs,
                         void (*change_cb)(void *opaque, int reason),
                         void *opaque);
diff --git a/block/raw.c b/block/raw.c
index b0f72d6..0e23c7c 100644
--- a/block/raw.c
+++ b/block/raw.c
@@ -77,7 +77,7 @@ static int raw_is_inserted(BlockDriverState *bs)
 
 static int raw_eject(BlockDriverState *bs, int eject_flag)
 {
-    return bdrv_eject(bs->file, eject_flag);
+    return bdrv_eject(bs->file, eject_flag, 0);
 }
 
 static int raw_set_locked(BlockDriverState *bs, int locked)
diff --git a/hw/ide/atapi.c b/hw/ide/atapi.c
index fe2fb0b..8af6e23 100644
--- a/hw/ide/atapi.c
+++ b/hw/ide/atapi.c
@@ -897,7 +897,7 @@ static void cmd_start_stop_unit(IDEState *s, uint8_t* buf)
     eject = (buf[4] >> 1) & 1;
 
     if (eject) {
-        err = bdrv_eject(s->bs, !start);
+        err = bdrv_eject(s->bs, !start, 0);
     }
 
     switch (err) {
diff --git a/hw/scsi-disk.c b/hw/scsi-disk.c
index a8c7372..d3f982e 100644
--- a/hw/scsi-disk.c
+++ b/hw/scsi-disk.c
@@ -872,7 +872,7 @@ static int scsi_disk_emulate_command(SCSIDiskReq *r, uint8_t *outbuf)
     case START_STOP:
         if (s->drive_kind == SCSI_CD && (req->cmd.buf[4] & 2)) {
             /* load/eject medium */
-            bdrv_eject(s->bs, !(req->cmd.buf[4] & 1));
+            bdrv_eject(s->bs, !(req->cmd.buf[4] & 1), 0);
         }
 	break;
     case ALLOW_MEDIUM_REMOVAL:
-- 
1.7.4.4

[Qemu-devel] [RFC 02/10] block: Rename bdrv_mon_event()

[Luiz Capitulino]

Rename it to bdrv_error_mon_event() in order to better communicate
its purpose.

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

diff --git a/block.c b/block.c
index a3d0556..959eedd 100644
--- a/block.c
+++ b/block.c
@@ -1656,8 +1656,8 @@ int bdrv_is_allocated(BlockDriverState *bs, int64_t sector_num, int nb_sectors,
     return bs->drv->bdrv_is_allocated(bs, sector_num, nb_sectors, pnum);
 }
 
-void bdrv_mon_event(const BlockDriverState *bdrv,
-                    BlockMonEventAction action, int is_read)
+void bdrv_error_mon_event(const BlockDriverState *bdrv,
+                          BlockMonEventAction action, int is_read)
 {
     QObject *data;
     const char *action_str;
diff --git a/block.h b/block.h
index 131812c..c71bf32 100644
--- a/block.h
+++ b/block.h
@@ -50,8 +50,8 @@ typedef enum {
     BDRV_ACTION_REPORT, BDRV_ACTION_IGNORE, BDRV_ACTION_STOP
 } BlockMonEventAction;
 
-void bdrv_mon_event(const BlockDriverState *bdrv,
-                    BlockMonEventAction action, int is_read);
+void bdrv_error_mon_event(const BlockDriverState *bdrv,
+                          BlockMonEventAction 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 45410e8..96c153e 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -440,7 +440,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_error_mon_event(s->bs, BDRV_ACTION_IGNORE, is_read);
         return 0;
     }
 
@@ -448,7 +448,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->dma->ops->add_status(s->bus->dma, op);
-        bdrv_mon_event(s->bs, BDRV_ACTION_STOP, is_read);
+        bdrv_error_mon_event(s->bs, BDRV_ACTION_STOP, is_read);
         vm_stop(VMSTOP_DISKFULL);
     } else {
         if (op & BM_STATUS_DMA_RETRY) {
@@ -457,7 +457,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_error_mon_event(s->bs, BDRV_ACTION_REPORT, is_read);
     }
 
     return 1;
diff --git a/hw/scsi-disk.c b/hw/scsi-disk.c
index d3f982e..d641309 100644
--- a/hw/scsi-disk.c
+++ b/hw/scsi-disk.c
@@ -204,7 +204,7 @@ static int scsi_handle_rw_error(SCSIDiskReq *r, int error, int type)
     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_error_mon_event(s->bs, BDRV_ACTION_IGNORE, is_read);
         return 0;
     }
 
@@ -214,7 +214,7 @@ static int scsi_handle_rw_error(SCSIDiskReq *r, int error, int type)
         type &= SCSI_REQ_STATUS_RETRY_TYPE_MASK;
         r->status |= SCSI_REQ_STATUS_RETRY | type;
 
-        bdrv_mon_event(s->bs, BDRV_ACTION_STOP, is_read);
+        bdrv_error_mon_event(s->bs, BDRV_ACTION_STOP, is_read);
         vm_stop(VMSTOP_DISKFULL);
     } else {
         if (type == SCSI_REQ_STATUS_RETRY_READ) {
@@ -234,7 +234,7 @@ static int scsi_handle_rw_error(SCSIDiskReq *r, int error, int type)
                                   SENSE_CODE(IO_ERROR));
             break;
         }
-        bdrv_mon_event(s->bs, BDRV_ACTION_REPORT, is_read);
+        bdrv_error_mon_event(s->bs, BDRV_ACTION_REPORT, is_read);
     }
     return 1;
 }
diff --git a/hw/virtio-blk.c b/hw/virtio-blk.c
index 91e0394..99db00a 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_error_mon_event(s->bs, BDRV_ACTION_IGNORE, is_read);
         return 0;
     }
 
@@ -77,11 +77,11 @@ 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_error_mon_event(s->bs, BDRV_ACTION_STOP, is_read);
         vm_stop(VMSTOP_DISKFULL);
     } else {
         virtio_blk_req_complete(req, VIRTIO_BLK_S_IOERR);
-        bdrv_mon_event(s->bs, BDRV_ACTION_REPORT, is_read);
+        bdrv_error_mon_event(s->bs, BDRV_ACTION_REPORT, is_read);
     }
 
     return 1;
-- 
1.7.4.4

[Qemu-devel] [RFC 03/10] QMP: query-block: Add the 'tray-open' key

[Luiz Capitulino]

As its name implies this new key informs the device's tray status
to clients. It's only present if the device is a removable one.

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 block.c         |    5 +++++
 qmp-commands.hx |    2 ++
 2 files changed, 7 insertions(+), 0 deletions(-)

diff --git a/block.c b/block.c
index 959eedd..e1b9057 100644
--- a/block.c
+++ b/block.c
@@ -1740,6 +1740,11 @@ void bdrv_info(Monitor *mon, QObject **ret_data)
                                     bs->device_name, bs->removable,
                                     bs->locked);
 
+        if (bs->removable) {
+            QDict *dict = qobject_to_qdict(bs_obj);
+            qdict_put(dict, "tray-open", qbool_from_int(bs->tray_open));
+        }
+
         if (bs->drv) {
             QObject *obj;
             QDict *bs_dict = qobject_to_qdict(bs_obj);
diff --git a/qmp-commands.hx b/qmp-commands.hx
index a9f109a..8393f22 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -1043,6 +1043,8 @@ Each json-object contain the following:
          - Possible values: "unknown"
 - "removable": true if the device is removable, false otherwise (json-bool)
 - "locked": true if the device is locked, false otherwise (json-bool)
+- "tray-open": true if the device's tray is open, false otherwise. Only present
+               for removable media (json-bool)
 - "inserted": only present if the device is inserted, it is a json-object
    containing the following:
          - "file": device file name (json-string)
-- 
1.7.4.4

[Qemu-devel] [RFC 04/10] HMP: info block: Print the 'tray-open' key

[Luiz Capitulino]

It's printed before the "[not inserted]" or the "file" fields, like this:

 (qemu) info block
 ide0-hd0: removable=0 file=disks/test.img ro=0 drv=qcow2 encrypted=0
 ide1-cd0: removable=1 locked=0 tray-open=0 file=/Fedora-14-x86_64-DVD.iso ro=1 drv=raw encrypted=0
 floppy0: removable=1 locked=0 tray-open=0 [not inserted]
 sd0: removable=1 locked=0 tray-open=0 [not inserted]
 (qemu)

TODO: Confirm this won't break libvirt.

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 block.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/block.c b/block.c
index e1b9057..3df5a4f 100644
--- a/block.c
+++ b/block.c
@@ -1698,6 +1698,7 @@ static void bdrv_print_dict(QObject *obj, void *opaque)
 
     if (qdict_get_bool(bs_dict, "removable")) {
         monitor_printf(mon, " locked=%d", qdict_get_bool(bs_dict, "locked"));
+        monitor_printf(mon, " tray-open=%d", qdict_get_bool(bs_dict, "tray-open"));
     }
 
     if (qdict_haskey(bs_dict, "inserted")) {
-- 
1.7.4.4

Re: [Qemu-devel] [RFC 04/10] HMP: info block: Print the 'tray-open' key

[Markus Armbruster]

Luiz Capitulino <lcapitulino@redhat.com> writes:

> It's printed before the "[not inserted]" or the "file" fields, like this:
>
>  (qemu) info block
>  ide0-hd0: removable=0 file=disks/test.img ro=0 drv=qcow2 encrypted=0
>  ide1-cd0: removable=1 locked=0 tray-open=0 file=/Fedora-14-x86_64-DVD.iso ro=1 drv=raw encrypted=0
>  floppy0: removable=1 locked=0 tray-open=0 [not inserted]
>  sd0: removable=1 locked=0 tray-open=0 [not inserted]
>  (qemu)
>
> TODO: Confirm this won't break libvirt.

As far as I can see, libvirt uses neither "info block" nor
"query-block".

I'd squash this into the previous commit.

Re: [Qemu-devel] [RFC 04/10] HMP: info block: Print the 'tray-open' key

[Luiz Capitulino]

On Mon, 06 Jun 2011 15:19:34 +0200
Markus Armbruster <armbru@redhat.com> wrote:

> Luiz Capitulino <lcapitulino@redhat.com> writes:
> 
> > It's printed before the "[not inserted]" or the "file" fields, like this:
> >
> >  (qemu) info block
> >  ide0-hd0: removable=0 file=disks/test.img ro=0 drv=qcow2 encrypted=0
> >  ide1-cd0: removable=1 locked=0 tray-open=0 file=/Fedora-14-x86_64-DVD.iso ro=1 drv=raw encrypted=0
> >  floppy0: removable=1 locked=0 tray-open=0 [not inserted]
> >  sd0: removable=1 locked=0 tray-open=0 [not inserted]
> >  (qemu)
> >
> > TODO: Confirm this won't break libvirt.
> 
> As far as I can see, libvirt uses neither "info block" nor
> "query-block".

Thanks for checking it.

> I'd squash this into the previous commit.
> 

[Qemu-devel] [RFC 05/10] QMP: Introduce the blockdev-tray-open command

[Luiz Capitulino]

This command opens a removable media drive's tray. It's only available
in QMP.

The do_tray_open() function is split into two smaller functions because
next commits will also use them.

Please, check the command's documentation (being introduced in this
commit) for a detailed description.

XXX: Should we return an error if the tray is already open?

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 blockdev.c      |   46 ++++++++++++++++++++++++++++++++++++++++++++++
 blockdev.h      |    1 +
 qmp-commands.hx |   27 +++++++++++++++++++++++++++
 3 files changed, 74 insertions(+), 0 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index 6e0eb83..b1c705c 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -665,6 +665,52 @@ static int eject_device(Monitor *mon, BlockDriverState *bs, int force)
     return 0;
 }
 
+static BlockDriverState *bdrv_removable_find(const char *name)
+{
+    BlockDriverState *bs;
+
+    bs = bdrv_find(name);
+    if (!bs) {
+        qerror_report(QERR_DEVICE_NOT_FOUND, name);
+        return NULL;
+    }
+
+    if (!bdrv_is_removable(bs)) {
+        qerror_report(QERR_DEVICE_NOT_REMOVABLE, bdrv_get_device_name(bs));
+        return NULL;
+    }
+
+    return bs;
+}
+
+static int tray_open(const char *device, int remove, int force)
+{
+    BlockDriverState *bs;
+
+    bs = bdrv_removable_find(device);
+    if (!bs) {
+        return -1;
+    }
+
+    if (bdrv_eject(bs, 1, force) < 0) {
+        /* FIXME: will report undefined error in QMP */
+        return -1;
+    }
+
+    if (remove) {
+        bdrv_close(bs);
+    }
+
+    return 0;
+}
+
+int do_tray_open(Monitor *mon, const QDict *qdict, QObject **ret_data)
+{
+    return tray_open(qdict_get_str(qdict, "device"),
+                     qdict_get_try_bool(qdict, "remove", 0),
+                     qdict_get_try_bool(qdict, "force", 0));
+}
+
 int do_eject(Monitor *mon, const QDict *qdict, QObject **ret_data)
 {
     BlockDriverState *bs;
diff --git a/blockdev.h b/blockdev.h
index 3587786..5e46aae 100644
--- a/blockdev.h
+++ b/blockdev.h
@@ -65,5 +65,6 @@ int do_change_block(Monitor *mon, const char *device,
 int do_drive_del(Monitor *mon, const QDict *qdict, QObject **ret_data);
 int do_snapshot_blkdev(Monitor *mon, const QDict *qdict, QObject **ret_data);
 int do_block_resize(Monitor *mon, const QDict *qdict, QObject **ret_data);
+int do_tray_open(Monitor *mon, const QDict *qdict, QObject **ret_data);
 
 #endif
diff --git a/qmp-commands.hx b/qmp-commands.hx
index 8393f22..58ab132 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -153,6 +153,33 @@ Examples:
 EQMP
 
     {
+        .name       = "blockdev-tray-open",
+        .args_type  = "device:B,force:-f,remove:-r",
+        .mhandler.cmd_new = do_tray_open,
+    },
+
+SQMP
+blockdev-tray-open
+------------------
+
+Open a removable media drive's tray.
+
+Arguments: 
+
+- device: device name (json-string)
+- force: force ejection (json-bool, optional)
+- remove: remove the media (json-bool, optional)
+
+Example:
+
+-> { "execute": "blockdev-tray-open", "arguments": { "device": "ide1-cd0" } }
+<- { "return": {} }
+
+Note: The tray remains open after this command is issued
+
+EQMP
+
+    {
         .name       = "screendump",
         .args_type  = "filename:F",
         .params     = "filename",
-- 
1.7.4.4

Re: [Qemu-devel] [RFC 05/10] QMP: Introduce the blockdev-tray-open command

[Amit Shah]

On (Fri) 03 Jun 2011 [16:03:57], Luiz Capitulino wrote:

> +static int tray_open(const char *device, int remove, int force)
> +{
> +    BlockDriverState *bs;
> +
> +    bs = bdrv_removable_find(device);
> +    if (!bs) {
> +        return -1;
> +    }
> +
> +    if (bdrv_eject(bs, 1, force) < 0) {
> +        /* FIXME: will report undefined error in QMP */
> +        return -1;
> +    }
> +
> +    if (remove) {
> +        bdrv_close(bs);
> +    }
> +
> +    return 0;
> +}

What's the reason to tie the 'remove' with tray open?  Won't it be
simpler to have it separated out, perhaps a 'change' event instead of
'insert' that can accept NULL which means just remove medium?

		Amit

Re: [Qemu-devel] [RFC 05/10] QMP: Introduce the blockdev-tray-open command

[Luiz Capitulino]

On Mon, 6 Jun 2011 17:10:32 +0530
Amit Shah <amit.shah@redhat.com> wrote:

> On (Fri) 03 Jun 2011 [16:03:57], Luiz Capitulino wrote:
> 
> > +static int tray_open(const char *device, int remove, int force)
> > +{
> > +    BlockDriverState *bs;
> > +
> > +    bs = bdrv_removable_find(device);
> > +    if (!bs) {
> > +        return -1;
> > +    }
> > +
> > +    if (bdrv_eject(bs, 1, force) < 0) {
> > +        /* FIXME: will report undefined error in QMP */
> > +        return -1;
> > +    }
> > +
> > +    if (remove) {
> > +        bdrv_close(bs);
> > +    }
> > +
> > +    return 0;
> > +}
> 
> What's the reason to tie the 'remove' with tray open?

In my first try I had a command called 'blockdev-media-remove', but then
I had the impression that I was going too far as the only reason a client
would ever want to open the tray is to remove the media.

>  Won't it be
> simpler to have it separated out, perhaps a 'change' event instead of
> 'insert' that can accept NULL which means just remove medium?

You meant 'command' instead of 'event', right?

I don't think a change command makes sense, because it's just a shortcut
to open/remove/insert/close.

Re: [Qemu-devel] [RFC 05/10] QMP: Introduce the blockdev-tray-open command

[Amit Shah]

On (Mon) 06 Jun 2011 [11:38:03], Luiz Capitulino wrote:
> On Mon, 6 Jun 2011 17:10:32 +0530
> Amit Shah <amit.shah@redhat.com> wrote:
> 
> > On (Fri) 03 Jun 2011 [16:03:57], Luiz Capitulino wrote:
> > 
> > > +static int tray_open(const char *device, int remove, int force)
> > > +{
> > > +    BlockDriverState *bs;
> > > +
> > > +    bs = bdrv_removable_find(device);
> > > +    if (!bs) {
> > > +        return -1;
> > > +    }
> > > +
> > > +    if (bdrv_eject(bs, 1, force) < 0) {
> > > +        /* FIXME: will report undefined error in QMP */
> > > +        return -1;
> > > +    }
> > > +
> > > +    if (remove) {
> > > +        bdrv_close(bs);
> > > +    }
> > > +
> > > +    return 0;
> > > +}
> > 
> > What's the reason to tie the 'remove' with tray open?
> 
> In my first try I had a command called 'blockdev-media-remove', but then
> I had the impression that I was going too far as the only reason a client
> would ever want to open the tray is to remove the media.

Not necessary -- CD/DVD writers eject and reload trays after erasing
media -- at least they used to.

> >  Won't it be
> > simpler to have it separated out, perhaps a 'change' event instead of
> > 'insert' that can accept NULL which means just remove medium?
> 
> You meant 'command' instead of 'event', right?
> 
> I don't think a change command makes sense, because it's just a shortcut
> to open/remove/insert/close.

Yes, command, sorry.

And by 'change' I don't mean the current monitor change command --
that's a badly-named one.

By change I mean just that -- replace the media.  And that should
succeed only if tray is open.  And tray remains open after the change.

		Amit

Re: [Qemu-devel] [RFC 05/10] QMP: Introduce the blockdev-tray-open command

[Luiz Capitulino]

On Tue, 7 Jun 2011 18:59:12 +0530
Amit Shah <amit.shah@redhat.com> wrote:

> On (Mon) 06 Jun 2011 [11:38:03], Luiz Capitulino wrote:
> > On Mon, 6 Jun 2011 17:10:32 +0530
> > Amit Shah <amit.shah@redhat.com> wrote:
> > 
> > > On (Fri) 03 Jun 2011 [16:03:57], Luiz Capitulino wrote:
> > > 
> > > > +static int tray_open(const char *device, int remove, int force)
> > > > +{
> > > > +    BlockDriverState *bs;
> > > > +
> > > > +    bs = bdrv_removable_find(device);
> > > > +    if (!bs) {
> > > > +        return -1;
> > > > +    }
> > > > +
> > > > +    if (bdrv_eject(bs, 1, force) < 0) {
> > > > +        /* FIXME: will report undefined error in QMP */
> > > > +        return -1;
> > > > +    }
> > > > +
> > > > +    if (remove) {
> > > > +        bdrv_close(bs);
> > > > +    }
> > > > +
> > > > +    return 0;
> > > > +}
> > > 
> > > What's the reason to tie the 'remove' with tray open?
> > 
> > In my first try I had a command called 'blockdev-media-remove', but then
> > I had the impression that I was going too far as the only reason a client
> > would ever want to open the tray is to remove the media.
> 
> Not necessary -- CD/DVD writers eject and reload trays after erasing
> media -- at least they used to.

But that's not done by the VM's user/client.

I think I'll add it anyway, as it's what people seem to expect from an
API like this.

> > >  Won't it be
> > > simpler to have it separated out, perhaps a 'change' event instead of
> > > 'insert' that can accept NULL which means just remove medium?
> > 
> > You meant 'command' instead of 'event', right?
> > 
> > I don't think a change command makes sense, because it's just a shortcut
> > to open/remove/insert/close.
> 
> Yes, command, sorry.
> 
> And by 'change' I don't mean the current monitor change command --
> that's a badly-named one.
> 
> By change I mean just that -- replace the media.  And that should
> succeed only if tray is open.  And tray remains open after the change.

The same thing can be done with blockdev-media-remove and
blockdev-media-insert, so I don't think this adds value.

Re: [Qemu-devel] [RFC 05/10] QMP: Introduce the blockdev-tray-open command

[Markus Armbruster]

Luiz Capitulino <lcapitulino@redhat.com> writes:

> This command opens a removable media drive's tray. It's only available
> in QMP.
>
> The do_tray_open() function is split into two smaller functions because
> next commits will also use them.
>
> Please, check the command's documentation (being introduced in this
> commit) for a detailed description.
>
> XXX: Should we return an error if the tray is already open?

Use case?

For what it's worth, Linux ioctl CDROMCLOSETRAY appears to return
success when it does nothing.

[Qemu-devel] [RFC 06/10] QMP: Introduce the blockdev-tray-close command

[Luiz Capitulino]

This command closes a removable media drive's tray. It's only available
in QMP.

Please, check the command's documentation (being introduced in this
commit) for a detailed description.

XXX: Should we return an error if the tray is already closed?

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 blockdev.c      |   22 ++++++++++++++++++++++
 blockdev.h      |    1 +
 qmp-commands.hx |   23 +++++++++++++++++++++++
 3 files changed, 46 insertions(+), 0 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index b1c705c..943905d 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -704,6 +704,28 @@ static int tray_open(const char *device, int remove, int force)
     return 0;
 }
 
+static int tray_close(const char *device)
+{
+    BlockDriverState *bs;
+
+    bs = bdrv_removable_find(device);
+    if (!bs) {
+        return -1;
+    }
+
+    if (bdrv_eject(bs, 0, 0) < 0) {
+        /* FIXME: will report undefined error in QMP */
+        return -1;
+    }
+
+    return 0;
+}
+
+int do_tray_close(Monitor *mon, const QDict *qdict, QObject **ret_data)
+{
+    return tray_close(qdict_get_str(qdict, "device"));
+}
+
 int do_tray_open(Monitor *mon, const QDict *qdict, QObject **ret_data)
 {
     return tray_open(qdict_get_str(qdict, "device"),
diff --git a/blockdev.h b/blockdev.h
index 5e46aae..975e91a 100644
--- a/blockdev.h
+++ b/blockdev.h
@@ -66,5 +66,6 @@ int do_drive_del(Monitor *mon, const QDict *qdict, QObject **ret_data);
 int do_snapshot_blkdev(Monitor *mon, const QDict *qdict, QObject **ret_data);
 int do_block_resize(Monitor *mon, const QDict *qdict, QObject **ret_data);
 int do_tray_open(Monitor *mon, const QDict *qdict, QObject **ret_data);
+int do_tray_close(Monitor *mon, const QDict *qdict, QObject **ret_data);
 
 #endif
diff --git a/qmp-commands.hx b/qmp-commands.hx
index 58ab132..fdf9750 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -153,6 +153,29 @@ Examples:
 EQMP
 
     {
+        .name       = "blockdev-tray-close",
+        .args_type  = "device:B",
+        .mhandler.cmd_new = do_tray_close,
+    },
+
+SQMP
+blockdev-tray-close
+-------------------
+
+Close a removable media drive's tray.
+
+Arguments: 
+
+- device: device name (json-string)
+
+Example:
+
+-> { "execute": "blockdev-tray-close", "arguments": { "device": "ide1-cd0" } }
+<- { "return": {} }
+
+EQMP
+
+    {
         .name       = "blockdev-tray-open",
         .args_type  = "device:B,force:-f,remove:-r",
         .mhandler.cmd_new = do_tray_open,
-- 
1.7.4.4

[Qemu-devel] [RFC 07/10] QMP: Introduce the blockdev-media-insert command

[Luiz Capitulino]

This command inserts a new media in an already opened tray. It's only
available in QMP.

Please, check the command's documentation (being introduced in this
commit) for a detailed description.

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 blockdev.c      |   52 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 blockdev.h      |    1 +
 qmp-commands.hx |   32 ++++++++++++++++++++++++++++++++
 3 files changed, 85 insertions(+), 0 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index 943905d..14c8312 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -721,6 +721,58 @@ static int tray_close(const char *device)
     return 0;
 }
 
+static int media_insert(const char *device, const char *mediafile,
+                        const char *format)
+{
+    BlockDriver *drv = NULL;
+    BlockDriverState *bs;
+    int bdrv_flags;
+
+    bs = bdrv_removable_find(device);
+    if (!bs) {
+        return -1;
+    }
+
+    if (bdrv_is_locked(bs)) {
+        qerror_report(QERR_DEVICE_LOCKED, bdrv_get_device_name(bs));
+        return -1;
+    }
+
+    if (bdrv_is_inserted(bs)) {
+        /* FIXME: will report undefined error in QMP */
+        return -1;
+    }
+
+    if (!bs->tray_open) {
+        /* FIXME: will report undefined error in QMP */
+        return 1;
+    }
+
+    if (format) {
+        drv = bdrv_find_whitelisted_format(format);
+        if (!drv) {
+            qerror_report(QERR_INVALID_BLOCK_FORMAT, format);
+            return -1;
+        }
+    }
+
+    bdrv_flags = bdrv_is_read_only(bs) ? 0 : BDRV_O_RDWR;
+    bdrv_flags |= bdrv_is_snapshot(bs) ? BDRV_O_SNAPSHOT : 0;
+    if (bdrv_open(bs, mediafile, bdrv_flags, drv) < 0) {
+        qerror_report(QERR_OPEN_FILE_FAILED, mediafile);
+        return -1;
+    }
+
+    return 0;
+}
+
+int do_media_insert(Monitor *mon, const QDict *qdict, QObject **ret_data)
+{
+    return media_insert(qdict_get_str(qdict, "device"),
+                        qdict_get_str(qdict, "media"),
+                        qdict_get_try_str(qdict, "format"));
+}
+
 int do_tray_close(Monitor *mon, const QDict *qdict, QObject **ret_data)
 {
     return tray_close(qdict_get_str(qdict, "device"));
diff --git a/blockdev.h b/blockdev.h
index 975e91a..4dfd869 100644
--- a/blockdev.h
+++ b/blockdev.h
@@ -67,5 +67,6 @@ int do_snapshot_blkdev(Monitor *mon, const QDict *qdict, QObject **ret_data);
 int do_block_resize(Monitor *mon, const QDict *qdict, QObject **ret_data);
 int do_tray_open(Monitor *mon, const QDict *qdict, QObject **ret_data);
 int do_tray_close(Monitor *mon, const QDict *qdict, QObject **ret_data);
+int do_media_insert(Monitor *mon, const QDict *qdict, QObject **ret_data);
 
 #endif
diff --git a/qmp-commands.hx b/qmp-commands.hx
index fdf9750..fe50593 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -480,6 +480,38 @@ Example:
 EQMP
 
     {
+        .name       = "blockdev-media-insert",
+        .args_type  = "device:B,media:F,format:s?",
+        .mhandler.cmd_new = do_media_insert,
+    },
+
+SQMP
+blockdev-media-insert
+---------------------
+
+Insert a new media in a removable drive. The tray must be empty and already
+opened. The tray is not automatically closed (please, see blockdev-tray-open
+and blockdev-tray-close commands).
+
+Arguments: 
+
+- "device": device name (json-string)
+- "media": media file path (json-string)
+- "format": media file format (json-string, optional)
+
+Example:
+
+-> { "execute": "blockdev-media-insert",
+             "arguments": { "device": "ide1-cd0",
+                            "media": "/srv/images/Fedora-12-x86_64-DVD.iso" } }
+<- { "return": {} }
+
+Note: If the media is encrypted, the command block_passwd has to be used to
+      set the media's password.
+
+EQMP
+
+    {
         .name       = "migrate",
         .args_type  = "detach:-d,blk:-b,inc:-i,uri:s",
         .params     = "[-d] [-b] [-i] uri",
-- 
1.7.4.4

Re: [Qemu-devel] [RFC 07/10] QMP: Introduce the blockdev-media-insert command

[Amit Shah]

On (Fri) 03 Jun 2011 [16:03:59], Luiz Capitulino wrote:
>  
> +static int media_insert(const char *device, const char *mediafile,
> +                        const char *format)
> +{
> +    BlockDriver *drv = NULL;
> +    BlockDriverState *bs;
> +    int bdrv_flags;
> +
> +    bs = bdrv_removable_find(device);
> +    if (!bs) {
> +        return -1;
> +    }
> +
> +    if (bdrv_is_locked(bs)) {
> +        qerror_report(QERR_DEVICE_LOCKED, bdrv_get_device_name(bs));
> +        return -1;
> +    }
> +
> +    if (bdrv_is_inserted(bs)) {
> +        /* FIXME: will report undefined error in QMP */
> +        return -1;
> +    }
> +
> +    if (!bs->tray_open) {
> +        /* FIXME: will report undefined error in QMP */
> +        return 1;
> +    }

Yes, these should be fixed.


		Amit

Re: [Qemu-devel] [RFC 07/10] QMP: Introduce the blockdev-media-insert command

[Markus Armbruster]

Luiz Capitulino <lcapitulino@redhat.com> writes:

> This command inserts a new media in an already opened tray. It's only
> available in QMP.
>
> Please, check the command's documentation (being introduced in this
> commit) for a detailed description.
>
> Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
> ---
>  blockdev.c      |   52 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>  blockdev.h      |    1 +
>  qmp-commands.hx |   32 ++++++++++++++++++++++++++++++++
>  3 files changed, 85 insertions(+), 0 deletions(-)
>
> diff --git a/blockdev.c b/blockdev.c
> index 943905d..14c8312 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -721,6 +721,58 @@ static int tray_close(const char *device)
>      return 0;
>  }
>  
> +static int media_insert(const char *device, const char *mediafile,
> +                        const char *format)
> +{
> +    BlockDriver *drv = NULL;
> +    BlockDriverState *bs;
> +    int bdrv_flags;
> +
> +    bs = bdrv_removable_find(device);
> +    if (!bs) {
> +        return -1;
> +    }
> +
> +    if (bdrv_is_locked(bs)) {
> +        qerror_report(QERR_DEVICE_LOCKED, bdrv_get_device_name(bs));
> +        return -1;
> +    }

Why fail if locked?

If you managed to pry open the tray, I guess nothing should stop you
from changing the media in it.

> +
> +    if (bdrv_is_inserted(bs)) {
> +        /* FIXME: will report undefined error in QMP */
> +        return -1;
> +    }
> +
> +    if (!bs->tray_open) {
> +        /* FIXME: will report undefined error in QMP */
> +        return 1;
> +    }
> +
> +    if (format) {
> +        drv = bdrv_find_whitelisted_format(format);
> +        if (!drv) {
> +            qerror_report(QERR_INVALID_BLOCK_FORMAT, format);
> +            return -1;
> +        }
> +    }
> +
> +    bdrv_flags = bdrv_is_read_only(bs) ? 0 : BDRV_O_RDWR;
> +    bdrv_flags |= bdrv_is_snapshot(bs) ? BDRV_O_SNAPSHOT : 0;
> +    if (bdrv_open(bs, mediafile, bdrv_flags, drv) < 0) {
> +        qerror_report(QERR_OPEN_FILE_FAILED, mediafile);
> +        return -1;
> +    }
> +
> +    return 0;
> +}
[...]

Re: [Qemu-devel] [RFC 07/10] QMP: Introduce the blockdev-media-insert command

[Luiz Capitulino]

On Mon, 06 Jun 2011 15:40:28 +0200
Markus Armbruster <armbru@redhat.com> wrote:

> Luiz Capitulino <lcapitulino@redhat.com> writes:
> 
> > This command inserts a new media in an already opened tray. It's only
> > available in QMP.
> >
> > Please, check the command's documentation (being introduced in this
> > commit) for a detailed description.
> >
> > Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
> > ---
> >  blockdev.c      |   52 ++++++++++++++++++++++++++++++++++++++++++++++++++++
> >  blockdev.h      |    1 +
> >  qmp-commands.hx |   32 ++++++++++++++++++++++++++++++++
> >  3 files changed, 85 insertions(+), 0 deletions(-)
> >
> > diff --git a/blockdev.c b/blockdev.c
> > index 943905d..14c8312 100644
> > --- a/blockdev.c
> > +++ b/blockdev.c
> > @@ -721,6 +721,58 @@ static int tray_close(const char *device)
> >      return 0;
> >  }
> >  
> > +static int media_insert(const char *device, const char *mediafile,
> > +                        const char *format)
> > +{
> > +    BlockDriver *drv = NULL;
> > +    BlockDriverState *bs;
> > +    int bdrv_flags;
> > +
> > +    bs = bdrv_removable_find(device);
> > +    if (!bs) {
> > +        return -1;
> > +    }
> > +
> > +    if (bdrv_is_locked(bs)) {
> > +        qerror_report(QERR_DEVICE_LOCKED, bdrv_get_device_name(bs));
> > +        return -1;
> > +    }
> 
> Why fail if locked?
> 
> If you managed to pry open the tray, I guess nothing should stop you
> from changing the media in it.

Makes sense.

> 
> > +
> > +    if (bdrv_is_inserted(bs)) {
> > +        /* FIXME: will report undefined error in QMP */
> > +        return -1;
> > +    }
> > +
> > +    if (!bs->tray_open) {
> > +        /* FIXME: will report undefined error in QMP */
> > +        return 1;
> > +    }
> > +
> > +    if (format) {
> > +        drv = bdrv_find_whitelisted_format(format);
> > +        if (!drv) {
> > +            qerror_report(QERR_INVALID_BLOCK_FORMAT, format);
> > +            return -1;
> > +        }
> > +    }
> > +
> > +    bdrv_flags = bdrv_is_read_only(bs) ? 0 : BDRV_O_RDWR;
> > +    bdrv_flags |= bdrv_is_snapshot(bs) ? BDRV_O_SNAPSHOT : 0;
> > +    if (bdrv_open(bs, mediafile, bdrv_flags, drv) < 0) {
> > +        qerror_report(QERR_OPEN_FILE_FAILED, mediafile);
> > +        return -1;
> > +    }
> > +
> > +    return 0;
> > +}
> [...]
> 

[Qemu-devel] [RFC 08/10] QMP: Introduce the BLOCK_TRAY_OPEN and BLOCK_TRAY_CLOSE events

[Luiz Capitulino]

They are emitted when the tray is opened or closed, either by the
guest or by monitor commands.

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 QMP/qmp-events.txt |   30 ++++++++++++++++++++++++++++++
 block.c            |   22 ++++++++++++++++++++++
 monitor.c          |    6 ++++++
 monitor.h          |    2 ++
 4 files changed, 60 insertions(+), 0 deletions(-)

diff --git a/QMP/qmp-events.txt b/QMP/qmp-events.txt
index 0ce5d4e..51a93d0 100644
--- a/QMP/qmp-events.txt
+++ b/QMP/qmp-events.txt
@@ -26,6 +26,36 @@ Example:
 Note: If action is "stop", a STOP event will eventually follow the
 BLOCK_IO_ERROR event.
 
+BLOCK_TRAY_CLOSE
+----------------
+
+Emitted when a removable disk media tray is closed.
+
+Data:
+
+- "device": device name (json-string)
+
+Example:
+
+{ "event": "BLOCK_TRAY_CLOSE",
+    "data": { "device": "ide1-cd0" },
+    "timestamp": { "seconds": 1265044280, "microseconds": 450456 } }
+
+BLOCK_TRAY_OPEN
+---------------
+
+Emitted when a removable disk media tray is opened.
+
+Data:
+
+- "device": device name (json-string)
+
+Example:
+
+{ "event": "BLOCK_TRAY_OPEN",
+    "data": { "device": "ide1-cd0" },
+    "timestamp": { "seconds": 1265044230, "microseconds": 450486 } }
+
 RESET
 -----
 
diff --git a/block.c b/block.c
index 3df5a4f..9224f22 100644
--- a/block.c
+++ b/block.c
@@ -1656,6 +1656,17 @@ int bdrv_is_allocated(BlockDriverState *bs, int64_t sector_num, int nb_sectors,
     return bs->drv->bdrv_is_allocated(bs, sector_num, nb_sectors, pnum);
 }
 
+static void bdrv_eject_mon_event(const BlockDriverState *bdrv, int tray_open)
+{
+    QObject *data;
+    int event;
+
+    data = qobject_from_jsonf("{ 'device': %s }", bdrv->device_name);
+    event = tray_open ? QEVENT_BLOCK_TRAY_OPEN : QEVENT_BLOCK_TRAY_CLOSE;
+    monitor_protocol_event(event, data);
+    qobject_decref(data);
+}
+
 void bdrv_error_mon_event(const BlockDriverState *bdrv,
                           BlockMonEventAction action, int is_read)
 {
@@ -2776,6 +2787,17 @@ int bdrv_eject(BlockDriverState *bs, int eject_flag, int force)
         ret = 0;
     }
     if (ret >= 0) {
+        if (bs->tray_open != eject_flag) {
+            if (bs->device_name[0] != '\0') {
+                /*
+                 * FIXME: raw_eject() calls bdrv_eject(), which makes the
+                 * event be emitted twice. The check above will prevent that
+                 * from happening, * but we need a better way to deal with
+                 * this...
+                 */
+                bdrv_eject_mon_event(bs, eject_flag);
+            }
+        }
         bs->tray_open = eject_flag;
     }
 
diff --git a/monitor.c b/monitor.c
index f63cce0..22353b5 100644
--- a/monitor.c
+++ b/monitor.c
@@ -453,6 +453,12 @@ void monitor_protocol_event(MonitorEvent event, QObject *data)
         case QEVENT_BLOCK_IO_ERROR:
             event_name = "BLOCK_IO_ERROR";
             break;
+        case QEVENT_BLOCK_TRAY_OPEN:
+            event_name = "BLOCK_TRAY_OPEN";
+            break;
+        case QEVENT_BLOCK_TRAY_CLOSE:
+            event_name = "BLOCK_TRAY_CLOSE";
+            break;
         case QEVENT_RTC_CHANGE:
             event_name = "RTC_CHANGE";
             break;
diff --git a/monitor.h b/monitor.h
index 4f2d328..fca1e18 100644
--- a/monitor.h
+++ b/monitor.h
@@ -30,6 +30,8 @@ typedef enum MonitorEvent {
     QEVENT_VNC_INITIALIZED,
     QEVENT_VNC_DISCONNECTED,
     QEVENT_BLOCK_IO_ERROR,
+    QEVENT_BLOCK_TRAY_OPEN,
+    QEVENT_BLOCK_TRAY_CLOSE,
     QEVENT_RTC_CHANGE,
     QEVENT_WATCHDOG,
     QEVENT_SPICE_CONNECTED,
-- 
1.7.4.4

[Qemu-devel] [RFC 09/10] QMP/HMP: eject: Use blockdev-tray-open

[Luiz Capitulino]

This commit rewrites eject as a special case of the blockdev-tray-open
command. In other words, do_eject() just calls tray_open().

This brings the following behavior *changes* to both QMP and HMP:

 1. Before this commit eject was capable of closing the BlockDriverState
    associated with *any* device (ie. removable or not, by using '-f').
    Now it's only capable of closing the BlockDriverState of removable
    devices

 2. Ejecting the host's cdrom now works

 3. After the eject command is ran the tray is left open

Please, also note that the use of this command will emit the
BLOCK_TRAY_OPEN event in QMP.

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 blockdev.c |   12 ++----------
 1 files changed, 2 insertions(+), 10 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index 14c8312..36c56fd 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -787,16 +787,8 @@ int do_tray_open(Monitor *mon, const QDict *qdict, QObject **ret_data)
 
 int do_eject(Monitor *mon, const QDict *qdict, QObject **ret_data)
 {
-    BlockDriverState *bs;
-    int force = qdict_get_try_bool(qdict, "force", 0);
-    const char *filename = qdict_get_str(qdict, "device");
-
-    bs = bdrv_find(filename);
-    if (!bs) {
-        qerror_report(QERR_DEVICE_NOT_FOUND, filename);
-        return -1;
-    }
-    return eject_device(mon, bs, force);
+    return tray_open(qdict_get_str(qdict, "device"), 1,
+                     qdict_get_try_bool(qdict, "force", 0));
 }
 
 int do_block_set_passwd(Monitor *mon, const QDict *qdict,
-- 
1.7.4.4

[Qemu-devel] [RFC 10/10] QMP/HMP: change: Use QMP tray commands

[Luiz Capitulino]

This commit rewrites change in terms of blockdev-tray-open,
blockdev-media-insert and blockdev-tray-close.

There should be no visible changes in HMP or QMP, except that
the use of this command causes the BLOCK_TRAY_OPEN *and*
BLOCK_TRAY_CLOSE events to emitted.

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 blockdev.c |   43 +++++++++----------------------------------
 1 files changed, 9 insertions(+), 34 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index 36c56fd..e9edcb3 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -648,23 +648,6 @@ out:
     return ret;
 }
 
-static int eject_device(Monitor *mon, BlockDriverState *bs, int force)
-{
-    if (!force) {
-        if (!bdrv_is_removable(bs)) {
-            qerror_report(QERR_DEVICE_NOT_REMOVABLE,
-                           bdrv_get_device_name(bs));
-            return -1;
-        }
-        if (bdrv_is_locked(bs)) {
-            qerror_report(QERR_DEVICE_LOCKED, bdrv_get_device_name(bs));
-            return -1;
-        }
-    }
-    bdrv_close(bs);
-    return 0;
-}
-
 static BlockDriverState *bdrv_removable_find(const char *name)
 {
     BlockDriverState *bs;
@@ -819,30 +802,22 @@ int do_change_block(Monitor *mon, const char *device,
                     const char *filename, const char *fmt)
 {
     BlockDriverState *bs;
-    BlockDriver *drv = NULL;
-    int bdrv_flags;
 
-    bs = bdrv_find(device);
-    if (!bs) {
-        qerror_report(QERR_DEVICE_NOT_FOUND, device);
+    if (tray_open(device, 1, 0) < 0) { /* XXX: should we force? */
         return -1;
     }
-    if (fmt) {
-        drv = bdrv_find_whitelisted_format(fmt);
-        if (!drv) {
-            qerror_report(QERR_INVALID_BLOCK_FORMAT, fmt);
-            return -1;
-        }
-    }
-    if (eject_device(mon, bs, 0) < 0) {
+
+    if (media_insert(device, filename, fmt) < 0) {
         return -1;
     }
-    bdrv_flags = bdrv_is_read_only(bs) ? 0 : BDRV_O_RDWR;
-    bdrv_flags |= bdrv_is_snapshot(bs) ? BDRV_O_SNAPSHOT : 0;
-    if (bdrv_open(bs, filename, bdrv_flags, drv) < 0) {
-        qerror_report(QERR_OPEN_FILE_FAILED, filename);
+
+    if (tray_close(device) < 0) {
+        /* XXX: open the tray and remove the media? */
         return -1;
     }
+
+    bs = bdrv_find(device); /* XXX: bdrv_find() is called 4 times */
+    assert(bs != NULL);
     return monitor_read_bdrv_key_start(mon, bs, NULL, NULL);
 }
 
-- 
1.7.4.4

Re: [Qemu-devel] [RFC 10/10] QMP/HMP: change: Use QMP tray commands

[Amit Shah]

On (Fri) 03 Jun 2011 [16:04:02], Luiz Capitulino wrote:
> This commit rewrites change in terms of blockdev-tray-open,
> blockdev-media-insert and blockdev-tray-close.
> 
> There should be no visible changes in HMP or QMP, except that
> the use of this command causes the BLOCK_TRAY_OPEN *and*
> BLOCK_TRAY_CLOSE events to emitted.
> 
> Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
> ---
>  blockdev.c |   43 +++++++++----------------------------------
>  1 files changed, 9 insertions(+), 34 deletions(-)
> 
> diff --git a/blockdev.c b/blockdev.c
> index 36c56fd..e9edcb3 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -648,23 +648,6 @@ out:
>      return ret;
>  }
>  
> -static int eject_device(Monitor *mon, BlockDriverState *bs, int force)
> -{
> -    if (!force) {
> -        if (!bdrv_is_removable(bs)) {
> -            qerror_report(QERR_DEVICE_NOT_REMOVABLE,
> -                           bdrv_get_device_name(bs));
> -            return -1;
> -        }
> -        if (bdrv_is_locked(bs)) {
> -            qerror_report(QERR_DEVICE_LOCKED, bdrv_get_device_name(bs));
> -            return -1;
> -        }
> -    }
> -    bdrv_close(bs);
> -    return 0;
> -}
> -
>  static BlockDriverState *bdrv_removable_find(const char *name)
>  {
>      BlockDriverState *bs;
> @@ -819,30 +802,22 @@ int do_change_block(Monitor *mon, const char *device,
>                      const char *filename, const char *fmt)
>  {
>      BlockDriverState *bs;
> -    BlockDriver *drv = NULL;
> -    int bdrv_flags;
>  
> -    bs = bdrv_find(device);
> -    if (!bs) {
> -        qerror_report(QERR_DEVICE_NOT_FOUND, device);
> +    if (tray_open(device, 1, 0) < 0) { /* XXX: should we force? */
>          return -1;
>      }
> -    if (fmt) {
> -        drv = bdrv_find_whitelisted_format(fmt);
> -        if (!drv) {
> -            qerror_report(QERR_INVALID_BLOCK_FORMAT, fmt);
> -            return -1;
> -        }
> -    }
> -    if (eject_device(mon, bs, 0) < 0) {
> +
> +    if (media_insert(device, filename, fmt) < 0) {
>          return -1;
>      }
> -    bdrv_flags = bdrv_is_read_only(bs) ? 0 : BDRV_O_RDWR;
> -    bdrv_flags |= bdrv_is_snapshot(bs) ? BDRV_O_SNAPSHOT : 0;
> -    if (bdrv_open(bs, filename, bdrv_flags, drv) < 0) {
> -        qerror_report(QERR_OPEN_FILE_FAILED, filename);
> +
> +    if (tray_close(device) < 0) {
> +        /* XXX: open the tray and remove the media? */
>          return -1;
>      }

So if the new medium points to a non-existent file, close will fail,
and leave the tray in the open state.  A next change command will fail
because the tray is already open.  Not a good thing.


		Amit

Re: [Qemu-devel] [RFC 10/10] QMP/HMP: change: Use QMP tray commands

[Luiz Capitulino]

On Mon, 6 Jun 2011 17:18:31 +0530
Amit Shah <amit.shah@redhat.com> wrote:

> On (Fri) 03 Jun 2011 [16:04:02], Luiz Capitulino wrote:
> > This commit rewrites change in terms of blockdev-tray-open,
> > blockdev-media-insert and blockdev-tray-close.
> > 
> > There should be no visible changes in HMP or QMP, except that
> > the use of this command causes the BLOCK_TRAY_OPEN *and*
> > BLOCK_TRAY_CLOSE events to emitted.
> > 
> > Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
> > ---
> >  blockdev.c |   43 +++++++++----------------------------------
> >  1 files changed, 9 insertions(+), 34 deletions(-)
> > 
> > diff --git a/blockdev.c b/blockdev.c
> > index 36c56fd..e9edcb3 100644
> > --- a/blockdev.c
> > +++ b/blockdev.c
> > @@ -648,23 +648,6 @@ out:
> >      return ret;
> >  }
> >  
> > -static int eject_device(Monitor *mon, BlockDriverState *bs, int force)
> > -{
> > -    if (!force) {
> > -        if (!bdrv_is_removable(bs)) {
> > -            qerror_report(QERR_DEVICE_NOT_REMOVABLE,
> > -                           bdrv_get_device_name(bs));
> > -            return -1;
> > -        }
> > -        if (bdrv_is_locked(bs)) {
> > -            qerror_report(QERR_DEVICE_LOCKED, bdrv_get_device_name(bs));
> > -            return -1;
> > -        }
> > -    }
> > -    bdrv_close(bs);
> > -    return 0;
> > -}
> > -
> >  static BlockDriverState *bdrv_removable_find(const char *name)
> >  {
> >      BlockDriverState *bs;
> > @@ -819,30 +802,22 @@ int do_change_block(Monitor *mon, const char *device,
> >                      const char *filename, const char *fmt)
> >  {
> >      BlockDriverState *bs;
> > -    BlockDriver *drv = NULL;
> > -    int bdrv_flags;
> >  
> > -    bs = bdrv_find(device);
> > -    if (!bs) {
> > -        qerror_report(QERR_DEVICE_NOT_FOUND, device);
> > +    if (tray_open(device, 1, 0) < 0) { /* XXX: should we force? */
> >          return -1;
> >      }
> > -    if (fmt) {
> > -        drv = bdrv_find_whitelisted_format(fmt);
> > -        if (!drv) {
> > -            qerror_report(QERR_INVALID_BLOCK_FORMAT, fmt);
> > -            return -1;
> > -        }
> > -    }
> > -    if (eject_device(mon, bs, 0) < 0) {
> > +
> > +    if (media_insert(device, filename, fmt) < 0) {
> >          return -1;
> >      }
> > -    bdrv_flags = bdrv_is_read_only(bs) ? 0 : BDRV_O_RDWR;
> > -    bdrv_flags |= bdrv_is_snapshot(bs) ? BDRV_O_SNAPSHOT : 0;
> > -    if (bdrv_open(bs, filename, bdrv_flags, drv) < 0) {
> > -        qerror_report(QERR_OPEN_FILE_FAILED, filename);
> > +
> > +    if (tray_close(device) < 0) {
> > +        /* XXX: open the tray and remove the media? */
> >          return -1;
> >      }
> 
> So if the new medium points to a non-existent file, close will fail,

media_insert() will and tray_close() won't be called. That's what you
meant, right?

> and leave the tray in the open state.  A next change command will fail
> because the tray is already open.  Not a good thing.

tray_open() shouldn't fail if the tray is already open.

Re: [Qemu-devel] [RFC 10/10] QMP/HMP: change: Use QMP tray commands

[Amit Shah]

On (Mon) 06 Jun 2011 [11:45:06], Luiz Capitulino wrote:
> On Mon, 6 Jun 2011 17:18:31 +0530
> Amit Shah <amit.shah@redhat.com> wrote:
> 
> > On (Fri) 03 Jun 2011 [16:04:02], Luiz Capitulino wrote:
> > > This commit rewrites change in terms of blockdev-tray-open,
> > > blockdev-media-insert and blockdev-tray-close.
> > > 
> > > There should be no visible changes in HMP or QMP, except that
> > > the use of this command causes the BLOCK_TRAY_OPEN *and*
> > > BLOCK_TRAY_CLOSE events to emitted.
> > > 
> > > Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
> > > ---
> > >  blockdev.c |   43 +++++++++----------------------------------
> > >  1 files changed, 9 insertions(+), 34 deletions(-)
> > > 
> > > diff --git a/blockdev.c b/blockdev.c
> > > index 36c56fd..e9edcb3 100644
> > > --- a/blockdev.c
> > > +++ b/blockdev.c
> > > @@ -648,23 +648,6 @@ out:
> > >      return ret;
> > >  }
> > >  
> > > -static int eject_device(Monitor *mon, BlockDriverState *bs, int force)
> > > -{
> > > -    if (!force) {
> > > -        if (!bdrv_is_removable(bs)) {
> > > -            qerror_report(QERR_DEVICE_NOT_REMOVABLE,
> > > -                           bdrv_get_device_name(bs));
> > > -            return -1;
> > > -        }
> > > -        if (bdrv_is_locked(bs)) {
> > > -            qerror_report(QERR_DEVICE_LOCKED, bdrv_get_device_name(bs));
> > > -            return -1;
> > > -        }
> > > -    }
> > > -    bdrv_close(bs);
> > > -    return 0;
> > > -}
> > > -
> > >  static BlockDriverState *bdrv_removable_find(const char *name)
> > >  {
> > >      BlockDriverState *bs;
> > > @@ -819,30 +802,22 @@ int do_change_block(Monitor *mon, const char *device,
> > >                      const char *filename, const char *fmt)
> > >  {
> > >      BlockDriverState *bs;
> > > -    BlockDriver *drv = NULL;
> > > -    int bdrv_flags;
> > >  
> > > -    bs = bdrv_find(device);
> > > -    if (!bs) {
> > > -        qerror_report(QERR_DEVICE_NOT_FOUND, device);
> > > +    if (tray_open(device, 1, 0) < 0) { /* XXX: should we force? */
> > >          return -1;
> > >      }
> > > -    if (fmt) {
> > > -        drv = bdrv_find_whitelisted_format(fmt);
> > > -        if (!drv) {
> > > -            qerror_report(QERR_INVALID_BLOCK_FORMAT, fmt);
> > > -            return -1;
> > > -        }
> > > -    }
> > > -    if (eject_device(mon, bs, 0) < 0) {
> > > +
> > > +    if (media_insert(device, filename, fmt) < 0) {
> > >          return -1;
> > >      }
> > > -    bdrv_flags = bdrv_is_read_only(bs) ? 0 : BDRV_O_RDWR;
> > > -    bdrv_flags |= bdrv_is_snapshot(bs) ? BDRV_O_SNAPSHOT : 0;
> > > -    if (bdrv_open(bs, filename, bdrv_flags, drv) < 0) {
> > > -        qerror_report(QERR_OPEN_FILE_FAILED, filename);
> > > +
> > > +    if (tray_close(device) < 0) {
> > > +        /* XXX: open the tray and remove the media? */
> > >          return -1;
> > >      }
> > 
> > So if the new medium points to a non-existent file, close will fail,
> 
> media_insert() will and tray_close() won't be called. That's what you
> meant, right?

Yes.

> > and leave the tray in the open state.  A next change command will fail
> > because the tray is already open.  Not a good thing.
> 
> tray_open() shouldn't fail if the tray is already open.

Ah, that's right.

		Amit

Re: [Qemu-devel] [RFC 00/10]: QMP/HMP: Introduce tray handling commands

[Markus Armbruster]

Luiz Capitulino <lcapitulino@redhat.com> writes:

> In a recent discussion on the mailing list regarding the introduction of
> the BLOCK_TRAY_OPEN and BLOCK_TRAY_CLOSE events[1], it was mentioned that
> we need to fix the eject command and maybe introduce new commands first.
>
> Here's a my proposal.
>
> This series introduces three new commands:
>
>  o blockdev-tray-open: opens the drive tray. Also Supports removing the inserted
>    media. The BLOCK_TRAY_OPEN event is emitted if this command succeeds.

Conflates tray control with media control.  Hmm.

>  o blockdev-tray-close: closes a drive tray. The BLOCK_TRAY_CLOSE event is
>    emitted.
>  o blockdev-media-insert: Inserts a media in the tray. The tray must empty
>    and already opened. No event is emitted.

What about separating tray control and media control like this:

* Tray control: either blockdev-tray-open and blockdev-tray-close, or a
  single blockdev-tray with a boolean argument.

* Media control: blockdev-media to change media.  Tray must be open, may
  or may not contain media.  Arguments specify new media, or no media.

Re: [Qemu-devel] [RFC 00/10]: QMP/HMP: Introduce tray handling commands

[Luiz Capitulino]

On Mon, 06 Jun 2011 15:31:35 +0200
Markus Armbruster <armbru@redhat.com> wrote:

> Luiz Capitulino <lcapitulino@redhat.com> writes:
> 
> > In a recent discussion on the mailing list regarding the introduction of
> > the BLOCK_TRAY_OPEN and BLOCK_TRAY_CLOSE events[1], it was mentioned that
> > we need to fix the eject command and maybe introduce new commands first.
> >
> > Here's a my proposal.
> >
> > This series introduces three new commands:
> >
> >  o blockdev-tray-open: opens the drive tray. Also Supports removing the inserted
> >    media. The BLOCK_TRAY_OPEN event is emitted if this command succeeds.
> 
> Conflates tray control with media control.  Hmm.
> 
> >  o blockdev-tray-close: closes a drive tray. The BLOCK_TRAY_CLOSE event is
> >    emitted.
> >  o blockdev-media-insert: Inserts a media in the tray. The tray must empty
> >    and already opened. No event is emitted.
> 
> What about separating tray control and media control like this:
> 
> * Tray control: either blockdev-tray-open and blockdev-tray-close, or a
>   single blockdev-tray with a boolean argument.

This might sound like a simple thing, but sometimes I'm really undecided when
something should be a new argument or a new command.

Following Anthony's idea that it can be helpful to think in a C interface,
I slightly prefer this:

 blockdev_tray_open(device);

 ...

 blockdev_tray_close(device);

Than this:

 blockdev_tray(device, TRAY_OPEN);

...

 blockdev_tray(device, TRAY_CLOSE);

> * Media control: blockdev-media to change media.  Tray must be open, may
>   or may not contain media.  Arguments specify new media, or no media.

Yeah, maybe it's better to drop the 'remove' flag from the open command.
But then I think I prefer having blockdev-media-remove and
blockdev-media-insert instead of a single blockdev-media command.

Re: [Qemu-devel] [RFC 00/10]: QMP/HMP: Introduce tray handling commands

[Osier Yang]

On 2011年06月04日 03:03, Luiz Capitulino wrote:
> In a recent discussion on the mailing list regarding the introduction of
> the BLOCK_TRAY_OPEN and BLOCK_TRAY_CLOSE events[1], it was mentioned that
> we need to fix the eject command and maybe introduce new commands first.
>
> Here's a my proposal.
>
> This series introduces three new commands:
>
>   o blockdev-tray-open: opens the drive tray. Also Supports removing the inserted
>     media. The BLOCK_TRAY_OPEN event is emitted if this command succeeds.
>   o blockdev-tray-close: closes a drive tray. The BLOCK_TRAY_CLOSE event is
>     emitted.
>   o blockdev-media-insert: Inserts a media in the tray. The tray must empty
>     and already opened. No event is emitted.
>
> The existing 'eject' and 'change' commands are completely rewriten in terms
> of the new commands. Besides fixing some bad behaviors, this also makes it
> possible for the events to be automtically emitted.
>
> Ejecting a device inside the guest (or closing its tray) also causes the
> events to be emitted.
>
> Everything which applies for the virtual tray seems to work as expected for
> the host's tray. But more testing is needed.
>

Hi, Luiz

Does it still plan to introduce new commands for tray & media
management? and by the way, I'd agree with Markus more for
seperating the commands for tray and media management. It
makes thing simpler and more clear from a user's point of
view.

Regards,
Osier

Re: [Qemu-devel] [RFC 00/10]: QMP/HMP: Introduce tray handling commands

[Luiz Capitulino]

On Tue, 20 Dec 2011 14:49:47 +0800
Osier Yang <jyang@redhat.com> wrote:

> On 2011年06月04日 03:03, Luiz Capitulino wrote:
> > In a recent discussion on the mailing list regarding the introduction of
> > the BLOCK_TRAY_OPEN and BLOCK_TRAY_CLOSE events[1], it was mentioned that
> > we need to fix the eject command and maybe introduce new commands first.
> >
> > Here's a my proposal.
> >
> > This series introduces three new commands:
> >
> >   o blockdev-tray-open: opens the drive tray. Also Supports removing the inserted
> >     media. The BLOCK_TRAY_OPEN event is emitted if this command succeeds.
> >   o blockdev-tray-close: closes a drive tray. The BLOCK_TRAY_CLOSE event is
> >     emitted.
> >   o blockdev-media-insert: Inserts a media in the tray. The tray must empty
> >     and already opened. No event is emitted.
> >
> > The existing 'eject' and 'change' commands are completely rewriten in terms
> > of the new commands. Besides fixing some bad behaviors, this also makes it
> > possible for the events to be automtically emitted.
> >
> > Ejecting a device inside the guest (or closing its tray) also causes the
> > events to be emitted.
> >
> > Everything which applies for the virtual tray seems to work as expected for
> > the host's tray. But more testing is needed.
> >
> 
> Hi, Luiz
> 
> Does it still plan to introduce new commands for tray & media
> management?

Yes, but I'm not exactly sure when. Do you need it?

> and by the way, I'd agree with Markus more for
> seperating the commands for tray and media management. It
> makes thing simpler and more clear from a user's point of
> view.
> 
> Regards,
> Osier
> 

Re: [Qemu-devel] [RFC 00/10]: QMP/HMP: Introduce tray handling commands

[Osier Yang]

On 2012年01月04日 03:28, Luiz Capitulino wrote:
> On Tue, 20 Dec 2011 14:49:47 +0800
> Osier Yang<jyang@redhat.com>  wrote:
>
>> On 2011年06月04日 03:03, Luiz Capitulino wrote:
>>> In a recent discussion on the mailing list regarding the introduction of
>>> the BLOCK_TRAY_OPEN and BLOCK_TRAY_CLOSE events[1], it was mentioned that
>>> we need to fix the eject command and maybe introduce new commands first.
>>>
>>> Here's a my proposal.
>>>
>>> This series introduces three new commands:
>>>
>>>    o blockdev-tray-open: opens the drive tray. Also Supports removing the inserted
>>>      media. The BLOCK_TRAY_OPEN event is emitted if this command succeeds.
>>>    o blockdev-tray-close: closes a drive tray. The BLOCK_TRAY_CLOSE event is
>>>      emitted.
>>>    o blockdev-media-insert: Inserts a media in the tray. The tray must empty
>>>      and already opened. No event is emitted.
>>>
>>> The existing 'eject' and 'change' commands are completely rewriten in terms
>>> of the new commands. Besides fixing some bad behaviors, this also makes it
>>> possible for the events to be automtically emitted.
>>>
>>> Ejecting a device inside the guest (or closing its tray) also causes the
>>> events to be emitted.
>>>
>>> Everything which applies for the virtual tray seems to work as expected for
>>> the host's tray. But more testing is needed.
>>>
>>
>> Hi, Luiz
>>
>> Does it still plan to introduce new commands for tray&  media
>> management?
>
> Yes, but I'm not exactly sure when. Do you need it?

Not necessary, just wanted to confirm if I could wait for a while
to get those support in together when creating similiar support
in libvirt layer.

>
>> and by the way, I'd agree with Markus more for
>> seperating the commands for tray and media management. It
>> makes thing simpler and more clear from a user's point of
>> view.
>>
>> Regards,
>> Osier
>>
>
>