| Message ID | 20120124161628.4bf2592c@doriath.home |
|---|---|
| State | New |
| Headers | show |
On 01/24/2012 11:16 AM, Luiz Capitulino wrote: > Libvirt wants to be notified when the guest ejects a medium, so that > it can update its view of the guest. > > This code has been originally written by Daniel Berrange. It adds > the event to IDE and SCSI emulation. > > Please, note that this only covers guest initiated ejects, that's, > the QMP/HMP commands 'eject' and 'change' are not covered. > > Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com> Reviewed-by: Eric Blake <eblake@redhat.com> > > +BLOCK_MEDIUM_EJECT > +------------------ > + > +Emitted when the guest succeeds ejecting a medium. If the device has a tray, s/succeeds ejecting/succeeds at ejecting/ Since libvirt is also tracking whether it makes 'eject' and 'change' monitor commands, and can also do a query when reconnecting to the monitor after a libvirtd restart to see if state changed in the meantime (when an event was lost), this should be sufficient for libvirt to have an accurate picture of the device state. I do have to wonder, however, if we also need an event for when the guest initiates a tray lock or tray unlock event, in order to track whether plain eject will work or whether a forced eject to override the tray lock would be required.
Am 24.01.2012 21:03, schrieb Eric Blake: > On 01/24/2012 11:16 AM, Luiz Capitulino wrote: >> Libvirt wants to be notified when the guest ejects a medium, so that >> it can update its view of the guest. >> >> This code has been originally written by Daniel Berrange. It adds >> the event to IDE and SCSI emulation. >> >> Please, note that this only covers guest initiated ejects, that's, >> the QMP/HMP commands 'eject' and 'change' are not covered. What's the reason for this behaviour? It feels inconsistent. Also, I seem to remember that once we had discussed some kind of a "tray status (open/closed) changed" event, which would be more generic. >> Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com> > > Reviewed-by: Eric Blake <eblake@redhat.com> > >> >> +BLOCK_MEDIUM_EJECT >> +------------------ >> + >> +Emitted when the guest succeeds ejecting a medium. If the device has a tray, > > s/succeeds ejecting/succeeds at ejecting/ > > Since libvirt is also tracking whether it makes 'eject' and 'change' > monitor commands, and can also do a query when reconnecting to the > monitor after a libvirtd restart to see if state changed in the meantime > (when an event was lost), this should be sufficient for libvirt to have > an accurate picture of the device state. > > I do have to wonder, however, if we also need an event for when the > guest initiates a tray lock or tray unlock event, in order to track > whether plain eject will work or whether a forced eject to override the > tray lock would be required. The tray is usually locked for a reason, so I would vote against libvirt automagically overriding it. Note that you don't really need a lock/unlock event for implementing it, you could just always pass force=true (or do it after you got QERR_DEVICE_LOCKED). The only reasonable thing for a management tool to do with a lock/unlock event would be updating some icon in a GUI or something like that. Kevin
Luiz Capitulino <lcapitulino@redhat.com> writes: > Libvirt wants to be notified when the guest ejects a medium, so that > it can update its view of the guest. > > This code has been originally written by Daniel Berrange. It adds > the event to IDE and SCSI emulation. > > Please, note that this only covers guest initiated ejects, that's, > the QMP/HMP commands 'eject' and 'change' are not covered. > > Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com> > --- > > PS: Bugs are mine. > > QMP/qmp-events.txt | 18 ++++++++++++++++++ > block.c | 12 ++++++++++++ > block.h | 5 +++++ > block_int.h | 3 +++ > blockdev.c | 13 +++++++++++++ > hw/ide/atapi.c | 1 + > hw/scsi-disk.c | 1 + > monitor.c | 3 +++ > monitor.h | 1 + > 9 files changed, 57 insertions(+), 0 deletions(-) > > diff --git a/QMP/qmp-events.txt b/QMP/qmp-events.txt > index af586ec..e414128 100644 > --- a/QMP/qmp-events.txt > +++ b/QMP/qmp-events.txt > @@ -26,6 +26,24 @@ Example: > Note: If action is "stop", a STOP event will eventually follow the > BLOCK_IO_ERROR event. > > +BLOCK_MEDIUM_EJECT > +------------------ > + > +Emitted when the guest succeeds ejecting a medium. If the device has a tray, > +then this event is emitted whenever the guest opens or closes the tray. This wording could be a bit confusing for devices where the physical device doesn't have a tray. Our CD-ROMs have a tray. Our floppy doesn't. Our floppy device model doesn't let the guest eject, so it doesn't matter right now. But trayless physical devices with removable media exist. Suggest to say something like "Emitted whenever the guest successfully ejects or loads a medium, or opens or closes the tray." > + > +Data: > + > +- "device": device name (json-string) This is the backend name, i.e. the name that's shown in "info block". It's not the qdev ID. Consistent with BLOCK_IO_ERROR. Good. We call too many things "device"... > +- "ejected": true if the tray has been opened or false if it has been closed > + > +Example: > + > +{ "event": "BLOCK_MEDIUM_EJECT", > + "data": { "device": "ide1-cd1", > + "ejected": true }, > + "timestamp": { "seconds": 1265044230, "microseconds": 450486 } } > + > RESET > ----- > > diff --git a/block.c b/block.c > index 3f072f6..b25932b 100644 > --- a/block.c > +++ b/block.c > @@ -1998,6 +1998,18 @@ BlockErrorAction bdrv_get_on_error(BlockDriverState *bs, int is_read) > return is_read ? bs->on_read_error : bs->on_write_error; > } > > +void bdrv_dev_set_medium_eject_notify(BlockDriverState *bs, > + bdrv_dev_medium_eject_notify_cb cb) > +{ > + bs->dev_medium_eject_notify_cb = cb; > +} > + > +void bdrv_dev_medium_eject_notify(BlockDriverState *bs, int is_ejected) > +{ > + assert(bs->dev_medium_eject_notify_cb); > + bs->dev_medium_eject_notify_cb(bs, is_ejected); > +} > + > int bdrv_is_read_only(BlockDriverState *bs) > { > return bs->read_only; > diff --git a/block.h b/block.h > index 3bd4398..344ca0d 100644 > --- a/block.h > +++ b/block.h > @@ -247,6 +247,11 @@ int bdrv_get_translation_hint(BlockDriverState *bs); > void bdrv_set_on_error(BlockDriverState *bs, BlockErrorAction on_read_error, > BlockErrorAction on_write_error); > BlockErrorAction bdrv_get_on_error(BlockDriverState *bs, int is_read); > +typedef void (*bdrv_dev_medium_eject_notify_cb)(BlockDriverState *bs, > + int is_ejected); > +void bdrv_dev_set_medium_eject_notify(BlockDriverState *bs, > + bdrv_dev_medium_eject_notify_cb cb); > +void bdrv_dev_medium_eject_notify(BlockDriverState *bs, int is_ejected); > int bdrv_is_read_only(BlockDriverState *bs); > int bdrv_is_sg(BlockDriverState *bs); > int bdrv_enable_write_cache(BlockDriverState *bs); > diff --git a/block_int.h b/block_int.h > index 311bd2a..50c34f6 100644 > --- a/block_int.h > +++ b/block_int.h > @@ -260,6 +260,9 @@ struct BlockDriverState { > QTAILQ_ENTRY(BlockDriverState) list; > void *private; > > + /* Callback for medium eject */ > + bdrv_dev_medium_eject_notify_cb dev_medium_eject_notify_cb; > + > QLIST_HEAD(, BdrvTrackedRequest) tracked_requests; > }; > Why the indirection? Isn't this always on_medium_eject()? bdrv_dev_medium_eject_notify_cb() asserts it's not null, and it's never set to anything else. If the indirection is justified, why is it per object (in BlockDriverState) and not per driver (in BlockDriver)? Update: I found a possible reason, see drive_init() below. block.h and block_int.h don't typedef their methods, suggest to stick to that. > diff --git a/blockdev.c b/blockdev.c > index 1f83c88..71b50fa 100644 > --- a/blockdev.c > +++ b/blockdev.c > @@ -11,6 +11,7 @@ > #include "blockdev.h" > #include "monitor.h" > #include "qerror.h" > +#include "qjson.h" > #include "qemu-option.h" > #include "qemu-config.h" > #include "sysemu.h" > @@ -237,6 +238,17 @@ static bool do_check_io_limits(BlockIOLimit *io_limits) > return true; > } > > +static void on_medium_eject(BlockDriverState *bs, int is_ejected) > +{ > + QObject *data; > + > + data = qobject_from_jsonf("{ 'device': %s, 'ejected': %i }", > + bdrv_get_device_name(bs), is_ejected); > + monitor_protocol_event(QEVENT_BLOCK_MEDIUM_EJECT, data); > + > + qobject_decref(data); > +} > + > DriveInfo *drive_init(QemuOpts *opts, int default_to_scsi) > { > const char *buf; > @@ -503,6 +515,7 @@ DriveInfo *drive_init(QemuOpts *opts, int default_to_scsi) > QTAILQ_INSERT_TAIL(&drives, dinfo, next); > > bdrv_set_on_error(dinfo->bdrv, on_read_error, on_write_error); > + bdrv_dev_set_medium_eject_notify(dinfo->bdrv, on_medium_eject); > > /* disk I/O throttling */ > bdrv_set_io_limits(dinfo->bdrv, &io_limits); Aha. The callback is set in BlockDriverStates created by drive_init() only. I.e. it's set for block backends created by the user, with -drive / drive_add, or its sugared forms. I figure you do this because you want the event emitted only for the "top" block backend (the the device model connects to), not the backends "below". Example: -drive if=none,id=foo,file=foo.img gives us a raw backend named "foo" on top of an anonymous file backend. The device model connects to "foo". You want the event emitted for raw "foo", but not the anonymous file backend. Example: -drive if=none,id=bar,file=bar.qcow2 gives us a qcow2 backend named "bar" on top of an anonymous file backend and another anonymous backend for the backing image (possibly on top of still more backends). The device model connects to "bar". You want the event emitted for "bar", but not the anonymous backends below. Device models are always connected to a backend created by drive_init() right now. That's why your assertion in bdrv_dev_set_medium_eject_notify() holds. However, the envisaged block backend configuration revamp hopefully relegates DriveInfo to legacy status, i.e. only block backends created in legacy ways have a DriveInfo. That'll break this code's assumption "the top block backend was created by drive_init()". There's a cleaner way to detect the top backend: it has a name, and is in bdrv_states. See bdrv_new(). But is it necessary? Why can't we just call on_medium_eject() and be done with it? Perhaps management applications eventually develop an interest in how the eject is propagated down. Then we'll have to report it for all block backends, not just the top one. And we'll also have to give the non-top ones names. We'll cross that bridge when we come to it. > diff --git a/hw/ide/atapi.c b/hw/ide/atapi.c > index 0adb27b..4b4bc61 100644 > --- a/hw/ide/atapi.c > +++ b/hw/ide/atapi.c > @@ -885,6 +885,7 @@ static void cmd_start_stop_unit(IDEState *s, uint8_t* buf) > } > bdrv_eject(s->bs, !start); > s->tray_open = !start; > + bdrv_dev_medium_eject_notify(s->bs, !start); > } > > ide_atapi_cmd_ok(s); > diff --git a/hw/scsi-disk.c b/hw/scsi-disk.c > index 5d8bf53..35e55f4 100644 > --- a/hw/scsi-disk.c > +++ b/hw/scsi-disk.c > @@ -1061,6 +1061,7 @@ static int scsi_disk_emulate_start_stop(SCSIDiskReq *r) > } > bdrv_eject(s->qdev.conf.bs, !start); > s->tray_open = !start; > + bdrv_dev_medium_eject_notify(s->qdev.conf.bs, !start); > } > return 0; > } Would it be possible to emit the event automatically from bdrv_eject()? We'd have to suppress it for anonymous block backends, of course. Hmm, if we do that, then ide_tray_state_post_load() emits the event, too. Would that be bad or good? > diff --git a/monitor.c b/monitor.c > index 187083c..df6b475 100644 > --- a/monitor.c > +++ b/monitor.c > @@ -479,6 +479,9 @@ void monitor_protocol_event(MonitorEvent event, QObject *data) > case QEVENT_SPICE_DISCONNECTED: > event_name = "SPICE_DISCONNECTED"; > break; > + case QEVENT_BLOCK_MEDIUM_EJECT: > + event_name = "BLOCK_MEDIUM_EJECT"; > + break; > default: > abort(); > break; > diff --git a/monitor.h b/monitor.h > index 887c472..16fab50 100644 > --- a/monitor.h > +++ b/monitor.h > @@ -36,6 +36,7 @@ typedef enum MonitorEvent { > QEVENT_SPICE_CONNECTED, > QEVENT_SPICE_INITIALIZED, > QEVENT_SPICE_DISCONNECTED, > + QEVENT_BLOCK_MEDIUM_EJECT, > QEVENT_MAX, > } MonitorEvent;
On Wed, 25 Jan 2012 09:41:20 +0100 Kevin Wolf <kwolf@redhat.com> wrote: > Am 24.01.2012 21:03, schrieb Eric Blake: > > On 01/24/2012 11:16 AM, Luiz Capitulino wrote: > >> Libvirt wants to be notified when the guest ejects a medium, so that > >> it can update its view of the guest. > >> > >> This code has been originally written by Daniel Berrange. It adds > >> the event to IDE and SCSI emulation. > >> > >> Please, note that this only covers guest initiated ejects, that's, > >> the QMP/HMP commands 'eject' and 'change' are not covered. > > What's the reason for this behaviour? It feels inconsistent. I don't think it's inconsistent because we're limiting it to guest initiated actions. Also, the mngt app knows when it sends a 'eject' or 'change' command. The exception is if it allows HMP to run in parallel with QMP, but I don't think this is recommended (at least not for commands that change any VM state). That said, I'm not necessarily against covering all cases. It just doesn't seem necessary to me. > Also, I seem to remember that once we had discussed some kind of a "tray > status (open/closed) changed" event, which would be more generic. Yes, but my old series got complex and way beyond the original event. If we decide to cover all eject scenarios, I'd like to do it w/o adding new commands. > > >> Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com> > > > > Reviewed-by: Eric Blake <eblake@redhat.com> > > > >> > >> +BLOCK_MEDIUM_EJECT > >> +------------------ > >> + > >> +Emitted when the guest succeeds ejecting a medium. If the device has a tray, > > > > s/succeeds ejecting/succeeds at ejecting/ > > > > Since libvirt is also tracking whether it makes 'eject' and 'change' > > monitor commands, and can also do a query when reconnecting to the > > monitor after a libvirtd restart to see if state changed in the meantime > > (when an event was lost), this should be sufficient for libvirt to have > > an accurate picture of the device state. > > > > I do have to wonder, however, if we also need an event for when the > > guest initiates a tray lock or tray unlock event, in order to track > > whether plain eject will work or whether a forced eject to override the > > tray lock would be required. > > The tray is usually locked for a reason, so I would vote against libvirt > automagically overriding it. Note that you don't really need a > lock/unlock event for implementing it, you could just always pass > force=true (or do it after you got QERR_DEVICE_LOCKED). Yes, libvirt will get the device locked error and can just retry. > > The only reasonable thing for a management tool to do with a lock/unlock > event would be updating some icon in a GUI or something like that. > > Kevin >
Am 25.01.2012 13:42, schrieb Luiz Capitulino: > On Wed, 25 Jan 2012 09:41:20 +0100 > Kevin Wolf <kwolf@redhat.com> wrote: > >> Am 24.01.2012 21:03, schrieb Eric Blake: >>> On 01/24/2012 11:16 AM, Luiz Capitulino wrote: >>>> Libvirt wants to be notified when the guest ejects a medium, so that >>>> it can update its view of the guest. >>>> >>>> This code has been originally written by Daniel Berrange. It adds >>>> the event to IDE and SCSI emulation. >>>> >>>> Please, note that this only covers guest initiated ejects, that's, >>>> the QMP/HMP commands 'eject' and 'change' are not covered. >> >> What's the reason for this behaviour? It feels inconsistent. > > I don't think it's inconsistent because we're limiting it to guest initiated > actions. Also, the mngt app knows when it sends a 'eject' or 'change' command. Yes, management shouldn't need an event in order to see what it just did itself. I haven't really looked at the code yet, but what I want to avoid is that we have to pass a flag all the way through qemu just so we don't send an event in the end. This might not be the case today, but after some more cleanup of the code it might just turn out like this. > The exception is if it allows HMP to run in parallel with QMP, but I don't think > this is recommended (at least not for commands that change any VM state). Or QMP passthrough. It would be very nice to support these scenarios one day. Probably not top priority, but we don't need to actively prevent such events from being generated. > That said, I'm not necessarily against covering all cases. It just doesn't > seem necessary to me. > >> Also, I seem to remember that once we had discussed some kind of a "tray >> status (open/closed) changed" event, which would be more generic. > > Yes, but my old series got complex and way beyond the original event. If we > decide to cover all eject scenarios, I'd like to do it w/o adding new commands. I don't really remember. Were it problems with the design of a tray status event or were you sidetracked by other problems? Kevin
On Wed, 25 Jan 2012 11:19:59 +0100 Markus Armbruster <armbru@redhat.com> wrote: > Luiz Capitulino <lcapitulino@redhat.com> writes: > > > Libvirt wants to be notified when the guest ejects a medium, so that > > it can update its view of the guest. > > > > This code has been originally written by Daniel Berrange. It adds > > the event to IDE and SCSI emulation. > > > > Please, note that this only covers guest initiated ejects, that's, > > the QMP/HMP commands 'eject' and 'change' are not covered. > > > > Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com> > > --- > > > > PS: Bugs are mine. > > > > QMP/qmp-events.txt | 18 ++++++++++++++++++ > > block.c | 12 ++++++++++++ > > block.h | 5 +++++ > > block_int.h | 3 +++ > > blockdev.c | 13 +++++++++++++ > > hw/ide/atapi.c | 1 + > > hw/scsi-disk.c | 1 + > > monitor.c | 3 +++ > > monitor.h | 1 + > > 9 files changed, 57 insertions(+), 0 deletions(-) > > > > diff --git a/QMP/qmp-events.txt b/QMP/qmp-events.txt > > index af586ec..e414128 100644 > > --- a/QMP/qmp-events.txt > > +++ b/QMP/qmp-events.txt > > @@ -26,6 +26,24 @@ Example: > > Note: If action is "stop", a STOP event will eventually follow the > > BLOCK_IO_ERROR event. > > > > +BLOCK_MEDIUM_EJECT > > +------------------ > > + > > +Emitted when the guest succeeds ejecting a medium. If the device has a tray, > > +then this event is emitted whenever the guest opens or closes the tray. > > This wording could be a bit confusing for devices where the physical > device doesn't have a tray. Our CD-ROMs have a tray. Our floppy > doesn't. Our floppy device model doesn't let the guest eject, so it > doesn't matter right now. But trayless physical devices with removable > media exist. > > Suggest to say something like "Emitted whenever the guest successfully > ejects or loads a medium, or opens or closes the tray." Ok. > > + > > +Data: > > + > > +- "device": device name (json-string) > > This is the backend name, i.e. the name that's shown in "info block". > It's not the qdev ID. Consistent with BLOCK_IO_ERROR. Good. > > We call too many things "device"... > > > +- "ejected": true if the tray has been opened or false if it has been closed > > + > > +Example: > > + > > +{ "event": "BLOCK_MEDIUM_EJECT", > > + "data": { "device": "ide1-cd1", > > + "ejected": true }, > > + "timestamp": { "seconds": 1265044230, "microseconds": 450486 } } > > + > > RESET > > ----- > > > > diff --git a/block.c b/block.c > > index 3f072f6..b25932b 100644 > > --- a/block.c > > +++ b/block.c > > @@ -1998,6 +1998,18 @@ BlockErrorAction bdrv_get_on_error(BlockDriverState *bs, int is_read) > > return is_read ? bs->on_read_error : bs->on_write_error; > > } > > > > +void bdrv_dev_set_medium_eject_notify(BlockDriverState *bs, > > + bdrv_dev_medium_eject_notify_cb cb) > > +{ > > + bs->dev_medium_eject_notify_cb = cb; > > +} > > + > > +void bdrv_dev_medium_eject_notify(BlockDriverState *bs, int is_ejected) > > +{ > > + assert(bs->dev_medium_eject_notify_cb); > > + bs->dev_medium_eject_notify_cb(bs, is_ejected); > > +} > > + > > int bdrv_is_read_only(BlockDriverState *bs) > > { > > return bs->read_only; > > diff --git a/block.h b/block.h > > index 3bd4398..344ca0d 100644 > > --- a/block.h > > +++ b/block.h > > @@ -247,6 +247,11 @@ int bdrv_get_translation_hint(BlockDriverState *bs); > > void bdrv_set_on_error(BlockDriverState *bs, BlockErrorAction on_read_error, > > BlockErrorAction on_write_error); > > BlockErrorAction bdrv_get_on_error(BlockDriverState *bs, int is_read); > > +typedef void (*bdrv_dev_medium_eject_notify_cb)(BlockDriverState *bs, > > + int is_ejected); > > +void bdrv_dev_set_medium_eject_notify(BlockDriverState *bs, > > + bdrv_dev_medium_eject_notify_cb cb); > > +void bdrv_dev_medium_eject_notify(BlockDriverState *bs, int is_ejected); > > int bdrv_is_read_only(BlockDriverState *bs); > > int bdrv_is_sg(BlockDriverState *bs); > > int bdrv_enable_write_cache(BlockDriverState *bs); > > diff --git a/block_int.h b/block_int.h > > index 311bd2a..50c34f6 100644 > > --- a/block_int.h > > +++ b/block_int.h > > @@ -260,6 +260,9 @@ struct BlockDriverState { > > QTAILQ_ENTRY(BlockDriverState) list; > > void *private; > > > > + /* Callback for medium eject */ > > + bdrv_dev_medium_eject_notify_cb dev_medium_eject_notify_cb; > > + > > QLIST_HEAD(, BdrvTrackedRequest) tracked_requests; > > }; > > > > Why the indirection? Isn't this always on_medium_eject()? > bdrv_dev_medium_eject_notify_cb() asserts it's not null, and it's never > set to anything else. > > If the indirection is justified, why is it per object (in > BlockDriverState) and not per driver (in BlockDriver)? > > Update: I found a possible reason, see drive_init() below. Let's discuss it below. > block.h and block_int.h don't typedef their methods, suggest to stick to > that. Ok. > > > diff --git a/blockdev.c b/blockdev.c > > index 1f83c88..71b50fa 100644 > > --- a/blockdev.c > > +++ b/blockdev.c > > @@ -11,6 +11,7 @@ > > #include "blockdev.h" > > #include "monitor.h" > > #include "qerror.h" > > +#include "qjson.h" > > #include "qemu-option.h" > > #include "qemu-config.h" > > #include "sysemu.h" > > @@ -237,6 +238,17 @@ static bool do_check_io_limits(BlockIOLimit *io_limits) > > return true; > > } > > > > +static void on_medium_eject(BlockDriverState *bs, int is_ejected) > > +{ > > + QObject *data; > > + > > + data = qobject_from_jsonf("{ 'device': %s, 'ejected': %i }", > > + bdrv_get_device_name(bs), is_ejected); > > + monitor_protocol_event(QEVENT_BLOCK_MEDIUM_EJECT, data); > > + > > + qobject_decref(data); > > +} > > + > > DriveInfo *drive_init(QemuOpts *opts, int default_to_scsi) > > { > > const char *buf; > > @@ -503,6 +515,7 @@ DriveInfo *drive_init(QemuOpts *opts, int default_to_scsi) > > QTAILQ_INSERT_TAIL(&drives, dinfo, next); > > > > bdrv_set_on_error(dinfo->bdrv, on_read_error, on_write_error); > > + bdrv_dev_set_medium_eject_notify(dinfo->bdrv, on_medium_eject); > > > > /* disk I/O throttling */ > > bdrv_set_io_limits(dinfo->bdrv, &io_limits); > > Aha. The callback is set in BlockDriverStates created by drive_init() > only. I.e. it's set for block backends created by the user, with -drive > / drive_add, or its sugared forms. > > I figure you do this because you want the event emitted only for the > "top" block backend (the the device model connects to), not the backends > "below". > > Example: -drive if=none,id=foo,file=foo.img gives us a raw backend named > "foo" on top of an anonymous file backend. The device model connects to > "foo". You want the event emitted for raw "foo", but not the anonymous > file backend. > > Example: -drive if=none,id=bar,file=bar.qcow2 gives us a qcow2 backend > named "bar" on top of an anonymous file backend and another anonymous > backend for the backing image (possibly on top of still more backends). > The device model connects to "bar". You want the event emitted for > "bar", but not the anonymous backends below. > > Device models are always connected to a backend created by drive_init() > right now. That's why your assertion in > bdrv_dev_set_medium_eject_notify() holds. Thanks for the explanation. I have to admit that all this theory wasn't clear to me. I based my review of the original code on what on_read_error and on_write_error do. > However, the envisaged block backend configuration revamp hopefully > relegates DriveInfo to legacy status, i.e. only block backends created > in legacy ways have a DriveInfo. That'll break this code's assumption > "the top block backend was created by drive_init()". > > There's a cleaner way to detect the top backend: it has a name, and is > in bdrv_states. See bdrv_new(). But is it necessary? Why can't we > just call on_medium_eject() and be done with it? Well, we can. I think that one benefit of the indirection is to avoid direct dependencies between modules that are unrelated in principle (like core ide & qmp). But calling on_medium_eject() directly works for me. > Perhaps management applications eventually develop an interest in how > the eject is propagated down. Then we'll have to report it for all > block backends, not just the top one. And we'll also have to give the > non-top ones names. We'll cross that bridge when we come to it. Yes. > > diff --git a/hw/ide/atapi.c b/hw/ide/atapi.c > > index 0adb27b..4b4bc61 100644 > > --- a/hw/ide/atapi.c > > +++ b/hw/ide/atapi.c > > @@ -885,6 +885,7 @@ static void cmd_start_stop_unit(IDEState *s, uint8_t* buf) > > } > > bdrv_eject(s->bs, !start); > > s->tray_open = !start; > > + bdrv_dev_medium_eject_notify(s->bs, !start); > > } > > > > ide_atapi_cmd_ok(s); > > diff --git a/hw/scsi-disk.c b/hw/scsi-disk.c > > index 5d8bf53..35e55f4 100644 > > --- a/hw/scsi-disk.c > > +++ b/hw/scsi-disk.c > > @@ -1061,6 +1061,7 @@ static int scsi_disk_emulate_start_stop(SCSIDiskReq *r) > > } > > bdrv_eject(s->qdev.conf.bs, !start); > > s->tray_open = !start; > > + bdrv_dev_medium_eject_notify(s->qdev.conf.bs, !start); > > } > > return 0; > > } > > Would it be possible to emit the event automatically from bdrv_eject()? > > We'd have to suppress it for anonymous block backends, of course. > > Hmm, if we do that, then ide_tray_state_post_load() emits the event, > too. Would that be bad or good? If we do eject the media, then it's probably good. But it's not clear to me why we do this, ie. why do we eject & lock after we load the state? > > > diff --git a/monitor.c b/monitor.c > > index 187083c..df6b475 100644 > > --- a/monitor.c > > +++ b/monitor.c > > @@ -479,6 +479,9 @@ void monitor_protocol_event(MonitorEvent event, QObject *data) > > case QEVENT_SPICE_DISCONNECTED: > > event_name = "SPICE_DISCONNECTED"; > > break; > > + case QEVENT_BLOCK_MEDIUM_EJECT: > > + event_name = "BLOCK_MEDIUM_EJECT"; > > + break; > > default: > > abort(); > > break; > > diff --git a/monitor.h b/monitor.h > > index 887c472..16fab50 100644 > > --- a/monitor.h > > +++ b/monitor.h > > @@ -36,6 +36,7 @@ typedef enum MonitorEvent { > > QEVENT_SPICE_CONNECTED, > > QEVENT_SPICE_INITIALIZED, > > QEVENT_SPICE_DISCONNECTED, > > + QEVENT_BLOCK_MEDIUM_EJECT, > > QEVENT_MAX, > > } MonitorEvent; >
On 01/25/2012 02:23 PM, Kevin Wolf wrote: > > > > Please, note that this only covers guest initiated ejects, that's, > > > > the QMP/HMP commands 'eject' and 'change' are not covered. > > > > > > What's the reason for this behaviour? It feels inconsistent. > > > > I don't think it's inconsistent because we're limiting it to guest initiated > > actions. Also, the mngt app knows when it sends a 'eject' or 'change' command. > > Yes, management shouldn't need an event in order to see what it just did > itself. I haven't really looked at the code yet, but what I want to > avoid is that we have to pass a flag all the way through qemu just so we > don't send an event in the end. This might not be the case today, but > after some more cleanup of the code it might just turn out like this. Management must be ready anyway to receive an event in response to eject/change actions that it started, because the guest might be trapping the host's eject requests ("press the button") and turning them into guest-initiated ejects. This is what recent udev does. Thus, a reliable eject procedure must be as follows: 1) Eject the disc 2) query the state of the tray. If it is open, poll for eject events and consume them. If it is closed, either exit or wait for an eject event to arrive. We can document that the event MAY NOT be reported for host-initiated ejects. It is future-proof and actually closer to what happens in practice if you consider a wide range of guests. Paolo
On Wed, 25 Jan 2012 14:23:55 +0100 Kevin Wolf <kwolf@redhat.com> wrote: > >> Also, I seem to remember that once we had discussed some kind of a "tray > >> status (open/closed) changed" event, which would be more generic. > > > > Yes, but my old series got complex and way beyond the original event. If we > > decide to cover all eject scenarios, I'd like to do it w/o adding new commands. > > I don't really remember. Were it problems with the design of a tray > status event or were you sidetracked by other problems? I was also trying to fix the 'change' command. So I added new commands to open, close and insert a medium in the tray. Then we had TRAY_OPEN & TRAY_CLOSE events... It was a 10 patch series iirc.
Am 25.01.2012 14:32, schrieb Paolo Bonzini: > On 01/25/2012 02:23 PM, Kevin Wolf wrote: >>>>> Please, note that this only covers guest initiated ejects, that's, >>>>> the QMP/HMP commands 'eject' and 'change' are not covered. >>>> >>>> What's the reason for this behaviour? It feels inconsistent. >>> >>> I don't think it's inconsistent because we're limiting it to guest initiated >>> actions. Also, the mngt app knows when it sends a 'eject' or 'change' command. >> >> Yes, management shouldn't need an event in order to see what it just did >> itself. I haven't really looked at the code yet, but what I want to >> avoid is that we have to pass a flag all the way through qemu just so we >> don't send an event in the end. This might not be the case today, but >> after some more cleanup of the code it might just turn out like this. > > Management must be ready anyway to receive an event in response to > eject/change actions that it started, because the guest might be > trapping the host's eject requests ("press the button") and turning them > into guest-initiated ejects. This is what recent udev does. Good point. The QMP client would still get an error that the device is locked, and only later receive the event that the guest ejected the medium, right? > Thus, a reliable eject procedure must be as follows: > > 1) Eject the disc > > 2) query the state of the tray. If it is open, poll for eject events > and consume them. If it is closed, either exit or wait for an eject > event to arrive. > > We can document that the event MAY NOT be reported for host-initiated > ejects. It is future-proof and actually closer to what happens in > practice if you consider a wide range of guests. In which case we did have an eject request, but we didn't actually eject. So I think defining it as being emitted whenever something is ejected/the tray is opened makes sense. Kevin
Paolo Bonzini <pbonzini@redhat.com> writes: > On 01/25/2012 02:23 PM, Kevin Wolf wrote: >> > > > Please, note that this only covers guest initiated ejects, that's, >> > > > the QMP/HMP commands 'eject' and 'change' are not covered. >> > > >> > > What's the reason for this behaviour? It feels inconsistent. >> > >> > I don't think it's inconsistent because we're limiting it to guest initiated >> > actions. Also, the mngt app knows when it sends a 'eject' or 'change' command. >> >> Yes, management shouldn't need an event in order to see what it just did >> itself. I haven't really looked at the code yet, but what I want to >> avoid is that we have to pass a flag all the way through qemu just so we >> don't send an event in the end. This might not be the case today, but >> after some more cleanup of the code it might just turn out like this. > > Management must be ready anyway to receive an event in response to > eject/change actions that it started, because the guest might be > trapping the host's eject requests ("press the button") and turning > them into guest-initiated ejects. This is what recent udev does. > > Thus, a reliable eject procedure must be as follows: > > 1) Eject the disc > > 2) query the state of the tray. If it is open, poll for eject events > and consume them. If it is closed, either exit or wait for an eject > event to arrive. > > We can document that the event MAY NOT be reported for host-initiated > ejects. It is future-proof and actually closer to what happens in > practice if you consider a wide range of guests. This is getting complicated... Feels like reporting tray open/close changes regardless of who triggered them could be simpler.
Luiz Capitulino <lcapitulino@redhat.com> writes: > On Wed, 25 Jan 2012 11:19:59 +0100 > Markus Armbruster <armbru@redhat.com> wrote: > >> Luiz Capitulino <lcapitulino@redhat.com> writes: >> >> > Libvirt wants to be notified when the guest ejects a medium, so that >> > it can update its view of the guest. >> > >> > This code has been originally written by Daniel Berrange. It adds >> > the event to IDE and SCSI emulation. >> > >> > Please, note that this only covers guest initiated ejects, that's, >> > the QMP/HMP commands 'eject' and 'change' are not covered. >> > >> > Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com> >> > --- >> > >> > PS: Bugs are mine. >> > >> > QMP/qmp-events.txt | 18 ++++++++++++++++++ >> > block.c | 12 ++++++++++++ >> > block.h | 5 +++++ >> > block_int.h | 3 +++ >> > blockdev.c | 13 +++++++++++++ >> > hw/ide/atapi.c | 1 + >> > hw/scsi-disk.c | 1 + >> > monitor.c | 3 +++ >> > monitor.h | 1 + >> > 9 files changed, 57 insertions(+), 0 deletions(-) >> > >> > diff --git a/QMP/qmp-events.txt b/QMP/qmp-events.txt >> > index af586ec..e414128 100644 >> > --- a/QMP/qmp-events.txt >> > +++ b/QMP/qmp-events.txt >> > @@ -26,6 +26,24 @@ Example: >> > Note: If action is "stop", a STOP event will eventually follow the >> > BLOCK_IO_ERROR event. >> > >> > +BLOCK_MEDIUM_EJECT >> > +------------------ >> > + >> > +Emitted when the guest succeeds ejecting a medium. If the device has a tray, >> > +then this event is emitted whenever the guest opens or closes the tray. >> >> This wording could be a bit confusing for devices where the physical >> device doesn't have a tray. Our CD-ROMs have a tray. Our floppy >> doesn't. Our floppy device model doesn't let the guest eject, so it >> doesn't matter right now. But trayless physical devices with removable >> media exist. >> >> Suggest to say something like "Emitted whenever the guest successfully >> ejects or loads a medium, or opens or closes the tray." > > Ok. > >> > + >> > +Data: >> > + >> > +- "device": device name (json-string) >> >> This is the backend name, i.e. the name that's shown in "info block". >> It's not the qdev ID. Consistent with BLOCK_IO_ERROR. Good. >> >> We call too many things "device"... >> >> > +- "ejected": true if the tray has been opened or false if it has been closed >> > + >> > +Example: >> > + >> > +{ "event": "BLOCK_MEDIUM_EJECT", >> > + "data": { "device": "ide1-cd1", >> > + "ejected": true }, >> > + "timestamp": { "seconds": 1265044230, "microseconds": 450486 } } >> > + >> > RESET >> > ----- >> > >> > diff --git a/block.c b/block.c >> > index 3f072f6..b25932b 100644 >> > --- a/block.c >> > +++ b/block.c >> > @@ -1998,6 +1998,18 @@ BlockErrorAction bdrv_get_on_error(BlockDriverState *bs, int is_read) >> > return is_read ? bs->on_read_error : bs->on_write_error; >> > } >> > >> > +void bdrv_dev_set_medium_eject_notify(BlockDriverState *bs, >> > + bdrv_dev_medium_eject_notify_cb cb) >> > +{ >> > + bs->dev_medium_eject_notify_cb = cb; >> > +} >> > + >> > +void bdrv_dev_medium_eject_notify(BlockDriverState *bs, int is_ejected) >> > +{ >> > + assert(bs->dev_medium_eject_notify_cb); >> > + bs->dev_medium_eject_notify_cb(bs, is_ejected); >> > +} >> > + >> > int bdrv_is_read_only(BlockDriverState *bs) >> > { >> > return bs->read_only; >> > diff --git a/block.h b/block.h >> > index 3bd4398..344ca0d 100644 >> > --- a/block.h >> > +++ b/block.h >> > @@ -247,6 +247,11 @@ int bdrv_get_translation_hint(BlockDriverState *bs); >> > void bdrv_set_on_error(BlockDriverState *bs, BlockErrorAction on_read_error, >> > BlockErrorAction on_write_error); >> > BlockErrorAction bdrv_get_on_error(BlockDriverState *bs, int is_read); >> > +typedef void (*bdrv_dev_medium_eject_notify_cb)(BlockDriverState *bs, >> > + int is_ejected); >> > +void bdrv_dev_set_medium_eject_notify(BlockDriverState *bs, >> > + bdrv_dev_medium_eject_notify_cb cb); >> > +void bdrv_dev_medium_eject_notify(BlockDriverState *bs, int is_ejected); >> > int bdrv_is_read_only(BlockDriverState *bs); >> > int bdrv_is_sg(BlockDriverState *bs); >> > int bdrv_enable_write_cache(BlockDriverState *bs); >> > diff --git a/block_int.h b/block_int.h >> > index 311bd2a..50c34f6 100644 >> > --- a/block_int.h >> > +++ b/block_int.h >> > @@ -260,6 +260,9 @@ struct BlockDriverState { >> > QTAILQ_ENTRY(BlockDriverState) list; >> > void *private; >> > >> > + /* Callback for medium eject */ >> > + bdrv_dev_medium_eject_notify_cb dev_medium_eject_notify_cb; >> > + >> > QLIST_HEAD(, BdrvTrackedRequest) tracked_requests; >> > }; >> > >> >> Why the indirection? Isn't this always on_medium_eject()? >> bdrv_dev_medium_eject_notify_cb() asserts it's not null, and it's never >> set to anything else. >> >> If the indirection is justified, why is it per object (in >> BlockDriverState) and not per driver (in BlockDriver)? >> >> Update: I found a possible reason, see drive_init() below. > > Let's discuss it below. > >> block.h and block_int.h don't typedef their methods, suggest to stick to >> that. > > Ok. > >> >> > diff --git a/blockdev.c b/blockdev.c >> > index 1f83c88..71b50fa 100644 >> > --- a/blockdev.c >> > +++ b/blockdev.c >> > @@ -11,6 +11,7 @@ >> > #include "blockdev.h" >> > #include "monitor.h" >> > #include "qerror.h" >> > +#include "qjson.h" >> > #include "qemu-option.h" >> > #include "qemu-config.h" >> > #include "sysemu.h" >> > @@ -237,6 +238,17 @@ static bool do_check_io_limits(BlockIOLimit *io_limits) >> > return true; >> > } >> > >> > +static void on_medium_eject(BlockDriverState *bs, int is_ejected) >> > +{ >> > + QObject *data; >> > + >> > + data = qobject_from_jsonf("{ 'device': %s, 'ejected': %i }", >> > + bdrv_get_device_name(bs), is_ejected); >> > + monitor_protocol_event(QEVENT_BLOCK_MEDIUM_EJECT, data); >> > + >> > + qobject_decref(data); >> > +} >> > + >> > DriveInfo *drive_init(QemuOpts *opts, int default_to_scsi) >> > { >> > const char *buf; >> > @@ -503,6 +515,7 @@ DriveInfo *drive_init(QemuOpts *opts, int default_to_scsi) >> > QTAILQ_INSERT_TAIL(&drives, dinfo, next); >> > >> > bdrv_set_on_error(dinfo->bdrv, on_read_error, on_write_error); >> > + bdrv_dev_set_medium_eject_notify(dinfo->bdrv, on_medium_eject); >> > >> > /* disk I/O throttling */ >> > bdrv_set_io_limits(dinfo->bdrv, &io_limits); >> >> Aha. The callback is set in BlockDriverStates created by drive_init() >> only. I.e. it's set for block backends created by the user, with -drive >> / drive_add, or its sugared forms. >> >> I figure you do this because you want the event emitted only for the >> "top" block backend (the the device model connects to), not the backends >> "below". >> >> Example: -drive if=none,id=foo,file=foo.img gives us a raw backend named >> "foo" on top of an anonymous file backend. The device model connects to >> "foo". You want the event emitted for raw "foo", but not the anonymous >> file backend. >> >> Example: -drive if=none,id=bar,file=bar.qcow2 gives us a qcow2 backend >> named "bar" on top of an anonymous file backend and another anonymous >> backend for the backing image (possibly on top of still more backends). >> The device model connects to "bar". You want the event emitted for >> "bar", but not the anonymous backends below. >> >> Device models are always connected to a backend created by drive_init() >> right now. That's why your assertion in >> bdrv_dev_set_medium_eject_notify() holds. > > Thanks for the explanation. I have to admit that all this theory wasn't > clear to me. I based my review of the original code on what on_read_error > and on_write_error do. That's fair :) >> However, the envisaged block backend configuration revamp hopefully >> relegates DriveInfo to legacy status, i.e. only block backends created >> in legacy ways have a DriveInfo. That'll break this code's assumption >> "the top block backend was created by drive_init()". >> >> There's a cleaner way to detect the top backend: it has a name, and is >> in bdrv_states. See bdrv_new(). But is it necessary? Why can't we >> just call on_medium_eject() and be done with it? > > Well, we can. I think that one benefit of the indirection is to avoid > direct dependencies between modules that are unrelated in principle (like > core ide & qmp). But calling on_medium_eject() directly works for me. Decoupling is a fair argument. >> Perhaps management applications eventually develop an interest in how >> the eject is propagated down. Then we'll have to report it for all >> block backends, not just the top one. And we'll also have to give the >> non-top ones names. We'll cross that bridge when we come to it. > > Yes. > >> > diff --git a/hw/ide/atapi.c b/hw/ide/atapi.c >> > index 0adb27b..4b4bc61 100644 >> > --- a/hw/ide/atapi.c >> > +++ b/hw/ide/atapi.c >> > @@ -885,6 +885,7 @@ static void cmd_start_stop_unit(IDEState *s, uint8_t* buf) >> > } >> > bdrv_eject(s->bs, !start); >> > s->tray_open = !start; >> > + bdrv_dev_medium_eject_notify(s->bs, !start); >> > } >> > >> > ide_atapi_cmd_ok(s); >> > diff --git a/hw/scsi-disk.c b/hw/scsi-disk.c >> > index 5d8bf53..35e55f4 100644 >> > --- a/hw/scsi-disk.c >> > +++ b/hw/scsi-disk.c >> > @@ -1061,6 +1061,7 @@ static int scsi_disk_emulate_start_stop(SCSIDiskReq *r) >> > } >> > bdrv_eject(s->qdev.conf.bs, !start); >> > s->tray_open = !start; >> > + bdrv_dev_medium_eject_notify(s->qdev.conf.bs, !start); >> > } >> > return 0; >> > } >> >> Would it be possible to emit the event automatically from bdrv_eject()? Just an idea, I'm not sure it's the right thing to do. >> We'd have to suppress it for anonymous block backends, of course. >> >> Hmm, if we do that, then ide_tray_state_post_load() emits the event, >> too. Would that be bad or good? > > If we do eject the media, then it's probably good. But it's not clear to me > why we do this, ie. why do we eject & lock after we load the state? Here's how migration works: we start qemu on the destination with devices matching the source's. Migration then copies device state from source to destination. In some cases we have state depending on migrated device state. That dependent state needs to be updated in a post_load hook. This is such a case. Block backends can elect to track the tray state, by implementing BlockDriver method bdrv_eject(). Currently, the only ones that do are "raw", which just passes it down, and "host_floppy", "host_cdrom", which enslave the physical tray to the virtual one: the physical tray opens / closes along with the virtual one. To stay in sync after migration, we need to call bdrv_eject() from the post_load hook. Same for the lock. Whether mixing migration and host drive passthrough make much sense is a separate question. By keeping the trays in sync, I sidestepped that question. Was easier than answering it :) If we emit the event from bdrv_eject(), we get a an event from the migration destination, even though the tray state didn't really change. >> > diff --git a/monitor.c b/monitor.c >> > index 187083c..df6b475 100644 >> > --- a/monitor.c >> > +++ b/monitor.c >> > @@ -479,6 +479,9 @@ void monitor_protocol_event(MonitorEvent event, QObject *data) >> > case QEVENT_SPICE_DISCONNECTED: >> > event_name = "SPICE_DISCONNECTED"; >> > break; >> > + case QEVENT_BLOCK_MEDIUM_EJECT: >> > + event_name = "BLOCK_MEDIUM_EJECT"; >> > + break; >> > default: >> > abort(); >> > break; >> > diff --git a/monitor.h b/monitor.h >> > index 887c472..16fab50 100644 >> > --- a/monitor.h >> > +++ b/monitor.h >> > @@ -36,6 +36,7 @@ typedef enum MonitorEvent { >> > QEVENT_SPICE_CONNECTED, >> > QEVENT_SPICE_INITIALIZED, >> > QEVENT_SPICE_DISCONNECTED, >> > + QEVENT_BLOCK_MEDIUM_EJECT, >> > QEVENT_MAX, >> > } MonitorEvent; >>
On Wed, 25 Jan 2012 10:42:04 -0200 Luiz Capitulino <lcapitulino@redhat.com> wrote: > On Wed, 25 Jan 2012 09:41:20 +0100 > Kevin Wolf <kwolf@redhat.com> wrote: > > > Am 24.01.2012 21:03, schrieb Eric Blake: > > > On 01/24/2012 11:16 AM, Luiz Capitulino wrote: > > >> Libvirt wants to be notified when the guest ejects a medium, so that > > >> it can update its view of the guest. > > >> > > >> This code has been originally written by Daniel Berrange. It adds > > >> the event to IDE and SCSI emulation. > > >> > > >> Please, note that this only covers guest initiated ejects, that's, > > >> the QMP/HMP commands 'eject' and 'change' are not covered. > > > > What's the reason for this behaviour? It feels inconsistent. > > I don't think it's inconsistent because we're limiting it to guest initiated > actions. Also, the mngt app knows when it sends a 'eject' or 'change' command. > The exception is if it allows HMP to run in parallel with QMP, but I don't think > this is recommended (at least not for commands that change any VM state). Let me elaborate more. We have two options: 1. Emit the event for guest initiated ejects (this patch, although I think the event should be renamed to GUEST_MEDIUM_EJECT) 2. Emit the event for guest & QMP initiated ejects, that's, also emit the event for the eject and change commands The first thing to note is that, they're not mutually exclusive. If we do item 1 now, we can add 2 later (as a different event). But my point is that doing 2 is problematic and we should avoid it. The problem is that the semantics of eject and change are complex and/or buggy. And something I've learned about confusing semantics in QMP is that, they will give you headaches soon or later. The main problem with eject is that it's inconsistent with the handling of the tray status. Try this: 1. Eject with no medium inserted (qemu) info block ide1-cd0: removable=1 locked=0 tray-open=0 io-status=ok [not inserted] (qemu) eject ide1-cd0 (qemu) info block ide1-cd0: removable=1 locked=0 tray-open=0 io-status=ok [not inserted] (qemu) Conclusion: the tray didn't move, we shouldn't emit the event. 2. Eject with medium inserted (qemu) info block ide1-cd0: removable=1 locked=0 tray-open=0 io-status=ok file=/tmp/vl.iKvBAF backing_file=/mnt/fernando/isos/linux/Fedora-14-x86_64-DVD.iso ro=0 drv=qcow2 encrypted=0 bps=0 bps_rd=0 bps_wr=0 iops=0 iops_rd=0 iops_wr=0 (qemu) eject ide1-cd0 (qemu) info block ide1-cd0: removable=1 locked=0 tray-open=1 io-status=ok [not inserted] (qemu) Conclusion: the tray opened and the media was purged. We should emit the event. 3. Eject with medium inserted and locked (qemu) info block ide1-cd0: removable=1 locked=1 tray-open=0 io-status=ok file=/tmp/vl.2LHApn backing_file=/mnt/fernando/isos/linux/Fedora-16-x86_64-DVD.iso ro=0 drv=qcow2 encrypted=0 bps=0 bps_rd=0 bps_wr=0 iops=0 iops_rd=0 iops_wr=0 (qemu) eject ide1-cd0 Device 'ide1-cd0' is locked (qemu) info block ide1-cd0: removable=1 locked=0 tray-open=1 io-status=ok file=/tmp/vl.2LHApn backing_file=/mnt/fernando/isos/linux/Fedora-16-x86_64-DVD.iso ro=0 drv=qcow2 encrypted=0 bps=0 bps_rd=0 bps_wr=0 iops=0 iops_rd=0 iops_wr=0 (qemu) Conclusion: eject returned an error, but a few seconds later the tray opened and the media wasn't purged. What happened here is that, the _guest_ opened the tray. The code in this patch would trigger the event, but we shouldn't emit it twice if we cover eject & change (ie. special case) Each of the three cases has different semantics wrt to the tray. We could try to fix them, but this seems to require some surgery... Is it worth it? Now, the change command. Basically, it opens the tray, purges the medium, inserts another medium and closes the tray. Should we generate the event twice? So, I'd just avoid all this and have only GUEST_MEDIUM_EJECT, which has clear semantics and satisfies libvirt needs...
Am 26.01.2012 18:57, schrieb Luiz Capitulino: > On Wed, 25 Jan 2012 10:42:04 -0200 > Luiz Capitulino <lcapitulino@redhat.com> wrote: > >> On Wed, 25 Jan 2012 09:41:20 +0100 >> Kevin Wolf <kwolf@redhat.com> wrote: >> >>> Am 24.01.2012 21:03, schrieb Eric Blake: >>>> On 01/24/2012 11:16 AM, Luiz Capitulino wrote: >>>>> Libvirt wants to be notified when the guest ejects a medium, so that >>>>> it can update its view of the guest. >>>>> >>>>> This code has been originally written by Daniel Berrange. It adds >>>>> the event to IDE and SCSI emulation. >>>>> >>>>> Please, note that this only covers guest initiated ejects, that's, >>>>> the QMP/HMP commands 'eject' and 'change' are not covered. >>> >>> What's the reason for this behaviour? It feels inconsistent. >> >> I don't think it's inconsistent because we're limiting it to guest initiated >> actions. Also, the mngt app knows when it sends a 'eject' or 'change' command. >> The exception is if it allows HMP to run in parallel with QMP, but I don't think >> this is recommended (at least not for commands that change any VM state). > > Let me elaborate more. We have two options: > > 1. Emit the event for guest initiated ejects (this patch, although I think > the event should be renamed to GUEST_MEDIUM_EJECT) > > 2. Emit the event for guest & QMP initiated ejects, that's, also emit the > event for the eject and change commands > > The first thing to note is that, they're not mutually exclusive. If we do > item 1 now, we can add 2 later (as a different event). > > But my point is that doing 2 is problematic and we should avoid it. The problem > is that the semantics of eject and change are complex and/or buggy. And something > I've learned about confusing semantics in QMP is that, they will give you headaches > soon or later. But I'm not really interested in the semantics of QMP commands, because they are irrelevant for the events. My view is that a device generates an event each time its try status changes. If it opens the tray, it does so by calling bdrv_eject - an obvious place to send an event. If it closes the tray or the eject monitor command is used, we go through bdrv_dev_change_media_cb - not quite as obvious, but I think this despite its name this is really a "tray status changed externally" callback. QMP commands may cause any of these actions to occur. But the event is tied to the actions and not to the QMP commands that may or may not cause them according to their confusing semantics. > The main problem with eject is that it's inconsistent with the handling of > the tray status. Try this: > > 1. Eject with no medium inserted > > (qemu) info block > ide1-cd0: removable=1 locked=0 tray-open=0 io-status=ok [not inserted] > (qemu) eject ide1-cd0 > (qemu) info block > ide1-cd0: removable=1 locked=0 tray-open=0 io-status=ok [not inserted] > (qemu) > > Conclusion: the tray didn't move, we shouldn't emit the event. Fails before bdrv_close, so bdrv_dev_change_media_cb is never called. Works as it is today. > > 2. Eject with medium inserted > > (qemu) info block > ide1-cd0: removable=1 locked=0 tray-open=0 io-status=ok file=/tmp/vl.iKvBAF backing_file=/mnt/fernando/isos/linux/Fedora-14-x86_64-DVD.iso ro=0 drv=qcow2 encrypted=0 bps=0 bps_rd=0 bps_wr=0 iops=0 iops_rd=0 iops_wr=0 > (qemu) eject ide1-cd0 > (qemu) info block > ide1-cd0: removable=1 locked=0 tray-open=1 io-status=ok [not inserted] > (qemu) > > Conclusion: the tray opened and the media was purged. We should emit the event. bdrv_dev_change_media_cb is called, we can emit an event there. > > 3. Eject with medium inserted and locked > > (qemu) info block > ide1-cd0: removable=1 locked=1 tray-open=0 io-status=ok file=/tmp/vl.2LHApn backing_file=/mnt/fernando/isos/linux/Fedora-16-x86_64-DVD.iso ro=0 drv=qcow2 encrypted=0 bps=0 bps_rd=0 bps_wr=0 iops=0 iops_rd=0 iops_wr=0 > (qemu) eject ide1-cd0 > Device 'ide1-cd0' is locked > (qemu) info block > ide1-cd0: removable=1 locked=0 tray-open=1 io-status=ok file=/tmp/vl.2LHApn backing_file=/mnt/fernando/isos/linux/Fedora-16-x86_64-DVD.iso ro=0 drv=qcow2 encrypted=0 bps=0 bps_rd=0 bps_wr=0 iops=0 iops_rd=0 iops_wr=0 > (qemu) > > Conclusion: eject returned an error, but a few seconds later the tray opened and > the media wasn't purged. What happened here is that, the _guest_ > opened the tray. The code in this patch would trigger the event, but > we shouldn't emit it twice if we cover eject & change (ie. special case) bdrv_dev_change_media_cb is not called because media cannot be ejected with a locked drive. Instead bdrv_dev_eject_request is called which doesn't emit an event. If the guest happens to initiate an eject itself after receiving the eject request, it calls bdrv_eject, where we can emit an event. If we had force=true in the initial eject command, bdrv_close is called, which in turn goes through bdrv_dev_change_media_cb where an event is emitted. > > Each of the three cases has different semantics wrt to the tray. We could try > to fix them, but this seems to require some surgery... Is it worth it? > > Now, the change command. Basically, it opens the tray, purges the medium, > inserts another medium and closes the tray. Should we generate the event twice? change is eject followed by bdrv_open. We don't teleport media into a device with closed tray, so yes, this is an open event followed by a close event. > So, I'd just avoid all this and have only GUEST_MEDIUM_EJECT, which has clear > semantics and satisfies libvirt needs... I think it's rather complex semantics compared to doing it always: Two more or less obvious places in block.c and no modification of the device emulation needed. For me that's a clear winner. Kevin
On 01/27/2012 10:52 AM, Kevin Wolf wrote: >> > Conclusion: eject returned an error, but a few seconds later the tray opened and >> > the media wasn't purged. What happened here is that, the_guest_ >> > opened the tray. The code in this patch would trigger the event, but >> > we shouldn't emit it twice if we cover eject& change (ie. special case) > bdrv_dev_change_media_cb is not called because media cannot be ejected > with a locked drive. Instead bdrv_dev_eject_request is called which > doesn't emit an event. > > If the guest happens to initiate an eject itself after receiving the > eject request, it calls bdrv_eject, where we can emit an event. > > If we had force=true in the initial eject command, bdrv_close is called, > which in turn goes through bdrv_dev_change_media_cb where an event is > emitted. But we still emit the eject request, and the guest will cause bdrv_eject to raise the event again. There can always be a race with the guest setting the locked bit, so management has to do a query-block anyway after eject or change. That's why the event is not necessary when the eject is host-initiated. Paolo
On 01/24/2012 07:16 PM, Luiz Capitulino wrote: > @@ -237,6 +238,17 @@ static bool do_check_io_limits(BlockIOLimit *io_limits) > return true; > } > > +static void on_medium_eject(BlockDriverState *bs, int is_ejected) > +{ > + QObject *data; > + > + data = qobject_from_jsonf("{ 'device': %s, 'ejected': %i }", > + bdrv_get_device_name(bs), is_ejected); > + monitor_protocol_event(QEVENT_BLOCK_MEDIUM_EJECT, data); > + > + qobject_decref(data); > +} > + > DriveInfo *drive_init(QemuOpts *opts, int default_to_scsi) > { > const char *buf; > @@ -503,6 +515,7 @@ DriveInfo *drive_init(QemuOpts *opts, int default_to_scsi) > QTAILQ_INSERT_TAIL(&drives, dinfo, next); > > bdrv_set_on_error(dinfo->bdrv, on_read_error, on_write_error); > + bdrv_dev_set_medium_eject_notify(dinfo->bdrv, on_medium_eject); > > /* disk I/O throttling */ > bdrv_set_io_limits(dinfo->bdrv, &io_limits); block.c/blockdev.c separation is nice, but do we really need a function pointer? Also, we're already emitting the BLOCK_IO_ERROR event in block.c; is that another place to cleanup, or is this overkill and we can just put bdrv_dev_medium_eject_notify in block.c? > diff --git a/hw/ide/atapi.c b/hw/ide/atapi.c > index 0adb27b..4b4bc61 100644 > --- a/hw/ide/atapi.c > +++ b/hw/ide/atapi.c > @@ -885,6 +885,7 @@ static void cmd_start_stop_unit(IDEState*s, uint8_t* buf) > } > bdrv_eject(s->bs, !start); > s->tray_open = !start; > + bdrv_dev_medium_eject_notify(s->bs, !start); > } > > ide_atapi_cmd_ok(s); > diff --git a/hw/scsi-disk.c b/hw/scsi-disk.c > index 5d8bf53..35e55f4 100644 > --- a/hw/scsi-disk.c > +++ b/hw/scsi-disk.c > @@ -1061,6 +1061,7 @@ static int scsi_disk_emulate_start_stop(SCSIDiskReq *r) > } > bdrv_eject(s->qdev.conf.bs, !start); > s->tray_open = !start; > + bdrv_dev_medium_eject_notify(s->qdev.conf.bs, !start); > } > return 0; > } Can you do the call directly in bdrv_eject (but I would skip bdrv_close)? The only other place where bdrv_eject is called is from block/raw.c's raw_eject, but I think you should only emit the event if bs->device_name[0] (otherwise the event is quite useless) and bs->file will fail the test. Paolo
On Fri, 27 Jan 2012 10:52:15 +0100 Kevin Wolf <kwolf@redhat.com> wrote: > Am 26.01.2012 18:57, schrieb Luiz Capitulino: > > On Wed, 25 Jan 2012 10:42:04 -0200 > > Luiz Capitulino <lcapitulino@redhat.com> wrote: > > > >> On Wed, 25 Jan 2012 09:41:20 +0100 > >> Kevin Wolf <kwolf@redhat.com> wrote: > >> > >>> Am 24.01.2012 21:03, schrieb Eric Blake: > >>>> On 01/24/2012 11:16 AM, Luiz Capitulino wrote: > >>>>> Libvirt wants to be notified when the guest ejects a medium, so that > >>>>> it can update its view of the guest. > >>>>> > >>>>> This code has been originally written by Daniel Berrange. It adds > >>>>> the event to IDE and SCSI emulation. > >>>>> > >>>>> Please, note that this only covers guest initiated ejects, that's, > >>>>> the QMP/HMP commands 'eject' and 'change' are not covered. > >>> > >>> What's the reason for this behaviour? It feels inconsistent. > >> > >> I don't think it's inconsistent because we're limiting it to guest initiated > >> actions. Also, the mngt app knows when it sends a 'eject' or 'change' command. > >> The exception is if it allows HMP to run in parallel with QMP, but I don't think > >> this is recommended (at least not for commands that change any VM state). > > > > Let me elaborate more. We have two options: > > > > 1. Emit the event for guest initiated ejects (this patch, although I think > > the event should be renamed to GUEST_MEDIUM_EJECT) > > > > 2. Emit the event for guest & QMP initiated ejects, that's, also emit the > > event for the eject and change commands > > > > The first thing to note is that, they're not mutually exclusive. If we do > > item 1 now, we can add 2 later (as a different event). > > > > But my point is that doing 2 is problematic and we should avoid it. The problem > > is that the semantics of eject and change are complex and/or buggy. And something > > I've learned about confusing semantics in QMP is that, they will give you headaches > > soon or later. > > But I'm not really interested in the semantics of QMP commands, because > they are irrelevant for the events. I do think they are relevant, because the event will have to match what the eject/change commands do with the tray. If what they do is messy, the event will turn out to be messy too. Now, I don't doubt this can be all fixed and made clean. I'm just not sure if it's worth it. > My view is that a device generates an event each time its try status > changes. Agreed. > If it opens the tray, it does so by calling bdrv_eject - an > obvious place to send an event. Yes, bdrv_eject() is called from device models. That's the easy part. But note that device models will also call bdrv_eject() when closing the tray. > If it closes the tray or the eject > monitor command is used, we go through bdrv_dev_change_media_cb - not > quite as obvious, but I think this despite its name this is really a > "tray status changed externally" callback. Not so simple for the eject command, because bdrv_dev_change_media_cb() is called from bdrv_close() but if the tray is locked by the guest and it later unlocks it (and that _will_ happen with you run eject with the tray locked by the guest) the event will be also emitted. > QMP commands may cause any of these actions to occur. But the event is > tied to the actions and not to the QMP commands that may or may not > cause them according to their confusing semantics. > > > The main problem with eject is that it's inconsistent with the handling of > > the tray status. Try this: > > > > 1. Eject with no medium inserted > > > > (qemu) info block > > ide1-cd0: removable=1 locked=0 tray-open=0 io-status=ok [not inserted] > > (qemu) eject ide1-cd0 > > (qemu) info block > > ide1-cd0: removable=1 locked=0 tray-open=0 io-status=ok [not inserted] > > (qemu) > > > > Conclusion: the tray didn't move, we shouldn't emit the event. > > Fails before bdrv_close, so bdrv_dev_change_media_cb is never called. > Works as it is today. Yes, my point with these two examples is that, ideally, eject should left the tray in the same state when it's issued. Today the tray status after eject depends on if a media is inserted or not. Maybe not a big issue, but I felt I should raise it. > > > > > 2. Eject with medium inserted > > > > (qemu) info block > > ide1-cd0: removable=1 locked=0 tray-open=0 io-status=ok file=/tmp/vl.iKvBAF backing_file=/mnt/fernando/isos/linux/Fedora-14-x86_64-DVD.iso ro=0 drv=qcow2 encrypted=0 bps=0 bps_rd=0 bps_wr=0 iops=0 iops_rd=0 iops_wr=0 > > (qemu) eject ide1-cd0 > > (qemu) info block > > ide1-cd0: removable=1 locked=0 tray-open=1 io-status=ok [not inserted] > > (qemu) > > > > Conclusion: the tray opened and the media was purged. We should emit the event. > > bdrv_dev_change_media_cb is called, we can emit an event there. > > > > > 3. Eject with medium inserted and locked > > > > (qemu) info block > > ide1-cd0: removable=1 locked=1 tray-open=0 io-status=ok file=/tmp/vl.2LHApn backing_file=/mnt/fernando/isos/linux/Fedora-16-x86_64-DVD.iso ro=0 drv=qcow2 encrypted=0 bps=0 bps_rd=0 bps_wr=0 iops=0 iops_rd=0 iops_wr=0 > > (qemu) eject ide1-cd0 > > Device 'ide1-cd0' is locked > > (qemu) info block > > ide1-cd0: removable=1 locked=0 tray-open=1 io-status=ok file=/tmp/vl.2LHApn backing_file=/mnt/fernando/isos/linux/Fedora-16-x86_64-DVD.iso ro=0 drv=qcow2 encrypted=0 bps=0 bps_rd=0 bps_wr=0 iops=0 iops_rd=0 iops_wr=0 > > (qemu) > > > > Conclusion: eject returned an error, but a few seconds later the tray opened and > > the media wasn't purged. What happened here is that, the _guest_ > > opened the tray. The code in this patch would trigger the event, but > > we shouldn't emit it twice if we cover eject & change (ie. special case) > > bdrv_dev_change_media_cb is not called because media cannot be ejected > with a locked drive. Instead bdrv_dev_eject_request is called which > doesn't emit an event. > > If the guest happens to initiate an eject itself after receiving the > eject request, it calls bdrv_eject, where we can emit an event. > > If we had force=true in the initial eject command, bdrv_close is called, > which in turn goes through bdrv_dev_change_media_cb where an event is > emitted. Can't this race with the guest eject? > > > > > Each of the three cases has different semantics wrt to the tray. We could try > > to fix them, but this seems to require some surgery... Is it worth it? > > > > Now, the change command. Basically, it opens the tray, purges the medium, > > inserts another medium and closes the tray. Should we generate the event twice? > > change is eject followed by bdrv_open. We don't teleport media into a > device with closed tray, so yes, this is an open event followed by a > close event. > > > So, I'd just avoid all this and have only GUEST_MEDIUM_EJECT, which has clear > > semantics and satisfies libvirt needs... > > I think it's rather complex semantics compared to doing it always: Two > more or less obvious places in block.c and no modification of the device > emulation needed. For me that's a clear winner. > > Kevin >
On Fri, 27 Jan 2012 13:10:39 +0100 Paolo Bonzini <pbonzini@redhat.com> wrote: > On 01/24/2012 07:16 PM, Luiz Capitulino wrote: > > @@ -237,6 +238,17 @@ static bool do_check_io_limits(BlockIOLimit *io_limits) > > return true; > > } > > > > +static void on_medium_eject(BlockDriverState *bs, int is_ejected) > > +{ > > + QObject *data; > > + > > + data = qobject_from_jsonf("{ 'device': %s, 'ejected': %i }", > > + bdrv_get_device_name(bs), is_ejected); > > + monitor_protocol_event(QEVENT_BLOCK_MEDIUM_EJECT, data); > > + > > + qobject_decref(data); > > +} > > + > > DriveInfo *drive_init(QemuOpts *opts, int default_to_scsi) > > { > > const char *buf; > > @@ -503,6 +515,7 @@ DriveInfo *drive_init(QemuOpts *opts, int default_to_scsi) > > QTAILQ_INSERT_TAIL(&drives, dinfo, next); > > > > bdrv_set_on_error(dinfo->bdrv, on_read_error, on_write_error); > > + bdrv_dev_set_medium_eject_notify(dinfo->bdrv, on_medium_eject); > > > > /* disk I/O throttling */ > > bdrv_set_io_limits(dinfo->bdrv, &io_limits); > > block.c/blockdev.c separation is nice, but do we really need a function > pointer? Also, we're already emitting the BLOCK_IO_ERROR event in > block.c; is that another place to cleanup, or is this overkill and we > can just put bdrv_dev_medium_eject_notify in block.c? This patch has changed after this whole discussion. My current version (not posted yet) adds a bdrv_emit_qmp_error_event() function to block.c and call it from bdrv_eject(). But that accounts only for the guest initiated ejects... > > diff --git a/hw/ide/atapi.c b/hw/ide/atapi.c > > index 0adb27b..4b4bc61 100644 > > --- a/hw/ide/atapi.c > > +++ b/hw/ide/atapi.c > > @@ -885,6 +885,7 @@ static void cmd_start_stop_unit(IDEState*s, uint8_t* buf) > > } > > bdrv_eject(s->bs, !start); > > s->tray_open = !start; > > + bdrv_dev_medium_eject_notify(s->bs, !start); > > } > > > > ide_atapi_cmd_ok(s); > > diff --git a/hw/scsi-disk.c b/hw/scsi-disk.c > > index 5d8bf53..35e55f4 100644 > > --- a/hw/scsi-disk.c > > +++ b/hw/scsi-disk.c > > @@ -1061,6 +1061,7 @@ static int scsi_disk_emulate_start_stop(SCSIDiskReq *r) > > } > > bdrv_eject(s->qdev.conf.bs, !start); > > s->tray_open = !start; > > + bdrv_dev_medium_eject_notify(s->qdev.conf.bs, !start); > > } > > return 0; > > } > > Can you do the call directly in bdrv_eject (but I would skip > bdrv_close)? The only other place where bdrv_eject is called is from > block/raw.c's raw_eject, but I think you should only emit the event if > bs->device_name[0] (otherwise the event is quite useless) and bs->file > will fail the test. Good point.
Am 30.01.2012 16:18, schrieb Luiz Capitulino: > On Fri, 27 Jan 2012 10:52:15 +0100 > Kevin Wolf <kwolf@redhat.com> wrote: > >> Am 26.01.2012 18:57, schrieb Luiz Capitulino: >>> On Wed, 25 Jan 2012 10:42:04 -0200 >>> Luiz Capitulino <lcapitulino@redhat.com> wrote: >>> >>>> On Wed, 25 Jan 2012 09:41:20 +0100 >>>> Kevin Wolf <kwolf@redhat.com> wrote: >>>> >>>>> Am 24.01.2012 21:03, schrieb Eric Blake: >>>>>> On 01/24/2012 11:16 AM, Luiz Capitulino wrote: >>>>>>> Libvirt wants to be notified when the guest ejects a medium, so that >>>>>>> it can update its view of the guest. >>>>>>> >>>>>>> This code has been originally written by Daniel Berrange. It adds >>>>>>> the event to IDE and SCSI emulation. >>>>>>> >>>>>>> Please, note that this only covers guest initiated ejects, that's, >>>>>>> the QMP/HMP commands 'eject' and 'change' are not covered. >>>>> >>>>> What's the reason for this behaviour? It feels inconsistent. >>>> >>>> I don't think it's inconsistent because we're limiting it to guest initiated >>>> actions. Also, the mngt app knows when it sends a 'eject' or 'change' command. >>>> The exception is if it allows HMP to run in parallel with QMP, but I don't think >>>> this is recommended (at least not for commands that change any VM state). >>> >>> Let me elaborate more. We have two options: >>> >>> 1. Emit the event for guest initiated ejects (this patch, although I think >>> the event should be renamed to GUEST_MEDIUM_EJECT) >>> >>> 2. Emit the event for guest & QMP initiated ejects, that's, also emit the >>> event for the eject and change commands >>> >>> The first thing to note is that, they're not mutually exclusive. If we do >>> item 1 now, we can add 2 later (as a different event). >>> >>> But my point is that doing 2 is problematic and we should avoid it. The problem >>> is that the semantics of eject and change are complex and/or buggy. And something >>> I've learned about confusing semantics in QMP is that, they will give you headaches >>> soon or later. >> >> But I'm not really interested in the semantics of QMP commands, because >> they are irrelevant for the events. > > I do think they are relevant, because the event will have to match what > the eject/change commands do with the tray. If what they do is messy, the > event will turn out to be messy too. > > Now, I don't doubt this can be all fixed and made clean. I'm just not sure > if it's worth it. If a mess best describes to the outside what we're doing to the device, then having a messy event is the best you can expect. Or in other words, if you're doing messy things with the device and you straighten things out in the event generation, then your events are lying to the management tools. >> My view is that a device generates an event each time its try status >> changes. > > Agreed. > >> If it opens the tray, it does so by calling bdrv_eject - an >> obvious place to send an event. > > Yes, bdrv_eject() is called from device models. That's the easy part. But note > that device models will also call bdrv_eject() when closing the tray. We should have events for both, so this is a good thing. But even if we didn't want to, devices tell us which one it is, so we could send an event only on tray open. >> If it closes the tray or the eject >> monitor command is used, we go through bdrv_dev_change_media_cb - not >> quite as obvious, but I think this despite its name this is really a >> "tray status changed externally" callback. > > Not so simple for the eject command, because bdrv_dev_change_media_cb() is > called from bdrv_close() but if the tray is locked by the guest and it later > unlocks it (and that _will_ happen with you run eject with the tray locked > by the guest) the event will be also emitted. Which is correct behaviour because the medium _has_ been ejected, right? The eject command doesn't go through bdrv_closed() with a locked tray, unless force=true. So you get the event only when the guest really ejects it, and not when your button press happens to do nothing more than sending an eject request to the guest. >> QMP commands may cause any of these actions to occur. But the event is >> tied to the actions and not to the QMP commands that may or may not >> cause them according to their confusing semantics. >> >>> The main problem with eject is that it's inconsistent with the handling of >>> the tray status. Try this: >>> >>> 1. Eject with no medium inserted >>> >>> (qemu) info block >>> ide1-cd0: removable=1 locked=0 tray-open=0 io-status=ok [not inserted] >>> (qemu) eject ide1-cd0 >>> (qemu) info block >>> ide1-cd0: removable=1 locked=0 tray-open=0 io-status=ok [not inserted] >>> (qemu) >>> >>> Conclusion: the tray didn't move, we shouldn't emit the event. >> >> Fails before bdrv_close, so bdrv_dev_change_media_cb is never called. >> Works as it is today. > > Yes, my point with these two examples is that, ideally, eject should left > the tray in the same state when it's issued. Today the tray status after > eject depends on if a media is inserted or not. > > Maybe not a big issue, but I felt I should raise it. Oh, I didn't even notice that. Would be less surprising if the tray was opened after ejecting an empty drive. I don't think it makes a real difference because we don't have a separate close_tray command, but the action is (just like tray_open, where necessary) automatically included in change. >>> 2. Eject with medium inserted >>> >>> (qemu) info block >>> ide1-cd0: removable=1 locked=0 tray-open=0 io-status=ok file=/tmp/vl.iKvBAF backing_file=/mnt/fernando/isos/linux/Fedora-14-x86_64-DVD.iso ro=0 drv=qcow2 encrypted=0 bps=0 bps_rd=0 bps_wr=0 iops=0 iops_rd=0 iops_wr=0 >>> (qemu) eject ide1-cd0 >>> (qemu) info block >>> ide1-cd0: removable=1 locked=0 tray-open=1 io-status=ok [not inserted] >>> (qemu) >>> >>> Conclusion: the tray opened and the media was purged. We should emit the event. >> >> bdrv_dev_change_media_cb is called, we can emit an event there. >> >>> >>> 3. Eject with medium inserted and locked >>> >>> (qemu) info block >>> ide1-cd0: removable=1 locked=1 tray-open=0 io-status=ok file=/tmp/vl.2LHApn backing_file=/mnt/fernando/isos/linux/Fedora-16-x86_64-DVD.iso ro=0 drv=qcow2 encrypted=0 bps=0 bps_rd=0 bps_wr=0 iops=0 iops_rd=0 iops_wr=0 >>> (qemu) eject ide1-cd0 >>> Device 'ide1-cd0' is locked >>> (qemu) info block >>> ide1-cd0: removable=1 locked=0 tray-open=1 io-status=ok file=/tmp/vl.2LHApn backing_file=/mnt/fernando/isos/linux/Fedora-16-x86_64-DVD.iso ro=0 drv=qcow2 encrypted=0 bps=0 bps_rd=0 bps_wr=0 iops=0 iops_rd=0 iops_wr=0 >>> (qemu) >>> >>> Conclusion: eject returned an error, but a few seconds later the tray opened and >>> the media wasn't purged. What happened here is that, the _guest_ >>> opened the tray. The code in this patch would trigger the event, but >>> we shouldn't emit it twice if we cover eject & change (ie. special case) >> >> bdrv_dev_change_media_cb is not called because media cannot be ejected >> with a locked drive. Instead bdrv_dev_eject_request is called which >> doesn't emit an event. >> >> If the guest happens to initiate an eject itself after receiving the >> eject request, it calls bdrv_eject, where we can emit an event. >> >> If we had force=true in the initial eject command, bdrv_close is called, >> which in turn goes through bdrv_dev_change_media_cb where an event is >> emitted. > > Can't this race with the guest eject? Can't see how, there's nothing asynchronous in the path. Kevin
On 01/30/2012 04:43 PM, Kevin Wolf wrote: >>> >> >>> >> If we had force=true in the initial eject command, bdrv_close is called, >>> >> which in turn goes through bdrv_dev_change_media_cb where an event is >>> >> emitted. >> > >> > Can't this race with the guest eject? > Can't see how, there's nothing asynchronous in the path. The button press is sent down to the guest even with -f. Paolo
Am 30.01.2012 16:55, schrieb Paolo Bonzini: > On 01/30/2012 04:43 PM, Kevin Wolf wrote: >>>>>> >>>>>> If we had force=true in the initial eject command, bdrv_close is called, >>>>>> which in turn goes through bdrv_dev_change_media_cb where an event is >>>>>> emitted. >>>> >>>> Can't this race with the guest eject? >> Can't see how, there's nothing asynchronous in the path. > > The button press is sent down to the guest even with -f. Right, but there's no race. The guest will always be later. Kevin
Kevin Wolf <kwolf@redhat.com> writes: > Am 30.01.2012 16:18, schrieb Luiz Capitulino: >> On Fri, 27 Jan 2012 10:52:15 +0100 >> Kevin Wolf <kwolf@redhat.com> wrote: >> >>> Am 26.01.2012 18:57, schrieb Luiz Capitulino: >>>> On Wed, 25 Jan 2012 10:42:04 -0200 >>>> Luiz Capitulino <lcapitulino@redhat.com> wrote: >>>> >>>>> On Wed, 25 Jan 2012 09:41:20 +0100 >>>>> Kevin Wolf <kwolf@redhat.com> wrote: >>>>> >>>>>> Am 24.01.2012 21:03, schrieb Eric Blake: >>>>>>> On 01/24/2012 11:16 AM, Luiz Capitulino wrote: >>>>>>>> Libvirt wants to be notified when the guest ejects a medium, so that >>>>>>>> it can update its view of the guest. >>>>>>>> >>>>>>>> This code has been originally written by Daniel Berrange. It adds >>>>>>>> the event to IDE and SCSI emulation. >>>>>>>> >>>>>>>> Please, note that this only covers guest initiated ejects, that's, >>>>>>>> the QMP/HMP commands 'eject' and 'change' are not covered. >>>>>> >>>>>> What's the reason for this behaviour? It feels inconsistent. >>>>> >>>>> I don't think it's inconsistent because we're limiting it to guest initiated >>>>> actions. Also, the mngt app knows when it sends a 'eject' or 'change' command. >>>>> The exception is if it allows HMP to run in parallel with QMP, but I don't think >>>>> this is recommended (at least not for commands that change any VM state). >>>> >>>> Let me elaborate more. We have two options: >>>> >>>> 1. Emit the event for guest initiated ejects (this patch, although I think >>>> the event should be renamed to GUEST_MEDIUM_EJECT) >>>> >>>> 2. Emit the event for guest & QMP initiated ejects, that's, also emit the >>>> event for the eject and change commands >>>> >>>> The first thing to note is that, they're not mutually exclusive. If we do >>>> item 1 now, we can add 2 later (as a different event). >>>> >>>> But my point is that doing 2 is problematic and we should avoid it. The problem >>>> is that the semantics of eject and change are complex and/or buggy. And something >>>> I've learned about confusing semantics in QMP is that, they will give you headaches >>>> soon or later. >>> >>> But I'm not really interested in the semantics of QMP commands, because >>> they are irrelevant for the events. >> >> I do think they are relevant, because the event will have to match what >> the eject/change commands do with the tray. If what they do is messy, the >> event will turn out to be messy too. >> >> Now, I don't doubt this can be all fixed and made clean. I'm just not sure >> if it's worth it. > > If a mess best describes to the outside what we're doing to the device, > then having a messy event is the best you can expect. Or in other words, > if you're doing messy things with the device and you straighten things > out in the event generation, then your events are lying to the > management tools. Yup. The event is merely a passive sensor. If reality is messy, sensor data better reflect that. Maybe it's easier to understand from a distance, so let's examine a more distant example: filesystem event monitoring with inotify(), specifically file permissions change. The event is simple enough: you get it when file permissions change. Now imagine chmod(2) was "improved" to refuse to set write bits unless read bits are also set, but only on Tuedays. That's a messy chmod() indeed. But the event remains as simple and clean as ever: you still get it when file permissions change. Back to QMP. In my opinion, the simple and clean event is "tray moved". Emit it when the tray moves, regardless of what made it move. Yes, the management app gets the event even when it itself directly triggered the move by commanding a media eject. That's a feature. It also gets the event when its media eject command actually becomes a polite request to the guest OS, because the tray happens to be locked, and the guest OS complies. That's a feature, too. [...]
On Tue, 31 Jan 2012 10:23:59 +0100 Markus Armbruster <armbru@redhat.com> wrote: > Kevin Wolf <kwolf@redhat.com> writes: > > > Am 30.01.2012 16:18, schrieb Luiz Capitulino: > >> On Fri, 27 Jan 2012 10:52:15 +0100 > >> Kevin Wolf <kwolf@redhat.com> wrote: > >> > >>> Am 26.01.2012 18:57, schrieb Luiz Capitulino: > >>>> On Wed, 25 Jan 2012 10:42:04 -0200 > >>>> Luiz Capitulino <lcapitulino@redhat.com> wrote: > >>>> > >>>>> On Wed, 25 Jan 2012 09:41:20 +0100 > >>>>> Kevin Wolf <kwolf@redhat.com> wrote: > >>>>> > >>>>>> Am 24.01.2012 21:03, schrieb Eric Blake: > >>>>>>> On 01/24/2012 11:16 AM, Luiz Capitulino wrote: > >>>>>>>> Libvirt wants to be notified when the guest ejects a medium, so that > >>>>>>>> it can update its view of the guest. > >>>>>>>> > >>>>>>>> This code has been originally written by Daniel Berrange. It adds > >>>>>>>> the event to IDE and SCSI emulation. > >>>>>>>> > >>>>>>>> Please, note that this only covers guest initiated ejects, that's, > >>>>>>>> the QMP/HMP commands 'eject' and 'change' are not covered. > >>>>>> > >>>>>> What's the reason for this behaviour? It feels inconsistent. > >>>>> > >>>>> I don't think it's inconsistent because we're limiting it to guest initiated > >>>>> actions. Also, the mngt app knows when it sends a 'eject' or 'change' command. > >>>>> The exception is if it allows HMP to run in parallel with QMP, but I don't think > >>>>> this is recommended (at least not for commands that change any VM state). > >>>> > >>>> Let me elaborate more. We have two options: > >>>> > >>>> 1. Emit the event for guest initiated ejects (this patch, although I think > >>>> the event should be renamed to GUEST_MEDIUM_EJECT) > >>>> > >>>> 2. Emit the event for guest & QMP initiated ejects, that's, also emit the > >>>> event for the eject and change commands > >>>> > >>>> The first thing to note is that, they're not mutually exclusive. If we do > >>>> item 1 now, we can add 2 later (as a different event). > >>>> > >>>> But my point is that doing 2 is problematic and we should avoid it. The problem > >>>> is that the semantics of eject and change are complex and/or buggy. And something > >>>> I've learned about confusing semantics in QMP is that, they will give you headaches > >>>> soon or later. > >>> > >>> But I'm not really interested in the semantics of QMP commands, because > >>> they are irrelevant for the events. > >> > >> I do think they are relevant, because the event will have to match what > >> the eject/change commands do with the tray. If what they do is messy, the > >> event will turn out to be messy too. > >> > >> Now, I don't doubt this can be all fixed and made clean. I'm just not sure > >> if it's worth it. > > > > If a mess best describes to the outside what we're doing to the device, > > then having a messy event is the best you can expect. Or in other words, > > if you're doing messy things with the device and you straighten things > > out in the event generation, then your events are lying to the > > management tools. > > Yup. The event is merely a passive sensor. If reality is messy, sensor > data better reflect that. > > Maybe it's easier to understand from a distance, so let's examine a more > distant example: filesystem event monitoring with inotify(), > specifically file permissions change. The event is simple enough: you > get it when file permissions change. Now imagine chmod(2) was > "improved" to refuse to set write bits unless read bits are also set, > but only on Tuedays. That's a messy chmod() indeed. But the event > remains as simple and clean as ever: you still get it when file > permissions change. > > Back to QMP. In my opinion, the simple and clean event is "tray moved". > Emit it when the tray moves, regardless of what made it move. I'll give it a tray. Oh, I meant a try :) > > Yes, the management app gets the event even when it itself directly > triggered the move by commanding a media eject. That's a feature. It > also gets the event when its media eject command actually becomes a > polite request to the guest OS, because the tray happens to be locked, > and the guest OS complies. That's a feature, too. > > [...] >
diff --git a/QMP/qmp-events.txt b/QMP/qmp-events.txt index af586ec..e414128 100644 --- a/QMP/qmp-events.txt +++ b/QMP/qmp-events.txt @@ -26,6 +26,24 @@ Example: Note: If action is "stop", a STOP event will eventually follow the BLOCK_IO_ERROR event. +BLOCK_MEDIUM_EJECT +------------------ + +Emitted when the guest succeeds ejecting a medium. If the device has a tray, +then this event is emitted whenever the guest opens or closes the tray. + +Data: + +- "device": device name (json-string) +- "ejected": true if the tray has been opened or false if it has been closed + +Example: + +{ "event": "BLOCK_MEDIUM_EJECT", + "data": { "device": "ide1-cd1", + "ejected": true }, + "timestamp": { "seconds": 1265044230, "microseconds": 450486 } } + RESET ----- diff --git a/block.c b/block.c index 3f072f6..b25932b 100644 --- a/block.c +++ b/block.c @@ -1998,6 +1998,18 @@ BlockErrorAction bdrv_get_on_error(BlockDriverState *bs, int is_read) return is_read ? bs->on_read_error : bs->on_write_error; } +void bdrv_dev_set_medium_eject_notify(BlockDriverState *bs, + bdrv_dev_medium_eject_notify_cb cb) +{ + bs->dev_medium_eject_notify_cb = cb; +} + +void bdrv_dev_medium_eject_notify(BlockDriverState *bs, int is_ejected) +{ + assert(bs->dev_medium_eject_notify_cb); + bs->dev_medium_eject_notify_cb(bs, is_ejected); +} + int bdrv_is_read_only(BlockDriverState *bs) { return bs->read_only; diff --git a/block.h b/block.h index 3bd4398..344ca0d 100644 --- a/block.h +++ b/block.h @@ -247,6 +247,11 @@ int bdrv_get_translation_hint(BlockDriverState *bs); void bdrv_set_on_error(BlockDriverState *bs, BlockErrorAction on_read_error, BlockErrorAction on_write_error); BlockErrorAction bdrv_get_on_error(BlockDriverState *bs, int is_read); +typedef void (*bdrv_dev_medium_eject_notify_cb)(BlockDriverState *bs, + int is_ejected); +void bdrv_dev_set_medium_eject_notify(BlockDriverState *bs, + bdrv_dev_medium_eject_notify_cb cb); +void bdrv_dev_medium_eject_notify(BlockDriverState *bs, int is_ejected); int bdrv_is_read_only(BlockDriverState *bs); int bdrv_is_sg(BlockDriverState *bs); int bdrv_enable_write_cache(BlockDriverState *bs); diff --git a/block_int.h b/block_int.h index 311bd2a..50c34f6 100644 --- a/block_int.h +++ b/block_int.h @@ -260,6 +260,9 @@ struct BlockDriverState { QTAILQ_ENTRY(BlockDriverState) list; void *private; + /* Callback for medium eject */ + bdrv_dev_medium_eject_notify_cb dev_medium_eject_notify_cb; + QLIST_HEAD(, BdrvTrackedRequest) tracked_requests; }; diff --git a/blockdev.c b/blockdev.c index 1f83c88..71b50fa 100644 --- a/blockdev.c +++ b/blockdev.c @@ -11,6 +11,7 @@ #include "blockdev.h" #include "monitor.h" #include "qerror.h" +#include "qjson.h" #include "qemu-option.h" #include "qemu-config.h" #include "sysemu.h" @@ -237,6 +238,17 @@ static bool do_check_io_limits(BlockIOLimit *io_limits) return true; } +static void on_medium_eject(BlockDriverState *bs, int is_ejected) +{ + QObject *data; + + data = qobject_from_jsonf("{ 'device': %s, 'ejected': %i }", + bdrv_get_device_name(bs), is_ejected); + monitor_protocol_event(QEVENT_BLOCK_MEDIUM_EJECT, data); + + qobject_decref(data); +} + DriveInfo *drive_init(QemuOpts *opts, int default_to_scsi) { const char *buf; @@ -503,6 +515,7 @@ DriveInfo *drive_init(QemuOpts *opts, int default_to_scsi) QTAILQ_INSERT_TAIL(&drives, dinfo, next); bdrv_set_on_error(dinfo->bdrv, on_read_error, on_write_error); + bdrv_dev_set_medium_eject_notify(dinfo->bdrv, on_medium_eject); /* disk I/O throttling */ bdrv_set_io_limits(dinfo->bdrv, &io_limits); diff --git a/hw/ide/atapi.c b/hw/ide/atapi.c index 0adb27b..4b4bc61 100644 --- a/hw/ide/atapi.c +++ b/hw/ide/atapi.c @@ -885,6 +885,7 @@ static void cmd_start_stop_unit(IDEState *s, uint8_t* buf) } bdrv_eject(s->bs, !start); s->tray_open = !start; + bdrv_dev_medium_eject_notify(s->bs, !start); } ide_atapi_cmd_ok(s); diff --git a/hw/scsi-disk.c b/hw/scsi-disk.c index 5d8bf53..35e55f4 100644 --- a/hw/scsi-disk.c +++ b/hw/scsi-disk.c @@ -1061,6 +1061,7 @@ static int scsi_disk_emulate_start_stop(SCSIDiskReq *r) } bdrv_eject(s->qdev.conf.bs, !start); s->tray_open = !start; + bdrv_dev_medium_eject_notify(s->qdev.conf.bs, !start); } return 0; } diff --git a/monitor.c b/monitor.c index 187083c..df6b475 100644 --- a/monitor.c +++ b/monitor.c @@ -479,6 +479,9 @@ void monitor_protocol_event(MonitorEvent event, QObject *data) case QEVENT_SPICE_DISCONNECTED: event_name = "SPICE_DISCONNECTED"; break; + case QEVENT_BLOCK_MEDIUM_EJECT: + event_name = "BLOCK_MEDIUM_EJECT"; + break; default: abort(); break; diff --git a/monitor.h b/monitor.h index 887c472..16fab50 100644 --- a/monitor.h +++ b/monitor.h @@ -36,6 +36,7 @@ typedef enum MonitorEvent { QEVENT_SPICE_CONNECTED, QEVENT_SPICE_INITIALIZED, QEVENT_SPICE_DISCONNECTED, + QEVENT_BLOCK_MEDIUM_EJECT, QEVENT_MAX, } MonitorEvent;
Libvirt wants to be notified when the guest ejects a medium, so that it can update its view of the guest. This code has been originally written by Daniel Berrange. It adds the event to IDE and SCSI emulation. Please, note that this only covers guest initiated ejects, that's, the QMP/HMP commands 'eject' and 'change' are not covered. Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com> --- PS: Bugs are mine. QMP/qmp-events.txt | 18 ++++++++++++++++++ block.c | 12 ++++++++++++ block.h | 5 +++++ block_int.h | 3 +++ blockdev.c | 13 +++++++++++++ hw/ide/atapi.c | 1 + hw/scsi-disk.c | 1 + monitor.c | 3 +++ monitor.h | 1 + 9 files changed, 57 insertions(+), 0 deletions(-)