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

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

[Luiz Capitulino]

The motivation for this event is that clients can get confused if removable
media is ejected by the guest (or by a human user).

You'll find detailed documentation in patch 2/3 and the actual implementation
in patch 3/3.

Thanks.

 QMP/qmp-events.txt |   18 ++++++++++++++++++
 block.c            |   16 ++++++++++++++--
 block.h            |    5 +++--
 blockdev.c         |    5 +++++
 hw/ide/core.c      |    6 +++---
 hw/scsi-disk.c     |    6 +++---
 hw/virtio-blk.c    |    6 +++---
 monitor.c          |    3 +++
 monitor.h          |    1 +
 9 files changed, 53 insertions(+), 13 deletions(-)

[Qemu-devel] [PATCH 1/3] 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 effa86f..f5014cf 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 da7d39c..1f58eab 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 397b9d6..687d34c 100644
--- a/hw/scsi-disk.c
+++ b/hw/scsi-disk.c
@@ -231,7 +231,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;
     }
 
@@ -241,7 +241,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) {
@@ -249,7 +249,7 @@ static int scsi_handle_rw_error(SCSIDiskReq *r, int error, int type)
         }
         scsi_command_complete(r, CHECK_CONDITION,
                 HARDWARE_ERROR);
-        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] [PATCH 2/3] QMP: Add BLOCK_MEDIA_EJECT event documentation

[Luiz Capitulino]

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 QMP/qmp-events.txt |   18 ++++++++++++++++++
 1 files changed, 18 insertions(+), 0 deletions(-)

diff --git a/QMP/qmp-events.txt b/QMP/qmp-events.txt
index 0ce5d4e..d53c129 100644
--- a/QMP/qmp-events.txt
+++ b/QMP/qmp-events.txt
@@ -1,6 +1,24 @@
                    QEMU Monitor Protocol Events
                    ============================
 
+BLOCK_MEDIA_EJECT
+-----------------
+
+Emitted when a removable disk media (such as a CDROM or floppy) is ejected.
+
+Data:
+
+- "device": device name (json-string)
+
+Example:
+
+{ "event": "BLOCK_MEDIA_EJECT",
+    "data": { "device": "ide1-cd0" },
+    "timestamp": { "seconds": 1265044230, "microseconds": 450486 } }
+
+NOTE: A disk media can be ejected by the guest or by monitor commands (such
+as "eject" and "change")
+
 BLOCK_IO_ERROR
 --------------
 
-- 
1.7.4.4

Re: [Qemu-devel] [PATCH 2/3] QMP: Add BLOCK_MEDIA_EJECT event documentation

[Kevin Wolf]

Am 27.05.2011 21:31, schrieb Luiz Capitulino:
> Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
> ---
>  QMP/qmp-events.txt |   18 ++++++++++++++++++
>  1 files changed, 18 insertions(+), 0 deletions(-)
> 
> diff --git a/QMP/qmp-events.txt b/QMP/qmp-events.txt
> index 0ce5d4e..d53c129 100644
> --- a/QMP/qmp-events.txt
> +++ b/QMP/qmp-events.txt
> @@ -1,6 +1,24 @@
>                     QEMU Monitor Protocol Events
>                     ============================
>  
> +BLOCK_MEDIA_EJECT
> +-----------------
> +
> +Emitted when a removable disk media (such as a CDROM or floppy) is ejected.
> +
> +Data:
> +
> +- "device": device name (json-string)
> +
> +Example:
> +
> +{ "event": "BLOCK_MEDIA_EJECT",
> +    "data": { "device": "ide1-cd0" },
> +    "timestamp": { "seconds": 1265044230, "microseconds": 450486 } }
> +
> +NOTE: A disk media can be ejected by the guest or by monitor commands (such
> +as "eject" and "change")

The monitor command 'eject' already caused a lot of confusion, please
don't make the same mistake in this event name. Even though I know more
or less what eject can mean in qemu, I'm not sure what "eject" means for
you in the context of this event.

The 'eject' monitor command means that the image is closed and the
BlockDriverState doesn't point to any image file any more. And then
there's bdrv_eject(), which is what the guest can do, and it's about the
virtual tray status.

Having a single event for both doesn't make sense because they are
fundamentally different. Something like BLOCKDEV_CLOSE would be the
right name for the 'eject' monitor command and maybe something like
BLOCKDEV_TRAY_STATUS for the other one.

Kevin

Re: [Qemu-devel] [PATCH 2/3] QMP: Add BLOCK_MEDIA_EJECT event documentation

[Luiz Capitulino]

On Mon, 30 May 2011 10:46:07 +0200
Kevin Wolf <kwolf@redhat.com> wrote:

> Am 27.05.2011 21:31, schrieb Luiz Capitulino:
> > Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
> > ---
> >  QMP/qmp-events.txt |   18 ++++++++++++++++++
> >  1 files changed, 18 insertions(+), 0 deletions(-)
> > 
> > diff --git a/QMP/qmp-events.txt b/QMP/qmp-events.txt
> > index 0ce5d4e..d53c129 100644
> > --- a/QMP/qmp-events.txt
> > +++ b/QMP/qmp-events.txt
> > @@ -1,6 +1,24 @@
> >                     QEMU Monitor Protocol Events
> >                     ============================
> >  
> > +BLOCK_MEDIA_EJECT
> > +-----------------
> > +
> > +Emitted when a removable disk media (such as a CDROM or floppy) is ejected.
> > +
> > +Data:
> > +
> > +- "device": device name (json-string)
> > +
> > +Example:
> > +
> > +{ "event": "BLOCK_MEDIA_EJECT",
> > +    "data": { "device": "ide1-cd0" },
> > +    "timestamp": { "seconds": 1265044230, "microseconds": 450486 } }
> > +
> > +NOTE: A disk media can be ejected by the guest or by monitor commands (such
> > +as "eject" and "change")
> 
> The monitor command 'eject' already caused a lot of confusion, please
> don't make the same mistake in this event name. Even though I know more
> or less what eject can mean in qemu, I'm not sure what "eject" means for
> you in the context of this event.

