Patchwork [2/3] QMP: Add BLOCK_MEDIA_EJECT event documentation

login
register
mail settings
Submitter Luiz Capitulino
Date May 27, 2011, 7:31 p.m.
Message ID <1306524712-13050-3-git-send-email-lcapitulino@redhat.com>
Download mbox | patch
Permalink /patch/97745/
State New
Headers show

Comments

Luiz Capitulino - May 27, 2011, 7:31 p.m.
Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 QMP/qmp-events.txt |   18 ++++++++++++++++++
 1 files changed, 18 insertions(+), 0 deletions(-)
Kevin Wolf - May 30, 2011, 8:46 a.m.
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
Luiz Capitulino - May 30, 2011, 2:21 p.m.
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.
Markus Armbruster - May 30, 2011, 2:54 p.m.
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.
Amit Shah - May 31, 2011, 7:09 a.m.
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
Kevin Wolf - May 31, 2011, 7:59 a.m.
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

Patch

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