| Message ID | 1306524712-13050-3-git-send-email-lcapitulino@redhat.com |
|---|---|
| State | New |
| Headers | show |
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
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.
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.
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
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
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 --------------
Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com> --- QMP/qmp-events.txt | 18 ++++++++++++++++++ 1 files changed, 18 insertions(+), 0 deletions(-)