I'll change it to report the tray status instead, as suggested by Markus.

> The 'eject' monitor command means that the image is closed and the
> BlockDriverState doesn't point to any image file any more. And then
> there's bdrv_eject(), which is what the guest can do, and it's about the
> virtual tray status.
> 
> Having a single event for both doesn't make sense because they are
> fundamentally different. Something like BLOCKDEV_CLOSE would be the
> right name for the 'eject' monitor command and maybe something like
> BLOCKDEV_TRAY_STATUS for the other one.

Well, there are two problems here. First, we shouldn't report something
like BLOCKDEV_CLOSE because closing a BlockDriverState is something
internal to qemu that clients/users shouldn't know about. The second
problem is that, unfortunately, clients do use "eject" to eject a
removable media. Actually it's _the_ interface available for that, so
not emitting the event there will probably confuse clients as much as
not having the event at all.

Maybe, a better solution is to fix eject to really eject the media
instead of closing its BlockDriverState and drop the event from the change
command.

Re: [Qemu-devel] [PATCH 2/3] QMP: Add BLOCK_MEDIA_EJECT event documentation

[Markus Armbruster]

Luiz Capitulino <lcapitulino@redhat.com> writes:

> On Mon, 30 May 2011 10:46:07 +0200
> Kevin Wolf <kwolf@redhat.com> wrote:
>
>> Am 27.05.2011 21:31, schrieb Luiz Capitulino:
>> > Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
>> > ---
>> >  QMP/qmp-events.txt |   18 ++++++++++++++++++
>> >  1 files changed, 18 insertions(+), 0 deletions(-)
>> > 
>> > diff --git a/QMP/qmp-events.txt b/QMP/qmp-events.txt
>> > index 0ce5d4e..d53c129 100644
>> > --- a/QMP/qmp-events.txt
>> > +++ b/QMP/qmp-events.txt
>> > @@ -1,6 +1,24 @@
>> >                     QEMU Monitor Protocol Events
>> >                     ============================
>> >  
>> > +BLOCK_MEDIA_EJECT
>> > +-----------------
>> > +
>> > +Emitted when a removable disk media (such as a CDROM or floppy) is ejected.
>> > +
>> > +Data:
>> > +
>> > +- "device": device name (json-string)
>> > +
>> > +Example:
>> > +
>> > +{ "event": "BLOCK_MEDIA_EJECT",
>> > +    "data": { "device": "ide1-cd0" },
>> > +    "timestamp": { "seconds": 1265044230, "microseconds": 450486 } }
>> > +
>> > +NOTE: A disk media can be ejected by the guest or by monitor commands (such
>> > +as "eject" and "change")
>> 
>> The monitor command 'eject' already caused a lot of confusion, please
>> don't make the same mistake in this event name. Even though I know more
>> or less what eject can mean in qemu, I'm not sure what "eject" means for
>> you in the context of this event.
>
> I'll change it to report the tray status instead, as suggested by Markus.
>
>> The 'eject' monitor command means that the image is closed and the
>> BlockDriverState doesn't point to any image file any more. And then
>> there's bdrv_eject(), which is what the guest can do, and it's about the
>> virtual tray status.
>> 
>> Having a single event for both doesn't make sense because they are
>> fundamentally different. Something like BLOCKDEV_CLOSE would be the
>> right name for the 'eject' monitor command and maybe something like
>> BLOCKDEV_TRAY_STATUS for the other one.
>
> Well, there are two problems here. First, we shouldn't report something
> like BLOCKDEV_CLOSE because closing a BlockDriverState is something
> internal to qemu that clients/users shouldn't know about. The second
> problem is that, unfortunately, clients do use "eject" to eject a
> removable media. Actually it's _the_ interface available for that, so
> not emitting the event there will probably confuse clients as much as
> not having the event at all.
>
> Maybe, a better solution is to fix eject to really eject the media
> instead of closing its BlockDriverState and drop the event from the change
> command.

Monitor command "eject" conflates three actions: open tray, remove media
(if any), close tray.

Monitor command "change" conflates four actions: open tray, remove media
(if any), insert media, close tray.

Except they don't really move the tray in a guest-visible manner.  They
teleport the media.  I figure that should be changed.

Re: [Qemu-devel] [PATCH 2/3] QMP: Add BLOCK_MEDIA_EJECT event documentation

[Amit Shah]

On (Mon) 30 May 2011 [16:54:22], Markus Armbruster wrote:
> Luiz Capitulino <lcapitulino@redhat.com> writes:

...

> >> The monitor command 'eject' already caused a lot of confusion, please
> >> don't make the same mistake in this event name. Even though I know more
> >> or less what eject can mean in qemu, I'm not sure what "eject" means for
> >> you in the context of this event.
> >
> > I'll change it to report the tray status instead, as suggested by Markus.
> >
> >> The 'eject' monitor command means that the image is closed and the
> >> BlockDriverState doesn't point to any image file any more. And then
> >> there's bdrv_eject(), which is what the guest can do, and it's about the
> >> virtual tray status.
> >> 
> >> Having a single event for both doesn't make sense because they are
> >> fundamentally different. Something like BLOCKDEV_CLOSE would be the
> >> right name for the 'eject' monitor command and maybe something like
> >> BLOCKDEV_TRAY_STATUS for the other one.
> >
> > Well, there are two problems here. First, we shouldn't report something
> > like BLOCKDEV_CLOSE because closing a BlockDriverState is something
> > internal to qemu that clients/users shouldn't know about. The second
> > problem is that, unfortunately, clients do use "eject" to eject a
> > removable media. Actually it's _the_ interface available for that, so
> > not emitting the event there will probably confuse clients as much as
> > not having the event at all.
> >
> > Maybe, a better solution is to fix eject to really eject the media
> > instead of closing its BlockDriverState and drop the event from the change
> > command.
> 
> Monitor command "eject" conflates three actions: open tray, remove media
> (if any), close tray.
> 
> Monitor command "change" conflates four actions: open tray, remove media
> (if any), insert media, close tray.
> 
> Except they don't really move the tray in a guest-visible manner.  They
> teleport the media.  I figure that should be changed.

Agreed.  We should be able to report these events to clients as well
as guests.

		Amit

Re: [Qemu-devel] [PATCH 2/3] QMP: Add BLOCK_MEDIA_EJECT event documentation

[Kevin Wolf]

Am 30.05.2011 16:21, schrieb Luiz Capitulino:
> On Mon, 30 May 2011 10:46:07 +0200
> Kevin Wolf <kwolf@redhat.com> wrote:
> 
>> Am 27.05.2011 21:31, schrieb Luiz Capitulino:
>>> Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
>>> ---
>>>  QMP/qmp-events.txt |   18 ++++++++++++++++++
>>>  1 files changed, 18 insertions(+), 0 deletions(-)
>>>
>>> diff --git a/QMP/qmp-events.txt b/QMP/qmp-events.txt
>>> index 0ce5d4e..d53c129 100644
>>> --- a/QMP/qmp-events.txt
>>> +++ b/QMP/qmp-events.txt
>>> @@ -1,6 +1,24 @@
>>>                     QEMU Monitor Protocol Events
>>>                     ============================
>>>  
>>> +BLOCK_MEDIA_EJECT
>>> +-----------------
>>> +
>>> +Emitted when a removable disk media (such as a CDROM or floppy) is ejected.
>>> +
>>> +Data:
>>> +
>>> +- "device": device name (json-string)
>>> +
>>> +Example:
>>> +
>>> +{ "event": "BLOCK_MEDIA_EJECT",
>>> +    "data": { "device": "ide1-cd0" },
>>> +    "timestamp": { "seconds": 1265044230, "microseconds": 450486 } }
>>> +
>>> +NOTE: A disk media can be ejected by the guest or by monitor commands (such
>>> +as "eject" and "change")
>>
>> The monitor command 'eject' already caused a lot of confusion, please
>> don't make the same mistake in this event name. Even though I know more
>> or less what eject can mean in qemu, I'm not sure what "eject" means for
>> you in the context of this event.
> 
> I'll change it to report the tray status instead, as suggested by Markus.
> 
>> The 'eject' monitor command means that the image is closed and the
>> BlockDriverState doesn't point to any image file any more. And then
>> there's bdrv_eject(), which is what the guest can do, and it's about the
>> virtual tray status.
>>
>> Having a single event for both doesn't make sense because they are
>> fundamentally different. Something like BLOCKDEV_CLOSE would be the
>> right name for the 'eject' monitor command and maybe something like
>> BLOCKDEV_TRAY_STATUS for the other one.
> 
> Well, there are two problems here. First, we shouldn't report something
> like BLOCKDEV_CLOSE because closing a BlockDriverState is something
> internal to qemu that clients/users shouldn't know about.

Of course they know about it. After all, it was them who created the
BlockDriverState using -drive or drive_add (or eventually blockdev_add).

Anyway, I'm not saying that there's a good use case for BLOCKDEV_CLOSE,
it might be completely useless. I'm just saying that we must not mix it
with the tray status event.

> The second
> problem is that, unfortunately, clients do use "eject" to eject a
> removable media. Actually it's _the_ interface available for that, so
> not emitting the event there will probably confuse clients as much as
> not having the event at all.
> 
> Maybe, a better solution is to fix eject to really eject the media
> instead of closing its BlockDriverState and drop the event from the change
> command.

Hm, it would surely change the behaviour which means that we have to be
careful with it. But the change makes some sense to me. If we do this
change, I think we should also add a command for closing the tray, so
that you get access to the image again.

Kevin

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

[Luiz Capitulino]

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

Please, note the following details:

 o The event should be emitted only by devices which support the
   eject operation, which currently are: CDROMs (IDE and SCSI)
   and floppies

 o Human monitor commands "eject" and "change" also cause the
   event to be emitted

 o The event is only emitted when there's a tray transition from
   closed to opened. To implement this in the human monitor, we
   only emit the event if the device is removable and a media is
   present

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 block.c    |   12 ++++++++++++
 block.h    |    1 +
 blockdev.c |    5 +++++
 monitor.c  |    3 +++
 monitor.h  |    1 +
 5 files changed, 22 insertions(+), 0 deletions(-)

diff --git a/block.c b/block.c
index f5014cf..dbe813b 100644
--- a/block.c
+++ b/block.c
@@ -1656,6 +1656,15 @@ int bdrv_is_allocated(BlockDriverState *bs, int64_t sector_num, int nb_sectors,
     return bs->drv->bdrv_is_allocated(bs, sector_num, nb_sectors, pnum);
 }
 
+void bdrv_eject_mon_event(const BlockDriverState *bdrv)
+{
+    QObject *data;
+
+    data = qobject_from_jsonf("{ 'device': %s }", bdrv->device_name);
+    monitor_protocol_event(QEVENT_BLOCK_MEDIA_EJECT, data);
+    qobject_decref(data);
+}
+
 void bdrv_error_mon_event(const BlockDriverState *bdrv,
                           BlockMonEventAction action, int is_read)
 {
@@ -2770,6 +2779,9 @@ int bdrv_eject(BlockDriverState *bs, int eject_flag)
         ret = 0;
     }
     if (ret >= 0) {
+        if (eject_flag && !bs->tray_open) {
+            bdrv_eject_mon_event(bs);
+        }
         bs->tray_open = eject_flag;
     }
 
diff --git a/block.h b/block.h
index 1f58eab..e4053dd 100644
--- a/block.h
+++ b/block.h
@@ -50,6 +50,7 @@ typedef enum {
     BDRV_ACTION_REPORT, BDRV_ACTION_IGNORE, BDRV_ACTION_STOP
 } BlockMonEventAction;
 
+void bdrv_eject_mon_event(const BlockDriverState *bdrv);
 void bdrv_error_mon_event(const BlockDriverState *bdrv,
                           BlockMonEventAction action, int is_read);
 void bdrv_info_print(Monitor *mon, const QObject *data);
diff --git a/blockdev.c b/blockdev.c
index 6e0eb83..5fd0043 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -661,6 +661,11 @@ static int eject_device(Monitor *mon, BlockDriverState *bs, int force)
             return -1;
         }
     }
+
+    if (bdrv_is_removable(bs) && bdrv_is_inserted(bs)) {
+        bdrv_eject_mon_event(bs);
+    }
+
     bdrv_close(bs);
     return 0;
 }
diff --git a/monitor.c b/monitor.c
index f63cce0..4a81467 100644
--- a/monitor.c
+++ b/monitor.c
@@ -450,6 +450,9 @@ void monitor_protocol_event(MonitorEvent event, QObject *data)
         case QEVENT_VNC_DISCONNECTED:
             event_name = "VNC_DISCONNECTED";
             break;
+        case QEVENT_BLOCK_MEDIA_EJECT:
+            event_name = "BLOCK_MEDIA_EJECT";
+            break;
         case QEVENT_BLOCK_IO_ERROR:
             event_name = "BLOCK_IO_ERROR";
             break;
diff --git a/monitor.h b/monitor.h
index 4f2d328..7a04137 100644
--- a/monitor.h
+++ b/monitor.h
@@ -29,6 +29,7 @@ typedef enum MonitorEvent {
     QEVENT_VNC_CONNECTED,
     QEVENT_VNC_INITIALIZED,
     QEVENT_VNC_DISCONNECTED,
+    QEVENT_BLOCK_MEDIA_EJECT,
     QEVENT_BLOCK_IO_ERROR,
     QEVENT_RTC_CHANGE,
     QEVENT_WATCHDOG,
-- 
1.7.4.4

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

[Markus Armbruster]

Luiz Capitulino <lcapitulino@redhat.com> writes:

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

I'd squash PATCH 2+3.

>
> Please, note the following details:
>
>  o The event should be emitted only by devices which support the
>    eject operation, which currently are: CDROMs (IDE and SCSI)
>    and floppies
>
>  o Human monitor commands "eject" and "change" also cause the
>    event to be emitted
>
>  o The event is only emitted when there's a tray transition from
>    closed to opened. To implement this in the human monitor, we
>    only emit the event if the device is removable and a media is
>    present

Rationale?

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

>
> Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
> ---
>  block.c    |   12 ++++++++++++
>  block.h    |    1 +
>  blockdev.c |    5 +++++
>  monitor.c  |    3 +++
>  monitor.h  |    1 +
>  5 files changed, 22 insertions(+), 0 deletions(-)
>

Copied from PATCH 2 for review purposes:

  diff --git a/QMP/qmp-events.txt b/QMP/qmp-events.txt
  index 0ce5d4e..d53c129 100644
  --- a/QMP/qmp-events.txt
  +++ b/QMP/qmp-events.txt
  @@ -1,6 +1,24 @@
                      QEMU Monitor Protocol Events
                      ============================

  +BLOCK_MEDIA_EJECT
  +-----------------
  +
  +Emitted when a removable disk media (such as a CDROM or floppy) is ejected.
  +
  +Data:
  +
  +- "device": device name (json-string)
  +
  +Example:
  +
  +{ "event": "BLOCK_MEDIA_EJECT",
  +    "data": { "device": "ide1-cd0" },
  +    "timestamp": { "seconds": 1265044230, "microseconds": 450486 } }
  +
  +NOTE: A disk media can be ejected by the guest or by monitor commands (such
  +as "eject" and "change")
  +
   BLOCK_IO_ERROR
   --------------

The event reports "tray opening".  Do we need one for "tray closing" as
well?

> diff --git a/block.c b/block.c
> index f5014cf..dbe813b 100644
> --- a/block.c
> +++ b/block.c
> @@ -1656,6 +1656,15 @@ int bdrv_is_allocated(BlockDriverState *bs, int64_t sector_num, int nb_sectors,
>      return bs->drv->bdrv_is_allocated(bs, sector_num, nb_sectors, pnum);
>  }
>  
> +void bdrv_eject_mon_event(const BlockDriverState *bdrv)
> +{
> +    QObject *data;
> +
> +    data = qobject_from_jsonf("{ 'device': %s }", bdrv->device_name);
> +    monitor_protocol_event(QEVENT_BLOCK_MEDIA_EJECT, data);
> +    qobject_decref(data);
> +}
> +
>  void bdrv_error_mon_event(const BlockDriverState *bdrv,
>                            BlockMonEventAction action, int is_read)
>  {
> @@ -2770,6 +2779,9 @@ int bdrv_eject(BlockDriverState *bs, int eject_flag)
>          ret = 0;
>      }
>      if (ret >= 0) {
> +        if (eject_flag && !bs->tray_open) {
> +            bdrv_eject_mon_event(bs);
> +        }
>          bs->tray_open = eject_flag;
>      }
>  

This covers guest-initiated eject.

The event is suppressed when the tray is already open.

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

> diff --git a/block.h b/block.h
> index 1f58eab..e4053dd 100644
> --- a/block.h
> +++ b/block.h
> @@ -50,6 +50,7 @@ typedef enum {
>      BDRV_ACTION_REPORT, BDRV_ACTION_IGNORE, BDRV_ACTION_STOP
>  } BlockMonEventAction;
>  
> +void bdrv_eject_mon_event(const BlockDriverState *bdrv);
>  void bdrv_error_mon_event(const BlockDriverState *bdrv,
>                            BlockMonEventAction action, int is_read);
>  void bdrv_info_print(Monitor *mon, const QObject *data);
> diff --git a/blockdev.c b/blockdev.c
> index 6e0eb83..5fd0043 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -661,6 +661,11 @@ static int eject_device(Monitor *mon, BlockDriverState *bs, int force)
>              return -1;
>          }
>      }
> +
> +    if (bdrv_is_removable(bs) && bdrv_is_inserted(bs)) {
> +        bdrv_eject_mon_event(bs);
> +    }
> +
>      bdrv_close(bs);
>      return 0;
>  }

This covers monitor-initiated eject (commands eject and change).

The event is not suppressed when the tray is already open (previous
guest-initiated eject), is it?.  Contradicts spec.

The event is suppressed when the tray is empty.

"eject -f" on a non-removable drive does not trigger an event.  Why
treat it specially?  I'm not saying you shouldn't, just wondering.

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

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

[Luiz Capitulino]

On Sat, 28 May 2011 09:58:24 +0200
Markus Armbruster <armbru@redhat.com> wrote:

> Luiz Capitulino <lcapitulino@redhat.com> writes:
> 
> > Conforms to the event specification defined in the
> > QMP/qmp-events.txt file.
> 
> I'd squash PATCH 2+3.

I agree this would be more logical, but people have complained that it's hard
to review new commands/events when its documentation is hidden or mixed with
code somewhere in the series. I think that make sense.

Maybe, it would be better to copy & paste the documentation in patch 0/0...

> > Please, note the following details:
> >
> >  o The event should be emitted only by devices which support the
> >    eject operation, which currently are: CDROMs (IDE and SCSI)
> >    and floppies
> >
> >  o Human monitor commands "eject" and "change" also cause the
> >    event to be emitted
> >
> >  o The event is only emitted when there's a tray transition from
> >    closed to opened. To implement this in the human monitor, we
> >    only emit the event if the device is removable and a media is
> >    present
> 
> Rationale?

This implementation covers the basic use case, which is to let clients know
that an already inserted media has been ejected. I was under the assumption
that this is the most important thing a client wants to know, as it will
have to update its internal state and that only has to be done for the media
it knows about (ie. the ones inserted by the client itself).

But...

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

Yes, this seems to make sense.

> > Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
> > ---
> >  block.c    |   12 ++++++++++++
> >  block.h    |    1 +
> >  blockdev.c |    5 +++++
> >  monitor.c  |    3 +++
> >  monitor.h  |    1 +
> >  5 files changed, 22 insertions(+), 0 deletions(-)
> >
> 
> Copied from PATCH 2 for review purposes:
> 
>   diff --git a/QMP/qmp-events.txt b/QMP/qmp-events.txt
>   index 0ce5d4e..d53c129 100644
>   --- a/QMP/qmp-events.txt
>   +++ b/QMP/qmp-events.txt
>   @@ -1,6 +1,24 @@
>                       QEMU Monitor Protocol Events
>                       ============================
> 
>   +BLOCK_MEDIA_EJECT
>   +-----------------
>   +
>   +Emitted when a removable disk media (such as a CDROM or floppy) is ejected.
>   +
>   +Data:
>   +
>   +- "device": device name (json-string)
>   +
>   +Example:
>   +
>   +{ "event": "BLOCK_MEDIA_EJECT",
>   +    "data": { "device": "ide1-cd0" },
>   +    "timestamp": { "seconds": 1265044230, "microseconds": 450486 } }
>   +
>   +NOTE: A disk media can be ejected by the guest or by monitor commands (such
>   +as "eject" and "change")
>   +
>    BLOCK_IO_ERROR
>    --------------
> 
> The event reports "tray opening".  Do we need one for "tray closing" as
> well?

At first I thought it wouldn't be needed (and maybe most use cases don't
require it), but reporting the tray status is more general and easier to do,
so I think it's what I'm going to do.

> > diff --git a/block.c b/block.c
> > index f5014cf..dbe813b 100644
> > --- a/block.c
> > +++ b/block.c
> > @@ -1656,6 +1656,15 @@ int bdrv_is_allocated(BlockDriverState *bs, int64_t sector_num, int nb_sectors,
> >      return bs->drv->bdrv_is_allocated(bs, sector_num, nb_sectors, pnum);
> >  }
> >  
> > +void bdrv_eject_mon_event(const BlockDriverState *bdrv)
> > +{
> > +    QObject *data;
> > +
> > +    data = qobject_from_jsonf("{ 'device': %s }", bdrv->device_name);
> > +    monitor_protocol_event(QEVENT_BLOCK_MEDIA_EJECT, data);
> > +    qobject_decref(data);
> > +}
> > +
> >  void bdrv_error_mon_event(const BlockDriverState *bdrv,
> >                            BlockMonEventAction action, int is_read)
> >  {
> > @@ -2770,6 +2779,9 @@ int bdrv_eject(BlockDriverState *bs, int eject_flag)
> >          ret = 0;
> >      }
> >      if (ret >= 0) {
> > +        if (eject_flag && !bs->tray_open) {
> > +            bdrv_eject_mon_event(bs);
> > +        }
> >          bs->tray_open = eject_flag;
> >      }
> >  
> 
> This covers guest-initiated eject.
>
> 
> The event is suppressed when the tray is already open.

Yes, because there's no visible state change.

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

Why does it contradicts the spec? No media is ejected if the tray is empty.
But this is probably not going to be an issue when we implement the more
general BLOCK_TRAY_STATUS (or something like it).

> > diff --git a/block.h b/block.h
> > index 1f58eab..e4053dd 100644
> > --- a/block.h
> > +++ b/block.h
> > @@ -50,6 +50,7 @@ typedef enum {
> >      BDRV_ACTION_REPORT, BDRV_ACTION_IGNORE, BDRV_ACTION_STOP
> >  } BlockMonEventAction;
> >  
> > +void bdrv_eject_mon_event(const BlockDriverState *bdrv);
> >  void bdrv_error_mon_event(const BlockDriverState *bdrv,
> >                            BlockMonEventAction action, int is_read);
> >  void bdrv_info_print(Monitor *mon, const QObject *data);
> > diff --git a/blockdev.c b/blockdev.c
> > index 6e0eb83..5fd0043 100644
> > --- a/blockdev.c
> > +++ b/blockdev.c
> > @@ -661,6 +661,11 @@ static int eject_device(Monitor *mon, BlockDriverState *bs, int force)
> >              return -1;
> >          }
> >      }
> > +
> > +    if (bdrv_is_removable(bs) && bdrv_is_inserted(bs)) {
> > +        bdrv_eject_mon_event(bs);
> > +    }
> > +
> >      bdrv_close(bs);
> >      return 0;
> >  }
> 
> This covers monitor-initiated eject (commands eject and change).
> 
> The event is not suppressed when the tray is already open (previous
> guest-initiated eject), is it?.  Contradicts spec.

That's a bug.

> The event is suppressed when the tray is empty.
> 
> "eject -f" on a non-removable drive does not trigger an event.  Why
> treat it specially?  I'm not saying you shouldn't, just wondering.

Ejecting a non-removable drive is a qemu bug.

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

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

[Markus Armbruster]

Luiz Capitulino <lcapitulino@redhat.com> writes:

> On Sat, 28 May 2011 09:58:24 +0200
> Markus Armbruster <armbru@redhat.com> wrote:
>
>> Luiz Capitulino <lcapitulino@redhat.com> writes:
>> 
>> > Conforms to the event specification defined in the
>> > QMP/qmp-events.txt file.
>> 
>> I'd squash PATCH 2+3.
>
> I agree this would be more logical, but people have complained that it's hard
> to review new commands/events when its documentation is hidden or mixed with
> code somewhere in the series. I think that make sense.
>
> Maybe, it would be better to copy & paste the documentation in patch 0/0...
>
>> > Please, note the following details:
>> >
>> >  o The event should be emitted only by devices which support the
>> >    eject operation, which currently are: CDROMs (IDE and SCSI)
>> >    and floppies
>> >
>> >  o Human monitor commands "eject" and "change" also cause the
>> >    event to be emitted
>> >
>> >  o The event is only emitted when there's a tray transition from
>> >    closed to opened. To implement this in the human monitor, we
>> >    only emit the event if the device is removable and a media is
>> >    present
>> 
>> Rationale?
>
> This implementation covers the basic use case, which is to let clients know
> that an already inserted media has been ejected. I was under the assumption
> that this is the most important thing a client wants to know, as it will
> have to update its internal state and that only has to be done for the media
> it knows about (ie. the ones inserted by the client itself).
>
> But...
>
>> Wouldn't it be easier if we just report tray status change, regardless
>> of media?
>
> Yes, this seems to make sense.
>
>> > Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
>> > ---
>> >  block.c    |   12 ++++++++++++
>> >  block.h    |    1 +
>> >  blockdev.c |    5 +++++
>> >  monitor.c  |    3 +++
>> >  monitor.h  |    1 +
>> >  5 files changed, 22 insertions(+), 0 deletions(-)
>> >
>> 
>> Copied from PATCH 2 for review purposes:
>> 
>>   diff --git a/QMP/qmp-events.txt b/QMP/qmp-events.txt
>>   index 0ce5d4e..d53c129 100644
>>   --- a/QMP/qmp-events.txt
>>   +++ b/QMP/qmp-events.txt
>>   @@ -1,6 +1,24 @@
>>                       QEMU Monitor Protocol Events
>>                       ============================
>> 
>>   +BLOCK_MEDIA_EJECT
>>   +-----------------
>>   +
>>   +Emitted when a removable disk media (such as a CDROM or floppy) is ejected.
>>   +
>>   +Data:
>>   +
>>   +- "device": device name (json-string)
>>   +
>>   +Example:
>>   +
>>   +{ "event": "BLOCK_MEDIA_EJECT",
>>   +    "data": { "device": "ide1-cd0" },
>>   +    "timestamp": { "seconds": 1265044230, "microseconds": 450486 } }
>>   +
>>   +NOTE: A disk media can be ejected by the guest or by monitor commands (such
>>   +as "eject" and "change")
>>   +
>>    BLOCK_IO_ERROR
>>    --------------
>> 
>> The event reports "tray opening".  Do we need one for "tray closing" as
>> well?
>
> At first I thought it wouldn't be needed (and maybe most use cases don't
> require it), but reporting the tray status is more general and easier to do,
> so I think it's what I'm going to do.
>
>> > diff --git a/block.c b/block.c
>> > index f5014cf..dbe813b 100644
>> > --- a/block.c
>> > +++ b/block.c
>> > @@ -1656,6 +1656,15 @@ int bdrv_is_allocated(BlockDriverState *bs, int64_t sector_num, int nb_sectors,
>> >      return bs->drv->bdrv_is_allocated(bs, sector_num, nb_sectors, pnum);
>> >  }
>> >  
>> > +void bdrv_eject_mon_event(const BlockDriverState *bdrv)
>> > +{
>> > +    QObject *data;
>> > +
>> > +    data = qobject_from_jsonf("{ 'device': %s }", bdrv->device_name);
>> > +    monitor_protocol_event(QEVENT_BLOCK_MEDIA_EJECT, data);
>> > +    qobject_decref(data);
>> > +}
>> > +
>> >  void bdrv_error_mon_event(const BlockDriverState *bdrv,
>> >                            BlockMonEventAction action, int is_read)
>> >  {
>> > @@ -2770,6 +2779,9 @@ int bdrv_eject(BlockDriverState *bs, int eject_flag)
>> >          ret = 0;
>> >      }
>> >      if (ret >= 0) {
>> > +        if (eject_flag && !bs->tray_open) {
>> > +            bdrv_eject_mon_event(bs);
>> > +        }
>> >          bs->tray_open = eject_flag;
>> >      }
>> >  
>> 
>> This covers guest-initiated eject.
>>
>> 
>> The event is suppressed when the tray is already open.
>
> Yes, because there's no visible state change.

No objection to that, as long as it's consistently done.

>> The event is not suppressed when the tray is empty, is it?  Contradicts
>> spec.
>
> Why does it contradicts the spec? No media is ejected if the tray is empty.

Yes.  Nevertheless, the event is triggered.

> But this is probably not going to be an issue when we implement the more
> general BLOCK_TRAY_STATUS (or something like it).

No need to debate the fine points of "media eject" then.

>> > diff --git a/block.h b/block.h
>> > index 1f58eab..e4053dd 100644
>> > --- a/block.h
>> > +++ b/block.h
>> > @@ -50,6 +50,7 @@ typedef enum {
>> >      BDRV_ACTION_REPORT, BDRV_ACTION_IGNORE, BDRV_ACTION_STOP
>> >  } BlockMonEventAction;
>> >  
>> > +void bdrv_eject_mon_event(const BlockDriverState *bdrv);
>> >  void bdrv_error_mon_event(const BlockDriverState *bdrv,
>> >                            BlockMonEventAction action, int is_read);
>> >  void bdrv_info_print(Monitor *mon, const QObject *data);
>> > diff --git a/blockdev.c b/blockdev.c
>> > index 6e0eb83..5fd0043 100644
>> > --- a/blockdev.c
>> > +++ b/blockdev.c
>> > @@ -661,6 +661,11 @@ static int eject_device(Monitor *mon, BlockDriverState *bs, int force)
>> >              return -1;
>> >          }
>> >      }
>> > +
>> > +    if (bdrv_is_removable(bs) && bdrv_is_inserted(bs)) {
>> > +        bdrv_eject_mon_event(bs);
>> > +    }
>> > +
>> >      bdrv_close(bs);
>> >      return 0;
>> >  }
>> 
>> This covers monitor-initiated eject (commands eject and change).
>> 
>> The event is not suppressed when the tray is already open (previous
>> guest-initiated eject), is it?.  Contradicts spec.
>
> That's a bug.
>
>> The event is suppressed when the tray is empty.
>> 
>> "eject -f" on a non-removable drive does not trigger an event.  Why
>> treat it specially?  I'm not saying you shouldn't, just wondering.
>
> Ejecting a non-removable drive is a qemu bug.

It's clearly intentional, so it's a (mis-)feature, not a bug.

[...]

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

[Kevin Wolf]

Am 30.05.2011 16:49, schrieb Markus Armbruster:
> Luiz Capitulino <lcapitulino@redhat.com> writes:
> 
>> On Sat, 28 May 2011 09:58:24 +0200
>> Markus Armbruster <armbru@redhat.com> wrote:
>>
>>> Luiz Capitulino <lcapitulino@redhat.com> writes:
>>>> diff --git a/block.h b/block.h
>>>> index 1f58eab..e4053dd 100644
>>>> --- a/block.h
>>>> +++ b/block.h
>>>> @@ -50,6 +50,7 @@ typedef enum {
>>>>      BDRV_ACTION_REPORT, BDRV_ACTION_IGNORE, BDRV_ACTION_STOP
>>>>  } BlockMonEventAction;
>>>>  
>>>> +void bdrv_eject_mon_event(const BlockDriverState *bdrv);
>>>>  void bdrv_error_mon_event(const BlockDriverState *bdrv,
>>>>                            BlockMonEventAction action, int is_read);
>>>>  void bdrv_info_print(Monitor *mon, const QObject *data);
>>>> diff --git a/blockdev.c b/blockdev.c
>>>> index 6e0eb83..5fd0043 100644
>>>> --- a/blockdev.c
>>>> +++ b/blockdev.c
>>>> @@ -661,6 +661,11 @@ static int eject_device(Monitor *mon, BlockDriverState *bs, int force)
>>>>              return -1;
>>>>          }
>>>>      }
>>>> +
>>>> +    if (bdrv_is_removable(bs) && bdrv_is_inserted(bs)) {
>>>> +        bdrv_eject_mon_event(bs);
>>>> +    }
>>>> +
>>>>      bdrv_close(bs);
>>>>      return 0;
>>>>  }
>>>
>>> This covers monitor-initiated eject (commands eject and change).
>>>
>>> The event is not suppressed when the tray is already open (previous
>>> guest-initiated eject), is it?.  Contradicts spec.
>>
>> That's a bug.
>>
>>> The event is suppressed when the tray is empty.
>>>
>>> "eject -f" on a non-removable drive does not trigger an event.  Why
>>> treat it specially?  I'm not saying you shouldn't, just wondering.
>>
>> Ejecting a non-removable drive is a qemu bug.
> 
> It's clearly intentional, so it's a (mis-)feature, not a bug.

Is there really a use case for it? The closest thing to a specification
that we have is the help text and it says:

        .help       = "eject a removable medium (use -f to force it)",

QMP describes it like this:

        Eject a removable medium.

So I start tending to agree that this whole trouble around the 'eject'
monitor command is in fact a long standing bug rather than overloaded
semantics. Nowhere is stated that it disconnects a BlockDriverState from
the image, and I can't imagine a use case for this semantics either.

Do we break anything if we make eject really eject the medium (we have a
virtual tray status now) instead of just closing the image? I think the
most visible change is that we'll eject the host medium when using
pass-through. I consider this an additional bugfix.

Kevin

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

[Luiz Capitulino]

On Tue, 31 May 2011 10:12:17 +0200
Kevin Wolf <kwolf@redhat.com> wrote:

> Am 30.05.2011 16:49, schrieb Markus Armbruster:
> > Luiz Capitulino <lcapitulino@redhat.com> writes:
> > 
> >> On Sat, 28 May 2011 09:58:24 +0200
> >> Markus Armbruster <armbru@redhat.com> wrote:
> >>
> >>> Luiz Capitulino <lcapitulino@redhat.com> writes:
> >>>> diff --git a/block.h b/block.h
> >>>> index 1f58eab..e4053dd 100644
> >>>> --- a/block.h
> >>>> +++ b/block.h
> >>>> @@ -50,6 +50,7 @@ typedef enum {
> >>>>      BDRV_ACTION_REPORT, BDRV_ACTION_IGNORE, BDRV_ACTION_STOP
> >>>>  } BlockMonEventAction;
> >>>>  
> >>>> +void bdrv_eject_mon_event(const BlockDriverState *bdrv);
> >>>>  void bdrv_error_mon_event(const BlockDriverState *bdrv,
> >>>>                            BlockMonEventAction action, int is_read);
> >>>>  void bdrv_info_print(Monitor *mon, const QObject *data);
> >>>> diff --git a/blockdev.c b/blockdev.c
> >>>> index 6e0eb83..5fd0043 100644
> >>>> --- a/blockdev.c
> >>>> +++ b/blockdev.c
> >>>> @@ -661,6 +661,11 @@ static int eject_device(Monitor *mon, BlockDriverState *bs, int force)
> >>>>              return -1;
> >>>>          }
> >>>>      }
> >>>> +
> >>>> +    if (bdrv_is_removable(bs) && bdrv_is_inserted(bs)) {
> >>>> +        bdrv_eject_mon_event(bs);
> >>>> +    }
> >>>> +
> >>>>      bdrv_close(bs);
> >>>>      return 0;
> >>>>  }
> >>>
> >>> This covers monitor-initiated eject (commands eject and change).
> >>>
> >>> The event is not suppressed when the tray is already open (previous
> >>> guest-initiated eject), is it?.  Contradicts spec.
> >>
> >> That's a bug.
> >>
> >>> The event is suppressed when the tray is empty.
> >>>
> >>> "eject -f" on a non-removable drive does not trigger an event.  Why
> >>> treat it specially?  I'm not saying you shouldn't, just wondering.
> >>
> >> Ejecting a non-removable drive is a qemu bug.
> > 
> > It's clearly intentional, so it's a (mis-)feature, not a bug.

Calling it "eject" is a bug, as it's not exactly what the command does.

> Is there really a use case for it? The closest thing to a specification
> that we have is the help text and it says:
> 
>         .help       = "eject a removable medium (use -f to force it)",
> 
> QMP describes it like this:
> 
>         Eject a removable medium.
> 
> So I start tending to agree that this whole trouble around the 'eject'
> monitor command is in fact a long standing bug rather than overloaded
> semantics. Nowhere is stated that it disconnects a BlockDriverState from
> the image, and I can't imagine a use case for this semantics either.

That's my thinking too.

> Do we break anything if we make eject really eject the medium (we have a
> virtual tray status now) instead of just closing the image?

I don't think so. I guess users/clients really have the expectation that
the only result is to get the media ejected.

Now, "-f" can be used with non-removable media. There's some risk of
breakage here if clients are using this to "unplug" devices. But I think
this a case where we'll have to pay the price for the breakage (if any).

> most visible change is that we'll eject the host medium when using
> pass-through. I consider this an additional bugfix.

Yes.

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

[Anthony Liguori]

On 05/31/2011 08:35 AM, Luiz Capitulino wrote:
> On Tue, 31 May 2011 10:12:17 +0200
>> Do we break anything if we make eject really eject the medium (we have a
>> virtual tray status now) instead of just closing the image?
>
> I don't think so. I guess users/clients really have the expectation that
> the only result is to get the media ejected.
>
> Now, "-f" can be used with non-removable media. There's some risk of
> breakage here if clients are using this to "unplug" devices. But I think
> this a case where we'll have to pay the price for the breakage (if any).

Only badness can ensue from doing that today so I see no reason to 
preserve this "functionality".

Regards,

Anthony Liguori

>
>> most visible change is that we'll eject the host medium when using
>> pass-through. I consider this an additional bugfix.
>
> Yes.
>

[Qemu-devel] [RFC 0/3]: QMP: Introduce BLOCK_MEDIA_EJECT event

[Luiz Capitulino]

Not very well tested, but I'm sending for early review.

Please, check individual patches for details. The event spec can be found
in patch 0/2.

 QMP/qmp-events.txt |   15 +++++++++++++++
 block.c            |   16 ++++++++++++++--
 block.h            |    5 +++--
 blockdev.c         |    5 +++++
 hw/ide/core.c      |    6 +++---
 hw/scsi-disk.c     |    6 +++---
 hw/virtio-blk.c    |    6 +++---
 monitor.c          |    3 +++
 monitor.h          |    1 +
 9 files changed, 50 insertions(+), 13 deletions(-)

[Qemu-devel] [PATCH 1/3] block: Rename bdrv_mon_event()

[Luiz Capitulino]

It's too generic and next commits will add a new event, rename it
to bdrv_error_mon_event() then.

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 ff2795b..5de7559 100644
--- a/block.c
+++ b/block.c
@@ -1553,8 +1553,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 f923add..4874b85 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 9496e99..2441a06 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -464,7 +464,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;
     }
 
@@ -472,7 +472,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(0);
     } else {
         if (op & BM_STATUS_DMA_RETRY) {
@@ -481,7 +481,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 6cb317c..0d0cd72 100644
--- a/hw/scsi-disk.c
+++ b/hw/scsi-disk.c
@@ -227,7 +227,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;
     }
 
@@ -237,7 +237,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(0);
     } else {
         if (type == SCSI_REQ_STATUS_RETRY_READ) {
@@ -245,7 +245,7 @@ static int scsi_handle_rw_error(SCSIDiskReq *r, int error, int type)
         }
         scsi_command_complete(r, CHECK_CONDITION,
                 HARDWARE_ERROR);
-        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 f62ccd1..8210902 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(0);
     } 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.rc1.14.g7fad99