| Message ID | 20130306130055.GA29446@redhat.com |
|---|---|
| State | New |
| Headers | show |
Am 06.03.2013 14:00, schrieb Michael S. Tsirkin: > libvirt has a long-standing bug: when removing the device, > it can request removal but does not know when does the > removal complete. Add an event so we can fix this in a robust way. > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com> Sounds like a good idea to me. :) [...] > diff --git a/hw/qdev.c b/hw/qdev.c > index 689cd54..f30d251 100644 > --- a/hw/qdev.c > +++ b/hw/qdev.c > @@ -29,6 +29,7 @@ > #include "sysemu/sysemu.h" > #include "qapi/error.h" > #include "qapi/visitor.h" > +#include "qapi/qmp/qjson.h" > > int qdev_hotplug = 0; > static bool qdev_hot_added = false; > @@ -267,6 +268,11 @@ void qdev_init_nofail(DeviceState *dev) > /* Unlink device from bus and free the structure. */ > void qdev_free(DeviceState *dev) > { > + if (dev->id) { > + QObject *data = qobject_from_jsonf("{ 'device': %s }", dev->id); > + monitor_protocol_event(QEVENT_DEVICE_DELETED, data); > + qobject_decref(data); > + } > object_unparent(OBJECT(dev)); > } > I'm pretty sure this is the wrong place to fire the notification. We should rather do this when the device is actually deleted - which qdev_free() does *not* actually guarantee, as criticized in the s390x and unref'ing contexts. I would suggest to place your code into device_unparent() instead. Another thing to consider is what data to pass to the event: Not all devices have an ID. We should still have a canonical path when we fire this event in either qdev_free() or in device_unparent() before the if (dev->parent_bus) block though. That would be a question for Anthony, not having a use case for the event I am indifferent there. Further, thinking of objects such as virtio-rng backends or future blockdev/chardev objects, might it make sense to turn this into a generic object deletion event rather than a device event? Andreas
On Wed, Mar 06, 2013 at 02:57:22PM +0100, Andreas Färber wrote: > Am 06.03.2013 14:00, schrieb Michael S. Tsirkin: > > libvirt has a long-standing bug: when removing the device, > > it can request removal but does not know when does the > > removal complete. Add an event so we can fix this in a robust way. > > > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com> > > Sounds like a good idea to me. :) > > [...] > > diff --git a/hw/qdev.c b/hw/qdev.c > > index 689cd54..f30d251 100644 > > --- a/hw/qdev.c > > +++ b/hw/qdev.c > > @@ -29,6 +29,7 @@ > > #include "sysemu/sysemu.h" > > #include "qapi/error.h" > > #include "qapi/visitor.h" > > +#include "qapi/qmp/qjson.h" > > > > int qdev_hotplug = 0; > > static bool qdev_hot_added = false; > > @@ -267,6 +268,11 @@ void qdev_init_nofail(DeviceState *dev) > > /* Unlink device from bus and free the structure. */ > > void qdev_free(DeviceState *dev) > > { > > + if (dev->id) { > > + QObject *data = qobject_from_jsonf("{ 'device': %s }", dev->id); > > + monitor_protocol_event(QEVENT_DEVICE_DELETED, data); > > + qobject_decref(data); > > + } > > object_unparent(OBJECT(dev)); > > } > > > > I'm pretty sure this is the wrong place to fire the notification. We > should rather do this when the device is actually deleted - which > qdev_free() does *not* actually guarantee, as criticized in the s390x > and unref'ing contexts. > I would suggest to place your code into device_unparent() instead. > > Another thing to consider is what data to pass to the event: Not all > devices have an ID. If they don't they were not created by management so management is probably not interested in them being removed. We could always add a 'path' key later if this assumption proves incorrect. > We should still have a canonical path when we fire > this event in either qdev_free() or in device_unparent() before the if > (dev->parent_bus) block though. That would be a question for Anthony, > not having a use case for the event I am indifferent there. > > Further, thinking of objects such as virtio-rng backends or future > blockdev/chardev objects, might it make sense to turn this into a > generic object deletion event rather than a device event? > > Andreas Backend deletion doesn't normally have guest interaction right? So why do we need an event? > -- > SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany > GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
On 03/06/2013 06:00 AM, Michael S. Tsirkin wrote: > libvirt has a long-standing bug: when removing the device, > it can request removal but does not know when does the > removal complete. Add an event so we can fix this in a robust way. s/complete/completes/ > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com> > > diff --git a/QMP/qmp-events.txt b/QMP/qmp-events.txt > index b2698e4..ec5b810 100644 > --- a/QMP/qmp-events.txt > +++ b/QMP/qmp-events.txt > @@ -136,6 +136,21 @@ Example: > Note: The "ready to complete" status is always reset by a BLOCK_JOB_ERROR > event. > > +DEVICE_DELETED > +----------------- > + > +It's emitted whenever the device removal completion is acknowledged I know this is copy-and-paste from DEVICE_TRAY_MOVED, but most of the file uses the more succinct: s/It's emitted/Emitted/ > +by the guest. At this point, it's safe to reuse the specified device ID. > +Device removal can be initiated by the guest or by HMP/QMP commands. > + > +Data: > + > +- "device": device name (json-string) > + > +{ "event": "DEVICE_DELETED", > + "data": { "device": "virtio-net-pci-0" }, > + "timestamp": { "seconds": 1265044230, "microseconds": 450486 } } Long overdue! > +++ b/qapi-schema.json > @@ -2354,7 +2354,9 @@ > # Notes: When this command completes, the device may not be removed from the > # guest. Hot removal is an operation that requires guest cooperation. > # This command merely requests that the guest begin the hot removal > -# process. > +# process. Completion of the device removal process is signaled with a > +# DEVICE_DELETED event. Guest reset will automatically complete removal > +# for all devices. Question - if libvirt misses the event (for example, if libvirtd requests a remove, but then gets restarted, and the event arrives before libvirtd is back up), is there a way to poll whether the the removal has completed? The event is great to minimize polling overhead in the common case, but we generally provide this sort of information via a pollable interface at the same time. > # > # Since: 0.14.0 > ## > --- > QMP/qmp-events.txt | 15 +++++++++++++++ > hw/qdev.c | 6 ++++++ > include/monitor/monitor.h | 1 + > monitor.c | 1 + > qapi-schema.json | 4 +++- > 5 files changed, 26 insertions(+), 1 deletion(-) Looks like your patch body is included twice?
On Wed, Mar 06, 2013 at 07:44:32AM -0700, Eric Blake wrote: > On 03/06/2013 06:00 AM, Michael S. Tsirkin wrote: > > libvirt has a long-standing bug: when removing the device, > > it can request removal but does not know when does the > > removal complete. Add an event so we can fix this in a robust way. > > s/complete/completes/ > > > > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com> > > > > diff --git a/QMP/qmp-events.txt b/QMP/qmp-events.txt > > index b2698e4..ec5b810 100644 > > --- a/QMP/qmp-events.txt > > +++ b/QMP/qmp-events.txt > > @@ -136,6 +136,21 @@ Example: > > Note: The "ready to complete" status is always reset by a BLOCK_JOB_ERROR > > event. > > > > +DEVICE_DELETED > > +----------------- > > + > > +It's emitted whenever the device removal completion is acknowledged > > I know this is copy-and-paste from DEVICE_TRAY_MOVED, but most of the > file uses the more succinct: s/It's emitted/Emitted/ > > > +by the guest. At this point, it's safe to reuse the specified device ID. > > +Device removal can be initiated by the guest or by HMP/QMP commands. > > + > > +Data: > > + > > +- "device": device name (json-string) > > + > > +{ "event": "DEVICE_DELETED", > > + "data": { "device": "virtio-net-pci-0" }, > > + "timestamp": { "seconds": 1265044230, "microseconds": 450486 } } > > Long overdue! > > +++ b/qapi-schema.json > > @@ -2354,7 +2354,9 @@ > > # Notes: When this command completes, the device may not be removed from the > > # guest. Hot removal is an operation that requires guest cooperation. > > # This command merely requests that the guest begin the hot removal > > -# process. > > +# process. Completion of the device removal process is signaled with a > > +# DEVICE_DELETED event. Guest reset will automatically complete removal > > +# for all devices. > > Question - if libvirt misses the event (for example, if libvirtd > requests a remove, but then gets restarted, and the event arrives before > libvirtd is back up), is there a way to poll whether the the removal has > completed? The event is great to minimize polling overhead in the > common case, but we generally provide this sort of information via a > pollable interface at the same time. If what you are trying to do is delete the device, you can simply retry that command and see if it failed. Is this enough? > > # > > # Since: 0.14.0 > > ## > > --- > > QMP/qmp-events.txt | 15 +++++++++++++++ > > hw/qdev.c | 6 ++++++ > > include/monitor/monitor.h | 1 + > > monitor.c | 1 + > > qapi-schema.json | 4 +++- > > 5 files changed, 26 insertions(+), 1 deletion(-) > > Looks like your patch body is included twice? Ugh. Yes. Donnu why.
Il 06/03/2013 15:44, Eric Blake ha scritto: > Question - if libvirt misses the event (for example, if libvirtd > requests a remove, but then gets restarted, and the event arrives before > libvirtd is back up), is there a way to poll whether the the removal has > completed? The event is great to minimize polling overhead in the > common case, but we generally provide this sort of information via a > pollable interface at the same time. Yes, you can use qom-list on /machine/peripheral. Paolo
[adding libvirt] On 03/06/2013 07:52 AM, Paolo Bonzini wrote: > Il 06/03/2013 15:44, Eric Blake ha scritto: >> Question - if libvirt misses the event (for example, if libvirtd >> requests a remove, but then gets restarted, and the event arrives before >> libvirtd is back up), is there a way to poll whether the the removal has >> completed? The event is great to minimize polling overhead in the >> common case, but we generally provide this sort of information via a >> pollable interface at the same time. > > Yes, you can use qom-list on /machine/peripheral. Which means libvirt should be patched to use qom-list right now, even before this event makes it in, in order to avoid its current bug of reusing a device id before the deletion has completed. Adding the event is still useful, as polling is never nice; and we already know how to make libvirt do conditional code based on whether query-events shows that the event has been added to minimize polling where possible.
Eric Blake <eblake@redhat.com> writes: > [adding libvirt] > > On 03/06/2013 07:52 AM, Paolo Bonzini wrote: >> Il 06/03/2013 15:44, Eric Blake ha scritto: >>> Question - if libvirt misses the event (for example, if libvirtd >>> requests a remove, but then gets restarted, and the event arrives before >>> libvirtd is back up), is there a way to poll whether the the removal has >>> completed? The event is great to minimize polling overhead in the >>> common case, but we generally provide this sort of information via a >>> pollable interface at the same time. General rule: no event without a way to poll. >> Yes, you can use qom-list on /machine/peripheral. > > Which means libvirt should be patched to use qom-list right now, even > before this event makes it in, in order to avoid its current bug of > reusing a device id before the deletion has completed. Adding the event > is still useful, as polling is never nice; and we already know how to > make libvirt do conditional code based on whether query-events shows > that the event has been added to minimize polling where possible. Yes.
"Michael S. Tsirkin" <mst@redhat.com> writes: > On Wed, Mar 06, 2013 at 02:57:22PM +0100, Andreas Färber wrote: >> Am 06.03.2013 14:00, schrieb Michael S. Tsirkin: >> > libvirt has a long-standing bug: when removing the device, >> > it can request removal but does not know when does the >> > removal complete. Add an event so we can fix this in a robust way. >> > >> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com> >> >> Sounds like a good idea to me. :) >> >> [...] >> > diff --git a/hw/qdev.c b/hw/qdev.c >> > index 689cd54..f30d251 100644 >> > --- a/hw/qdev.c >> > +++ b/hw/qdev.c >> > @@ -29,6 +29,7 @@ >> > #include "sysemu/sysemu.h" >> > #include "qapi/error.h" >> > #include "qapi/visitor.h" >> > +#include "qapi/qmp/qjson.h" >> > >> > int qdev_hotplug = 0; >> > static bool qdev_hot_added = false; >> > @@ -267,6 +268,11 @@ void qdev_init_nofail(DeviceState *dev) >> > /* Unlink device from bus and free the structure. */ >> > void qdev_free(DeviceState *dev) >> > { >> > + if (dev->id) { >> > + QObject *data = qobject_from_jsonf("{ 'device': %s }", dev->id); >> > + monitor_protocol_event(QEVENT_DEVICE_DELETED, data); >> > + qobject_decref(data); >> > + } >> > object_unparent(OBJECT(dev)); >> > } >> > >> >> I'm pretty sure this is the wrong place to fire the notification. We >> should rather do this when the device is actually deleted - which >> qdev_free() does *not* actually guarantee, as criticized in the s390x >> and unref'ing contexts. >> I would suggest to place your code into device_unparent() instead. >> >> Another thing to consider is what data to pass to the event: Not all >> devices have an ID. > > If they don't they were not created by management so management is > probably not interested in them being removed. > > We could always add a 'path' key later if this assumption > proves incorrect. In old qdev, ID was all we had, because paths were busted. Thus, management had no choice but use IDs. If I understand modern qdev correctly, we got a canonical path. Old APIs like device_del still accept only ID. Should new APIs still be designed that way? Or should they always accept / provide the canonical path, plus optional ID for convenience? >> We should still have a canonical path when we fire >> this event in either qdev_free() or in device_unparent() before the if >> (dev->parent_bus) block though. That would be a question for Anthony, >> not having a use case for the event I am indifferent there. >> >> Further, thinking of objects such as virtio-rng backends or future >> blockdev/chardev objects, might it make sense to turn this into a >> generic object deletion event rather than a device event? >> >> Andreas > > Backend deletion doesn't normally have guest interaction right? > So why do we need an event? We need an event because device_del may send its reply before it completes the job. device_del does that when it deletion needs to interact with the guest, which can take unbounded time. Conversely, we don't need an event when a QMP always completes the job (as far as observable by the QMP client) before it sends its reply. Off hand, I can't see why backend deletion would do anything else. I'm always reluctant to abstract when there are fewer than two different, concrete things to abstract from. Right now, we got just one: device models.
On Thu, Mar 07, 2013 at 10:55:23AM +0100, Markus Armbruster wrote: > "Michael S. Tsirkin" <mst@redhat.com> writes: > > > On Wed, Mar 06, 2013 at 02:57:22PM +0100, Andreas Färber wrote: > >> Am 06.03.2013 14:00, schrieb Michael S. Tsirkin: > >> > libvirt has a long-standing bug: when removing the device, > >> > it can request removal but does not know when does the > >> > removal complete. Add an event so we can fix this in a robust way. > >> > > >> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com> > >> > >> Sounds like a good idea to me. :) > >> > >> [...] > >> > diff --git a/hw/qdev.c b/hw/qdev.c > >> > index 689cd54..f30d251 100644 > >> > --- a/hw/qdev.c > >> > +++ b/hw/qdev.c > >> > @@ -29,6 +29,7 @@ > >> > #include "sysemu/sysemu.h" > >> > #include "qapi/error.h" > >> > #include "qapi/visitor.h" > >> > +#include "qapi/qmp/qjson.h" > >> > > >> > int qdev_hotplug = 0; > >> > static bool qdev_hot_added = false; > >> > @@ -267,6 +268,11 @@ void qdev_init_nofail(DeviceState *dev) > >> > /* Unlink device from bus and free the structure. */ > >> > void qdev_free(DeviceState *dev) > >> > { > >> > + if (dev->id) { > >> > + QObject *data = qobject_from_jsonf("{ 'device': %s }", dev->id); > >> > + monitor_protocol_event(QEVENT_DEVICE_DELETED, data); > >> > + qobject_decref(data); > >> > + } > >> > object_unparent(OBJECT(dev)); > >> > } > >> > > >> > >> I'm pretty sure this is the wrong place to fire the notification. We > >> should rather do this when the device is actually deleted - which > >> qdev_free() does *not* actually guarantee, as criticized in the s390x > >> and unref'ing contexts. > >> I would suggest to place your code into device_unparent() instead. > >> > >> Another thing to consider is what data to pass to the event: Not all > >> devices have an ID. > > > > If they don't they were not created by management so management is > > probably not interested in them being removed. > > > > We could always add a 'path' key later if this assumption > > proves incorrect. > > In old qdev, ID was all we had, because paths were busted. Thus, > management had no choice but use IDs. > > If I understand modern qdev correctly, we got a canonical path. Old > APIs like device_del still accept only ID. Should new APIs still be > designed that way? Or should they always accept / provide the canonical > path, plus optional ID for convenience? What are advantages of exposing the path to users in this way? Looks like maintainance hassle without real benefits? > >> We should still have a canonical path when we fire > >> this event in either qdev_free() or in device_unparent() before the if > >> (dev->parent_bus) block though. That would be a question for Anthony, > >> not having a use case for the event I am indifferent there. > >> > >> Further, thinking of objects such as virtio-rng backends or future > >> blockdev/chardev objects, might it make sense to turn this into a > >> generic object deletion event rather than a device event? > >> > >> Andreas > > > > Backend deletion doesn't normally have guest interaction right? > > So why do we need an event? > > We need an event because device_del may send its reply before it > completes the job. > > device_del does that when it deletion needs to interact with the guest, > which can take unbounded time. > > Conversely, we don't need an event when a QMP always completes the job > (as far as observable by the QMP client) before it sends its reply. Off > hand, I can't see why backend deletion would do anything else. > > I'm always reluctant to abstract when there are fewer than two > different, concrete things to abstract from. Right now, we got just > one: device models.
Am 07.03.2013 11:07, schrieb Michael S. Tsirkin: > On Thu, Mar 07, 2013 at 10:55:23AM +0100, Markus Armbruster wrote: >> "Michael S. Tsirkin" <mst@redhat.com> writes: >> >>> On Wed, Mar 06, 2013 at 02:57:22PM +0100, Andreas Färber wrote: >>>> Am 06.03.2013 14:00, schrieb Michael S. Tsirkin: >>>>> libvirt has a long-standing bug: when removing the device, >>>>> it can request removal but does not know when does the >>>>> removal complete. Add an event so we can fix this in a robust way. >>>>> >>>>> Signed-off-by: Michael S. Tsirkin <mst@redhat.com> >>>> >>>> Sounds like a good idea to me. :) >>>> >>>> [...] >>>>> diff --git a/hw/qdev.c b/hw/qdev.c >>>>> index 689cd54..f30d251 100644 >>>>> --- a/hw/qdev.c >>>>> +++ b/hw/qdev.c >>>>> @@ -29,6 +29,7 @@ >>>>> #include "sysemu/sysemu.h" >>>>> #include "qapi/error.h" >>>>> #include "qapi/visitor.h" >>>>> +#include "qapi/qmp/qjson.h" >>>>> >>>>> int qdev_hotplug = 0; >>>>> static bool qdev_hot_added = false; >>>>> @@ -267,6 +268,11 @@ void qdev_init_nofail(DeviceState *dev) >>>>> /* Unlink device from bus and free the structure. */ >>>>> void qdev_free(DeviceState *dev) >>>>> { >>>>> + if (dev->id) { >>>>> + QObject *data = qobject_from_jsonf("{ 'device': %s }", dev->id); >>>>> + monitor_protocol_event(QEVENT_DEVICE_DELETED, data); >>>>> + qobject_decref(data); >>>>> + } >>>>> object_unparent(OBJECT(dev)); >>>>> } >>>>> >>>> >>>> I'm pretty sure this is the wrong place to fire the notification. We >>>> should rather do this when the device is actually deleted - which >>>> qdev_free() does *not* actually guarantee, as criticized in the s390x >>>> and unref'ing contexts. >>>> I would suggest to place your code into device_unparent() instead. >>>> >>>> Another thing to consider is what data to pass to the event: Not all >>>> devices have an ID. >>> >>> If they don't they were not created by management so management is >>> probably not interested in them being removed. >>> >>> We could always add a 'path' key later if this assumption >>> proves incorrect. >> >> In old qdev, ID was all we had, because paths were busted. Thus, >> management had no choice but use IDs. >> >> If I understand modern qdev correctly, we got a canonical path. Old >> APIs like device_del still accept only ID. Should new APIs still be >> designed that way? Or should they always accept / provide the canonical >> path, plus optional ID for convenience? > > What are advantages of exposing the path to users in this way? > Looks like maintainance hassle without real benefits? Anthony had rejected earlier QOM patches by Paolo related to qdev id, saying it was deprecated in favor of those QOM paths. >>>> We should still have a canonical path when we fire >>>> this event in either qdev_free() or in device_unparent() before the if >>>> (dev->parent_bus) block though. That would be a question for Anthony, >>>> not having a use case for the event I am indifferent there. >>>> >>>> Further, thinking of objects such as virtio-rng backends or future >>>> blockdev/chardev objects, might it make sense to turn this into a >>>> generic object deletion event rather than a device event? >>>> >>>> Andreas >>> >>> Backend deletion doesn't normally have guest interaction right? >>> So why do we need an event? >> >> We need an event because device_del may send its reply before it >> completes the job. >> >> device_del does that when it deletion needs to interact with the guest, >> which can take unbounded time. >> >> Conversely, we don't need an event when a QMP always completes the job >> (as far as observable by the QMP client) before it sends its reply. Off >> hand, I can't see why backend deletion would do anything else. >> >> I'm always reluctant to abstract when there are fewer than two >> different, concrete things to abstract from. Right now, we got just >> one: device models. Not quite: It's about unparenting hook and object deletion, which are both not limited to devices. But if the ID based approach gets accepted by Anthony then we can still introduce an OBJECT_DELETED event once someone needs it. Andreas
Andreas Färber <afaerber@suse.de> writes: > Am 07.03.2013 11:07, schrieb Michael S. Tsirkin: >> On Thu, Mar 07, 2013 at 10:55:23AM +0100, Markus Armbruster wrote: >>> "Michael S. Tsirkin" <mst@redhat.com> writes: >>> >>>> On Wed, Mar 06, 2013 at 02:57:22PM +0100, Andreas Färber wrote: >>>>> Am 06.03.2013 14:00, schrieb Michael S. Tsirkin: >>>>>> libvirt has a long-standing bug: when removing the device, >>>>>> it can request removal but does not know when does the >>>>>> removal complete. Add an event so we can fix this in a robust way. >>>>>> >>>>>> Signed-off-by: Michael S. Tsirkin <mst@redhat.com> >>>>> >>>>> Sounds like a good idea to me. :) >>>>> >>>>> [...] >>>>>> diff --git a/hw/qdev.c b/hw/qdev.c >>>>>> index 689cd54..f30d251 100644 >>>>>> --- a/hw/qdev.c >>>>>> +++ b/hw/qdev.c >>>>>> @@ -29,6 +29,7 @@ >>>>>> #include "sysemu/sysemu.h" >>>>>> #include "qapi/error.h" >>>>>> #include "qapi/visitor.h" >>>>>> +#include "qapi/qmp/qjson.h" >>>>>> >>>>>> int qdev_hotplug = 0; >>>>>> static bool qdev_hot_added = false; >>>>>> @@ -267,6 +268,11 @@ void qdev_init_nofail(DeviceState *dev) >>>>>> /* Unlink device from bus and free the structure. */ >>>>>> void qdev_free(DeviceState *dev) >>>>>> { >>>>>> + if (dev->id) { >>>>>> + QObject *data = qobject_from_jsonf("{ 'device': %s }", dev->id); >>>>>> + monitor_protocol_event(QEVENT_DEVICE_DELETED, data); >>>>>> + qobject_decref(data); >>>>>> + } >>>>>> object_unparent(OBJECT(dev)); >>>>>> } >>>>>> >>>>> >>>>> I'm pretty sure this is the wrong place to fire the notification. We >>>>> should rather do this when the device is actually deleted - which >>>>> qdev_free() does *not* actually guarantee, as criticized in the s390x >>>>> and unref'ing contexts. >>>>> I would suggest to place your code into device_unparent() instead. >>>>> >>>>> Another thing to consider is what data to pass to the event: Not all >>>>> devices have an ID. >>>> >>>> If they don't they were not created by management so management is >>>> probably not interested in them being removed. >>>> >>>> We could always add a 'path' key later if this assumption >>>> proves incorrect. >>> >>> In old qdev, ID was all we had, because paths were busted. Thus, >>> management had no choice but use IDs. >>> >>> If I understand modern qdev correctly, we got a canonical path. Old >>> APIs like device_del still accept only ID. Should new APIs still be >>> designed that way? Or should they always accept / provide the canonical >>> path, plus optional ID for convenience? >> >> What are advantages of exposing the path to users in this way? The path is the device's canonical name. Canonical means path:device is 1:1. Path always works. Qdev ID only works when the user assigned one. Funny case: board creates a hot-pluggable device by default (thus no qdev ID), guest ejects it, what do you put into the event? Your code simply doesn't emit one. You could blame the user; after all he could've used -nodefaults, and added the device himself, with an ID. I blame your design instead, which needlessly complicates the event's semantics: it gets emitted only for devices with a qdev ID. Which you neglected to document clearly, by the way. If you put the path into the event, you can emit it always, which is simpler. Feel free to throw in the qdev ID. >> Looks like maintainance hassle without real benefits? I can't see path being a greater maintenance hassle than ID. > Anthony had rejected earlier QOM patches by Paolo related to qdev id, > saying it was deprecated in favor of those QOM paths. More reason to put the path into the event, not just the qdev ID. >>>>> We should still have a canonical path when we fire >>>>> this event in either qdev_free() or in device_unparent() before the if >>>>> (dev->parent_bus) block though. That would be a question for Anthony, >>>>> not having a use case for the event I am indifferent there. >>>>> >>>>> Further, thinking of objects such as virtio-rng backends or future >>>>> blockdev/chardev objects, might it make sense to turn this into a >>>>> generic object deletion event rather than a device event? >>>>> >>>>> Andreas >>>> >>>> Backend deletion doesn't normally have guest interaction right? >>>> So why do we need an event? >>> >>> We need an event because device_del may send its reply before it >>> completes the job. >>> >>> device_del does that when it deletion needs to interact with the guest, >>> which can take unbounded time. >>> >>> Conversely, we don't need an event when a QMP always completes the job >>> (as far as observable by the QMP client) before it sends its reply. Off >>> hand, I can't see why backend deletion would do anything else. >>> >>> I'm always reluctant to abstract when there are fewer than two >>> different, concrete things to abstract from. Right now, we got just >>> one: device models. > > Not quite: It's about unparenting hook and object deletion, which are > both not limited to devices. Yes, the implementation of the event happens to be sit outside qdev land in object land. On a conceptual level, the event makes sense only for objects that can go away asynchronously, i.e. not in response to a QMP command and before that QMP command sends its reply. The only such objects behaving like that are device models, as far as I know (unless we widen the scope to multiple monitors, where one monitor doesn't know what the other is doing). That makes me reluctant to design an event for more general cases. That said, if you'd prefer a more OBJECT_DELETED event, I think I'd defer to your judgement. > But if the ID based approach gets accepted by Anthony then we can still > introduce an OBJECT_DELETED event once someone needs it.
On Thu, Mar 07, 2013 at 03:14:15PM +0100, Markus Armbruster wrote: > Andreas Färber <afaerber@suse.de> writes: > > > Am 07.03.2013 11:07, schrieb Michael S. Tsirkin: > >> On Thu, Mar 07, 2013 at 10:55:23AM +0100, Markus Armbruster wrote: > >>> "Michael S. Tsirkin" <mst@redhat.com> writes: > >>> > >>>> On Wed, Mar 06, 2013 at 02:57:22PM +0100, Andreas Färber wrote: > >>>>> Am 06.03.2013 14:00, schrieb Michael S. Tsirkin: > >>>>>> libvirt has a long-standing bug: when removing the device, > >>>>>> it can request removal but does not know when does the > >>>>>> removal complete. Add an event so we can fix this in a robust way. > >>>>>> > >>>>>> Signed-off-by: Michael S. Tsirkin <mst@redhat.com> > >>>>> > >>>>> Sounds like a good idea to me. :) > >>>>> > >>>>> [...] > >>>>>> diff --git a/hw/qdev.c b/hw/qdev.c > >>>>>> index 689cd54..f30d251 100644 > >>>>>> --- a/hw/qdev.c > >>>>>> +++ b/hw/qdev.c > >>>>>> @@ -29,6 +29,7 @@ > >>>>>> #include "sysemu/sysemu.h" > >>>>>> #include "qapi/error.h" > >>>>>> #include "qapi/visitor.h" > >>>>>> +#include "qapi/qmp/qjson.h" > >>>>>> > >>>>>> int qdev_hotplug = 0; > >>>>>> static bool qdev_hot_added = false; > >>>>>> @@ -267,6 +268,11 @@ void qdev_init_nofail(DeviceState *dev) > >>>>>> /* Unlink device from bus and free the structure. */ > >>>>>> void qdev_free(DeviceState *dev) > >>>>>> { > >>>>>> + if (dev->id) { > >>>>>> + QObject *data = qobject_from_jsonf("{ 'device': %s }", dev->id); > >>>>>> + monitor_protocol_event(QEVENT_DEVICE_DELETED, data); > >>>>>> + qobject_decref(data); > >>>>>> + } > >>>>>> object_unparent(OBJECT(dev)); > >>>>>> } > >>>>>> > >>>>> > >>>>> I'm pretty sure this is the wrong place to fire the notification. We > >>>>> should rather do this when the device is actually deleted - which > >>>>> qdev_free() does *not* actually guarantee, as criticized in the s390x > >>>>> and unref'ing contexts. > >>>>> I would suggest to place your code into device_unparent() instead. > >>>>> > >>>>> Another thing to consider is what data to pass to the event: Not all > >>>>> devices have an ID. > >>>> > >>>> If they don't they were not created by management so management is > >>>> probably not interested in them being removed. > >>>> > >>>> We could always add a 'path' key later if this assumption > >>>> proves incorrect. > >>> > >>> In old qdev, ID was all we had, because paths were busted. Thus, > >>> management had no choice but use IDs. > >>> > >>> If I understand modern qdev correctly, we got a canonical path. Old > >>> APIs like device_del still accept only ID. Should new APIs still be > >>> designed that way? Or should they always accept / provide the canonical > >>> path, plus optional ID for convenience? > >> > >> What are advantages of exposing the path to users in this way? > > The path is the device's canonical name. Canonical means path:device is > 1:1. Path always works. Qdev ID only works when the user assigned one. > > Funny case: board creates a hot-pluggable device by default (thus no > qdev ID), guest ejects it, what do you put into the event? Your code > simply doesn't emit one. > > You could blame the user; after all he could've used -nodefaults, and > added the device himself, with an ID. > > I blame your design instead, which needlessly complicates the event's > semantics: it gets emitted only for devices with a qdev ID. Which you > neglected to document clearly, by the way. Good point, I'll document this. > If you put the path into the event, you can emit it always, which is > simpler. Feel free to throw in the qdev ID. I don't blame anyone. User not assigning an id is a clear indication that user does not care about the lifetime of this device. > >> Looks like maintainance hassle without real benefits? > > I can't see path being a greater maintenance hassle than ID. Sure, the less events we emit the less we need to support. You want to expose all kind of internal events, then management will come to depend on it and we'll have to maintain them forever. > > Anthony had rejected earlier QOM patches by Paolo related to qdev id, > > saying it was deprecated in favor of those QOM paths. > > More reason to put the path into the event, not just the qdev ID. libvirt does not seems to want it there. We'll always be able to add info but will never be able to remove info, keep it minimal. > >>>>> We should still have a canonical path when we fire > >>>>> this event in either qdev_free() or in device_unparent() before the if > >>>>> (dev->parent_bus) block though. That would be a question for Anthony, > >>>>> not having a use case for the event I am indifferent there. > >>>>> > >>>>> Further, thinking of objects such as virtio-rng backends or future > >>>>> blockdev/chardev objects, might it make sense to turn this into a > >>>>> generic object deletion event rather than a device event? > >>>>> > >>>>> Andreas > >>>> > >>>> Backend deletion doesn't normally have guest interaction right? > >>>> So why do we need an event? > >>> > >>> We need an event because device_del may send its reply before it > >>> completes the job. > >>> > >>> device_del does that when it deletion needs to interact with the guest, > >>> which can take unbounded time. > >>> > >>> Conversely, we don't need an event when a QMP always completes the job > >>> (as far as observable by the QMP client) before it sends its reply. Off > >>> hand, I can't see why backend deletion would do anything else. > >>> > >>> I'm always reluctant to abstract when there are fewer than two > >>> different, concrete things to abstract from. Right now, we got just > >>> one: device models. > > > > Not quite: It's about unparenting hook and object deletion, which are > > both not limited to devices. > > Yes, the implementation of the event happens to be sit outside qdev land > in object land. > > On a conceptual level, the event makes sense only for objects that can > go away asynchronously, i.e. not in response to a QMP command and before > that QMP command sends its reply. The only such objects behaving like > that are device models, as far as I know (unless we widen the scope to > multiple monitors, where one monitor doesn't know what the other is > doing). > > That makes me reluctant to design an event for more general cases. > > That said, if you'd prefer a more OBJECT_DELETED event, I think I'd > defer to your judgement. > > > But if the ID based approach gets accepted by Anthony then we can still > > introduce an OBJECT_DELETED event once someone needs it.
"Michael S. Tsirkin" <mst@redhat.com> writes: > On Thu, Mar 07, 2013 at 03:14:15PM +0100, Markus Armbruster wrote: >> Andreas Färber <afaerber@suse.de> writes: >> >> > Am 07.03.2013 11:07, schrieb Michael S. Tsirkin: >> >> On Thu, Mar 07, 2013 at 10:55:23AM +0100, Markus Armbruster wrote: >> >>> "Michael S. Tsirkin" <mst@redhat.com> writes: >> >>> >> >>>> On Wed, Mar 06, 2013 at 02:57:22PM +0100, Andreas Färber wrote: >> >>>>> Am 06.03.2013 14:00, schrieb Michael S. Tsirkin: >> >>>>>> libvirt has a long-standing bug: when removing the device, >> >>>>>> it can request removal but does not know when does the >> >>>>>> removal complete. Add an event so we can fix this in a robust way. >> >>>>>> >> >>>>>> Signed-off-by: Michael S. Tsirkin <mst@redhat.com> >> >>>>> >> >>>>> Sounds like a good idea to me. :) >> >>>>> >> >>>>> [...] >> >>>>>> diff --git a/hw/qdev.c b/hw/qdev.c >> >>>>>> index 689cd54..f30d251 100644 >> >>>>>> --- a/hw/qdev.c >> >>>>>> +++ b/hw/qdev.c >> >>>>>> @@ -29,6 +29,7 @@ >> >>>>>> #include "sysemu/sysemu.h" >> >>>>>> #include "qapi/error.h" >> >>>>>> #include "qapi/visitor.h" >> >>>>>> +#include "qapi/qmp/qjson.h" >> >>>>>> >> >>>>>> int qdev_hotplug = 0; >> >>>>>> static bool qdev_hot_added = false; >> >>>>>> @@ -267,6 +268,11 @@ void qdev_init_nofail(DeviceState *dev) >> >>>>>> /* Unlink device from bus and free the structure. */ >> >>>>>> void qdev_free(DeviceState *dev) >> >>>>>> { >> >>>>>> + if (dev->id) { >> >>>>>> + QObject *data = qobject_from_jsonf("{ 'device': %s }", dev->id); >> >>>>>> + monitor_protocol_event(QEVENT_DEVICE_DELETED, data); >> >>>>>> + qobject_decref(data); >> >>>>>> + } >> >>>>>> object_unparent(OBJECT(dev)); >> >>>>>> } >> >>>>>> >> >>>>> >> >>>>> I'm pretty sure this is the wrong place to fire the notification. We >> >>>>> should rather do this when the device is actually deleted - which >> >>>>> qdev_free() does *not* actually guarantee, as criticized in the s390x >> >>>>> and unref'ing contexts. >> >>>>> I would suggest to place your code into device_unparent() instead. >> >>>>> >> >>>>> Another thing to consider is what data to pass to the event: Not all >> >>>>> devices have an ID. >> >>>> >> >>>> If they don't they were not created by management so management is >> >>>> probably not interested in them being removed. >> >>>> >> >>>> We could always add a 'path' key later if this assumption >> >>>> proves incorrect. >> >>> >> >>> In old qdev, ID was all we had, because paths were busted. Thus, >> >>> management had no choice but use IDs. >> >>> >> >>> If I understand modern qdev correctly, we got a canonical path. Old >> >>> APIs like device_del still accept only ID. Should new APIs still be >> >>> designed that way? Or should they always accept / provide the canonical >> >>> path, plus optional ID for convenience? >> >> >> >> What are advantages of exposing the path to users in this way? >> >> The path is the device's canonical name. Canonical means path:device is >> 1:1. Path always works. Qdev ID only works when the user assigned one. >> >> Funny case: board creates a hot-pluggable device by default (thus no >> qdev ID), guest ejects it, what do you put into the event? Your code >> simply doesn't emit one. >> >> You could blame the user; after all he could've used -nodefaults, and >> added the device himself, with an ID. >> >> I blame your design instead, which needlessly complicates the event's >> semantics: it gets emitted only for devices with a qdev ID. Which you >> neglected to document clearly, by the way. > > Good point, I'll document this. > >> If you put the path into the event, you can emit it always, which is >> simpler. Feel free to throw in the qdev ID. > > I don't blame anyone. User not assigning an id is a clear indication > that user does not care about the lifetime of this device. > >> >> Looks like maintainance hassle without real benefits? >> >> I can't see path being a greater maintenance hassle than ID. > > Sure, the less events we emit the less we need to support. > You want to expose all kind of internal events, > then management will come to depend on it and > we'll have to maintain them forever. Misunderstanding. I'm *not* asking for more events. I'm asking for the DEVICE_DELETED event to carry the device's canonical name: its QOM path. >> > Anthony had rejected earlier QOM patches by Paolo related to qdev id, >> > saying it was deprecated in favor of those QOM paths. >> >> More reason to put the path into the event, not just the qdev ID. > > libvirt does not seems to want it there. We'll always be able to > add info but will never be able to remove info, keep it minimal. Yes, adding members to an event is easy. Doesn't mean we should do it just for the heck of it. If we don't need a member now, and we think there's a chance we won't need in the future, then we probably shouldn't add it now. I believe the chance of not needing the QOM path is effectively zero. Moreover, we'd add not just a member in this case, we'd add a *trigger*. Before: the event gets emitted only for devices with a qdev ID. After: the event gets emitted for all devices. I very much prefer the latter, because it's simpler. [...]
On Thu, Mar 07, 2013 at 06:23:46PM +0100, Markus Armbruster wrote: > "Michael S. Tsirkin" <mst@redhat.com> writes: > > > On Thu, Mar 07, 2013 at 03:14:15PM +0100, Markus Armbruster wrote: > >> Andreas Färber <afaerber@suse.de> writes: > >> > >> > Am 07.03.2013 11:07, schrieb Michael S. Tsirkin: > >> >> On Thu, Mar 07, 2013 at 10:55:23AM +0100, Markus Armbruster wrote: > >> >>> "Michael S. Tsirkin" <mst@redhat.com> writes: > >> >>> > >> >>>> On Wed, Mar 06, 2013 at 02:57:22PM +0100, Andreas Färber wrote: > >> >>>>> Am 06.03.2013 14:00, schrieb Michael S. Tsirkin: > >> >>>>>> libvirt has a long-standing bug: when removing the device, > >> >>>>>> it can request removal but does not know when does the > >> >>>>>> removal complete. Add an event so we can fix this in a robust way. > >> >>>>>> > >> >>>>>> Signed-off-by: Michael S. Tsirkin <mst@redhat.com> > >> >>>>> > >> >>>>> Sounds like a good idea to me. :) > >> >>>>> > >> >>>>> [...] > >> >>>>>> diff --git a/hw/qdev.c b/hw/qdev.c > >> >>>>>> index 689cd54..f30d251 100644 > >> >>>>>> --- a/hw/qdev.c > >> >>>>>> +++ b/hw/qdev.c > >> >>>>>> @@ -29,6 +29,7 @@ > >> >>>>>> #include "sysemu/sysemu.h" > >> >>>>>> #include "qapi/error.h" > >> >>>>>> #include "qapi/visitor.h" > >> >>>>>> +#include "qapi/qmp/qjson.h" > >> >>>>>> > >> >>>>>> int qdev_hotplug = 0; > >> >>>>>> static bool qdev_hot_added = false; > >> >>>>>> @@ -267,6 +268,11 @@ void qdev_init_nofail(DeviceState *dev) > >> >>>>>> /* Unlink device from bus and free the structure. */ > >> >>>>>> void qdev_free(DeviceState *dev) > >> >>>>>> { > >> >>>>>> + if (dev->id) { > >> >>>>>> + QObject *data = qobject_from_jsonf("{ 'device': %s }", dev->id); > >> >>>>>> + monitor_protocol_event(QEVENT_DEVICE_DELETED, data); > >> >>>>>> + qobject_decref(data); > >> >>>>>> + } > >> >>>>>> object_unparent(OBJECT(dev)); > >> >>>>>> } > >> >>>>>> > >> >>>>> > >> >>>>> I'm pretty sure this is the wrong place to fire the notification. We > >> >>>>> should rather do this when the device is actually deleted - which > >> >>>>> qdev_free() does *not* actually guarantee, as criticized in the s390x > >> >>>>> and unref'ing contexts. > >> >>>>> I would suggest to place your code into device_unparent() instead. > >> >>>>> > >> >>>>> Another thing to consider is what data to pass to the event: Not all > >> >>>>> devices have an ID. > >> >>>> > >> >>>> If they don't they were not created by management so management is > >> >>>> probably not interested in them being removed. > >> >>>> > >> >>>> We could always add a 'path' key later if this assumption > >> >>>> proves incorrect. > >> >>> > >> >>> In old qdev, ID was all we had, because paths were busted. Thus, > >> >>> management had no choice but use IDs. > >> >>> > >> >>> If I understand modern qdev correctly, we got a canonical path. Old > >> >>> APIs like device_del still accept only ID. Should new APIs still be > >> >>> designed that way? Or should they always accept / provide the canonical > >> >>> path, plus optional ID for convenience? > >> >> > >> >> What are advantages of exposing the path to users in this way? > >> > >> The path is the device's canonical name. Canonical means path:device is > >> 1:1. Path always works. Qdev ID only works when the user assigned one. > >> > >> Funny case: board creates a hot-pluggable device by default (thus no > >> qdev ID), guest ejects it, what do you put into the event? Your code > >> simply doesn't emit one. > >> > >> You could blame the user; after all he could've used -nodefaults, and > >> added the device himself, with an ID. > >> > >> I blame your design instead, which needlessly complicates the event's > >> semantics: it gets emitted only for devices with a qdev ID. Which you > >> neglected to document clearly, by the way. > > > > Good point, I'll document this. > > > >> If you put the path into the event, you can emit it always, which is > >> simpler. Feel free to throw in the qdev ID. > > > > I don't blame anyone. User not assigning an id is a clear indication > > that user does not care about the lifetime of this device. > > > >> >> Looks like maintainance hassle without real benefits? > >> > >> I can't see path being a greater maintenance hassle than ID. > > > > Sure, the less events we emit the less we need to support. > > You want to expose all kind of internal events, > > then management will come to depend on it and > > we'll have to maintain them forever. > > Misunderstanding. I'm *not* asking for more events. I'm asking for the > DEVICE_DELETED event to carry the device's canonical name: its QOM path. > > >> > Anthony had rejected earlier QOM patches by Paolo related to qdev id, > >> > saying it was deprecated in favor of those QOM paths. > >> > >> More reason to put the path into the event, not just the qdev ID. > > > > libvirt does not seems to want it there. We'll always be able to > > add info but will never be able to remove info, keep it minimal. > > Yes, adding members to an event is easy. Doesn't mean we should do it > just for the heck of it. If we don't need a member now, and we think > there's a chance we won't need in the future, then we probably shouldn't > add it now. > > I believe the chance of not needing the QOM path is effectively zero. > > Moreover, we'd add not just a member in this case, we'd add a *trigger*. > > Before: the event gets emitted only for devices with a qdev ID. > > After: the event gets emitted for all devices. > > I very much prefer the latter, because it's simpler. > > [...] I still don't see why it's useful for anyone. For now I hear from the libvirt guys that this patch does exactly what they need so I'll keep it simple. You are welcome to send a follow-up patch adding a path and more triggers, I won't object.
Am 07.03.2013 19:12, schrieb Michael S. Tsirkin: > On Thu, Mar 07, 2013 at 06:23:46PM +0100, Markus Armbruster wrote: >> "Michael S. Tsirkin" <mst@redhat.com> writes: >> >>> On Thu, Mar 07, 2013 at 03:14:15PM +0100, Markus Armbruster wrote: >>>> Andreas Färber <afaerber@suse.de> writes: >>>> >>>>> Am 07.03.2013 11:07, schrieb Michael S. Tsirkin: >>>>>> On Thu, Mar 07, 2013 at 10:55:23AM +0100, Markus Armbruster wrote: >>>>>>> "Michael S. Tsirkin" <mst@redhat.com> writes: >>>>>>> >>>>>>>> On Wed, Mar 06, 2013 at 02:57:22PM +0100, Andreas Färber wrote: >>>>>>>>> Am 06.03.2013 14:00, schrieb Michael S. Tsirkin: >>>>>>>>>> libvirt has a long-standing bug: when removing the device, >>>>>>>>>> it can request removal but does not know when does the >>>>>>>>>> removal complete. Add an event so we can fix this in a robust way. >>>>>>>>>> >>>>>>>>>> Signed-off-by: Michael S. Tsirkin <mst@redhat.com> >>>>>>>>> >>>>>>>>> Sounds like a good idea to me. :) >>>>>>>>> >>>>>>>>> [...] >>>>>>>>>> diff --git a/hw/qdev.c b/hw/qdev.c >>>>>>>>>> index 689cd54..f30d251 100644 >>>>>>>>>> --- a/hw/qdev.c >>>>>>>>>> +++ b/hw/qdev.c >>>>>>>>>> @@ -29,6 +29,7 @@ >>>>>>>>>> #include "sysemu/sysemu.h" >>>>>>>>>> #include "qapi/error.h" >>>>>>>>>> #include "qapi/visitor.h" >>>>>>>>>> +#include "qapi/qmp/qjson.h" >>>>>>>>>> >>>>>>>>>> int qdev_hotplug = 0; >>>>>>>>>> static bool qdev_hot_added = false; >>>>>>>>>> @@ -267,6 +268,11 @@ void qdev_init_nofail(DeviceState *dev) >>>>>>>>>> /* Unlink device from bus and free the structure. */ >>>>>>>>>> void qdev_free(DeviceState *dev) >>>>>>>>>> { >>>>>>>>>> + if (dev->id) { >>>>>>>>>> + QObject *data = qobject_from_jsonf("{ 'device': %s }", dev->id); >>>>>>>>>> + monitor_protocol_event(QEVENT_DEVICE_DELETED, data); >>>>>>>>>> + qobject_decref(data); >>>>>>>>>> + } >>>>>>>>>> object_unparent(OBJECT(dev)); >>>>>>>>>> } >>>>>>>>>> >>>>>>>>> >>>>>>>>> I'm pretty sure this is the wrong place to fire the notification. We >>>>>>>>> should rather do this when the device is actually deleted - which >>>>>>>>> qdev_free() does *not* actually guarantee, as criticized in the s390x >>>>>>>>> and unref'ing contexts. >>>>>>>>> I would suggest to place your code into device_unparent() instead. >>>>>>>>> >>>>>>>>> Another thing to consider is what data to pass to the event: Not all >>>>>>>>> devices have an ID. >>>>>>>> >>>>>>>> If they don't they were not created by management so management is >>>>>>>> probably not interested in them being removed. >>>>>>>> >>>>>>>> We could always add a 'path' key later if this assumption >>>>>>>> proves incorrect. >>>>>>> >>>>>>> In old qdev, ID was all we had, because paths were busted. Thus, >>>>>>> management had no choice but use IDs. >>>>>>> >>>>>>> If I understand modern qdev correctly, we got a canonical path. Old >>>>>>> APIs like device_del still accept only ID. Should new APIs still be >>>>>>> designed that way? Or should they always accept / provide the canonical >>>>>>> path, plus optional ID for convenience? >>>>>> >>>>>> What are advantages of exposing the path to users in this way? >>>> >>>> The path is the device's canonical name. Canonical means path:device is >>>> 1:1. Path always works. Qdev ID only works when the user assigned one. >>>> >>>> Funny case: board creates a hot-pluggable device by default (thus no >>>> qdev ID), guest ejects it, what do you put into the event? Your code >>>> simply doesn't emit one. >>>> >>>> You could blame the user; after all he could've used -nodefaults, and >>>> added the device himself, with an ID. >>>> >>>> I blame your design instead, which needlessly complicates the event's >>>> semantics: it gets emitted only for devices with a qdev ID. Which you >>>> neglected to document clearly, by the way. >>> >>> Good point, I'll document this. >>> >>>> If you put the path into the event, you can emit it always, which is >>>> simpler. Feel free to throw in the qdev ID. >>> >>> I don't blame anyone. User not assigning an id is a clear indication >>> that user does not care about the lifetime of this device. >>> >>>>>> Looks like maintainance hassle without real benefits? >>>> >>>> I can't see path being a greater maintenance hassle than ID. >>> >>> Sure, the less events we emit the less we need to support. >>> You want to expose all kind of internal events, >>> then management will come to depend on it and >>> we'll have to maintain them forever. >> >> Misunderstanding. I'm *not* asking for more events. I'm asking for the >> DEVICE_DELETED event to carry the device's canonical name: its QOM path. >> >>>>> Anthony had rejected earlier QOM patches by Paolo related to qdev id, >>>>> saying it was deprecated in favor of those QOM paths. >>>> >>>> More reason to put the path into the event, not just the qdev ID. >>> >>> libvirt does not seems to want it there. We'll always be able to >>> add info but will never be able to remove info, keep it minimal. >> >> Yes, adding members to an event is easy. Doesn't mean we should do it >> just for the heck of it. If we don't need a member now, and we think >> there's a chance we won't need in the future, then we probably shouldn't >> add it now. >> >> I believe the chance of not needing the QOM path is effectively zero. >> >> Moreover, we'd add not just a member in this case, we'd add a *trigger*. >> >> Before: the event gets emitted only for devices with a qdev ID. >> >> After: the event gets emitted for all devices. >> >> I very much prefer the latter, because it's simpler. >> >> [...] > > I still don't see why it's useful for anyone. For now I hear from the > libvirt guys that this patch does exactly what they need so I'll keep it > simple. You are welcome to send a follow-up patch adding a path > and more triggers, I won't object. Well, the libvirt guys have been told to poll using qom-list, which needs the path, not an ID. Using it in both places would make it symmetrical - that may qualify as useful. (I'm not aware of any id -> path lookup QMP command.) Nontheless, you can retain my Reviewed-by on v4+ as long as the code in hw/qdev.c doesn't change. Andreas
On Thu, Mar 07, 2013 at 08:00:29PM +0100, Andreas Färber wrote: > Am 07.03.2013 19:12, schrieb Michael S. Tsirkin: > > On Thu, Mar 07, 2013 at 06:23:46PM +0100, Markus Armbruster wrote: > >> "Michael S. Tsirkin" <mst@redhat.com> writes: > >> > >>> On Thu, Mar 07, 2013 at 03:14:15PM +0100, Markus Armbruster wrote: > >>>> Andreas Färber <afaerber@suse.de> writes: > >>>> > >>>>> Am 07.03.2013 11:07, schrieb Michael S. Tsirkin: > >>>>>> On Thu, Mar 07, 2013 at 10:55:23AM +0100, Markus Armbruster wrote: > >>>>>>> "Michael S. Tsirkin" <mst@redhat.com> writes: > >>>>>>> > >>>>>>>> On Wed, Mar 06, 2013 at 02:57:22PM +0100, Andreas Färber wrote: > >>>>>>>>> Am 06.03.2013 14:00, schrieb Michael S. Tsirkin: > >>>>>>>>>> libvirt has a long-standing bug: when removing the device, > >>>>>>>>>> it can request removal but does not know when does the > >>>>>>>>>> removal complete. Add an event so we can fix this in a robust way. > >>>>>>>>>> > >>>>>>>>>> Signed-off-by: Michael S. Tsirkin <mst@redhat.com> > >>>>>>>>> > >>>>>>>>> Sounds like a good idea to me. :) > >>>>>>>>> > >>>>>>>>> [...] > >>>>>>>>>> diff --git a/hw/qdev.c b/hw/qdev.c > >>>>>>>>>> index 689cd54..f30d251 100644 > >>>>>>>>>> --- a/hw/qdev.c > >>>>>>>>>> +++ b/hw/qdev.c > >>>>>>>>>> @@ -29,6 +29,7 @@ > >>>>>>>>>> #include "sysemu/sysemu.h" > >>>>>>>>>> #include "qapi/error.h" > >>>>>>>>>> #include "qapi/visitor.h" > >>>>>>>>>> +#include "qapi/qmp/qjson.h" > >>>>>>>>>> > >>>>>>>>>> int qdev_hotplug = 0; > >>>>>>>>>> static bool qdev_hot_added = false; > >>>>>>>>>> @@ -267,6 +268,11 @@ void qdev_init_nofail(DeviceState *dev) > >>>>>>>>>> /* Unlink device from bus and free the structure. */ > >>>>>>>>>> void qdev_free(DeviceState *dev) > >>>>>>>>>> { > >>>>>>>>>> + if (dev->id) { > >>>>>>>>>> + QObject *data = qobject_from_jsonf("{ 'device': %s }", dev->id); > >>>>>>>>>> + monitor_protocol_event(QEVENT_DEVICE_DELETED, data); > >>>>>>>>>> + qobject_decref(data); > >>>>>>>>>> + } > >>>>>>>>>> object_unparent(OBJECT(dev)); > >>>>>>>>>> } > >>>>>>>>>> > >>>>>>>>> > >>>>>>>>> I'm pretty sure this is the wrong place to fire the notification. We > >>>>>>>>> should rather do this when the device is actually deleted - which > >>>>>>>>> qdev_free() does *not* actually guarantee, as criticized in the s390x > >>>>>>>>> and unref'ing contexts. > >>>>>>>>> I would suggest to place your code into device_unparent() instead. > >>>>>>>>> > >>>>>>>>> Another thing to consider is what data to pass to the event: Not all > >>>>>>>>> devices have an ID. > >>>>>>>> > >>>>>>>> If they don't they were not created by management so management is > >>>>>>>> probably not interested in them being removed. > >>>>>>>> > >>>>>>>> We could always add a 'path' key later if this assumption > >>>>>>>> proves incorrect. > >>>>>>> > >>>>>>> In old qdev, ID was all we had, because paths were busted. Thus, > >>>>>>> management had no choice but use IDs. > >>>>>>> > >>>>>>> If I understand modern qdev correctly, we got a canonical path. Old > >>>>>>> APIs like device_del still accept only ID. Should new APIs still be > >>>>>>> designed that way? Or should they always accept / provide the canonical > >>>>>>> path, plus optional ID for convenience? > >>>>>> > >>>>>> What are advantages of exposing the path to users in this way? > >>>> > >>>> The path is the device's canonical name. Canonical means path:device is > >>>> 1:1. Path always works. Qdev ID only works when the user assigned one. > >>>> > >>>> Funny case: board creates a hot-pluggable device by default (thus no > >>>> qdev ID), guest ejects it, what do you put into the event? Your code > >>>> simply doesn't emit one. > >>>> > >>>> You could blame the user; after all he could've used -nodefaults, and > >>>> added the device himself, with an ID. > >>>> > >>>> I blame your design instead, which needlessly complicates the event's > >>>> semantics: it gets emitted only for devices with a qdev ID. Which you > >>>> neglected to document clearly, by the way. > >>> > >>> Good point, I'll document this. > >>> > >>>> If you put the path into the event, you can emit it always, which is > >>>> simpler. Feel free to throw in the qdev ID. > >>> > >>> I don't blame anyone. User not assigning an id is a clear indication > >>> that user does not care about the lifetime of this device. > >>> > >>>>>> Looks like maintainance hassle without real benefits? > >>>> > >>>> I can't see path being a greater maintenance hassle than ID. > >>> > >>> Sure, the less events we emit the less we need to support. > >>> You want to expose all kind of internal events, > >>> then management will come to depend on it and > >>> we'll have to maintain them forever. > >> > >> Misunderstanding. I'm *not* asking for more events. I'm asking for the > >> DEVICE_DELETED event to carry the device's canonical name: its QOM path. > >> > >>>>> Anthony had rejected earlier QOM patches by Paolo related to qdev id, > >>>>> saying it was deprecated in favor of those QOM paths. > >>>> > >>>> More reason to put the path into the event, not just the qdev ID. > >>> > >>> libvirt does not seems to want it there. We'll always be able to > >>> add info but will never be able to remove info, keep it minimal. > >> > >> Yes, adding members to an event is easy. Doesn't mean we should do it > >> just for the heck of it. If we don't need a member now, and we think > >> there's a chance we won't need in the future, then we probably shouldn't > >> add it now. > >> > >> I believe the chance of not needing the QOM path is effectively zero. > >> > >> Moreover, we'd add not just a member in this case, we'd add a *trigger*. > >> > >> Before: the event gets emitted only for devices with a qdev ID. > >> > >> After: the event gets emitted for all devices. > >> > >> I very much prefer the latter, because it's simpler. > >> > >> [...] > > > > I still don't see why it's useful for anyone. For now I hear from the > > libvirt guys that this patch does exactly what they need so I'll keep it > > simple. You are welcome to send a follow-up patch adding a path > > and more triggers, I won't object. > > Well, the libvirt guys have been told to poll using qom-list, which > needs the path, not an ID. Using it in both places would make it > symmetrical - that may qualify as useful. > (I'm not aware of any id -> path lookup QMP command.) > > Nontheless, you can retain my Reviewed-by on v4+ as long as the code in > hw/qdev.c doesn't change. > > Andreas I suggested retrying device_del, this has an advantage of working on more qemu version. > -- > SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany > GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
"Michael S. Tsirkin" <mst@redhat.com> writes: > On Thu, Mar 07, 2013 at 06:23:46PM +0100, Markus Armbruster wrote: >> "Michael S. Tsirkin" <mst@redhat.com> writes: >> >> > On Thu, Mar 07, 2013 at 03:14:15PM +0100, Markus Armbruster wrote: >> >> Andreas Färber <afaerber@suse.de> writes: >> >> >> >> > Am 07.03.2013 11:07, schrieb Michael S. Tsirkin: >> >> >> On Thu, Mar 07, 2013 at 10:55:23AM +0100, Markus Armbruster wrote: >> >> >>> "Michael S. Tsirkin" <mst@redhat.com> writes: >> >> >>> >> >> >>>> On Wed, Mar 06, 2013 at 02:57:22PM +0100, Andreas Färber wrote: >> >> >>>>> Am 06.03.2013 14:00, schrieb Michael S. Tsirkin: >> >> >>>>>> libvirt has a long-standing bug: when removing the device, >> >> >>>>>> it can request removal but does not know when does the >> >> >>>>>> removal complete. Add an event so we can fix this in a robust way. >> >> >>>>>> >> >> >>>>>> Signed-off-by: Michael S. Tsirkin <mst@redhat.com> >> >> >>>>> >> >> >>>>> Sounds like a good idea to me. :) >> >> >>>>> >> >> >>>>> [...] >> >> >>>>>> diff --git a/hw/qdev.c b/hw/qdev.c >> >> >>>>>> index 689cd54..f30d251 100644 >> >> >>>>>> --- a/hw/qdev.c >> >> >>>>>> +++ b/hw/qdev.c >> >> >>>>>> @@ -29,6 +29,7 @@ >> >> >>>>>> #include "sysemu/sysemu.h" >> >> >>>>>> #include "qapi/error.h" >> >> >>>>>> #include "qapi/visitor.h" >> >> >>>>>> +#include "qapi/qmp/qjson.h" >> >> >>>>>> >> >> >>>>>> int qdev_hotplug = 0; >> >> >>>>>> static bool qdev_hot_added = false; >> >> >>>>>> @@ -267,6 +268,11 @@ void qdev_init_nofail(DeviceState *dev) >> >> >>>>>> /* Unlink device from bus and free the structure. */ >> >> >>>>>> void qdev_free(DeviceState *dev) >> >> >>>>>> { >> >> >>>>>> + if (dev->id) { >> >> >>>>>> + QObject *data = qobject_from_jsonf("{ 'device': %s }", dev->id); >> >> >>>>>> + monitor_protocol_event(QEVENT_DEVICE_DELETED, data); >> >> >>>>>> + qobject_decref(data); >> >> >>>>>> + } >> >> >>>>>> object_unparent(OBJECT(dev)); >> >> >>>>>> } >> >> >>>>>> >> >> >>>>> >> >> >>>>> I'm pretty sure this is the wrong place to fire the notification. We >> >> >>>>> should rather do this when the device is actually deleted - which >> >> >>>>> qdev_free() does *not* actually guarantee, as criticized in the s390x >> >> >>>>> and unref'ing contexts. >> >> >>>>> I would suggest to place your code into device_unparent() instead. >> >> >>>>> >> >> >>>>> Another thing to consider is what data to pass to the event: Not all >> >> >>>>> devices have an ID. >> >> >>>> >> >> >>>> If they don't they were not created by management so management is >> >> >>>> probably not interested in them being removed. >> >> >>>> >> >> >>>> We could always add a 'path' key later if this assumption >> >> >>>> proves incorrect. >> >> >>> >> >> >>> In old qdev, ID was all we had, because paths were busted. Thus, >> >> >>> management had no choice but use IDs. >> >> >>> >> >> >>> If I understand modern qdev correctly, we got a canonical path. Old >> >> >>> APIs like device_del still accept only ID. Should new APIs still be >> >> >>> designed that way? Or should they always accept / provide >> >> >>> the canonical >> >> >>> path, plus optional ID for convenience? >> >> >> >> >> >> What are advantages of exposing the path to users in this way? >> >> >> >> The path is the device's canonical name. Canonical means path:device is >> >> 1:1. Path always works. Qdev ID only works when the user assigned one. >> >> >> >> Funny case: board creates a hot-pluggable device by default (thus no >> >> qdev ID), guest ejects it, what do you put into the event? Your code >> >> simply doesn't emit one. >> >> >> >> You could blame the user; after all he could've used -nodefaults, and >> >> added the device himself, with an ID. >> >> >> >> I blame your design instead, which needlessly complicates the event's >> >> semantics: it gets emitted only for devices with a qdev ID. Which you >> >> neglected to document clearly, by the way. >> > >> > Good point, I'll document this. >> > >> >> If you put the path into the event, you can emit it always, which is >> >> simpler. Feel free to throw in the qdev ID. >> > >> > I don't blame anyone. User not assigning an id is a clear indication >> > that user does not care about the lifetime of this device. >> > >> >> >> Looks like maintainance hassle without real benefits? >> >> >> >> I can't see path being a greater maintenance hassle than ID. >> > >> > Sure, the less events we emit the less we need to support. >> > You want to expose all kind of internal events, >> > then management will come to depend on it and >> > we'll have to maintain them forever. >> >> Misunderstanding. I'm *not* asking for more events. I'm asking for the >> DEVICE_DELETED event to carry the device's canonical name: its QOM path. >> >> >> > Anthony had rejected earlier QOM patches by Paolo related to qdev id, >> >> > saying it was deprecated in favor of those QOM paths. >> >> >> >> More reason to put the path into the event, not just the qdev ID. >> > >> > libvirt does not seems to want it there. We'll always be able to >> > add info but will never be able to remove info, keep it minimal. >> >> Yes, adding members to an event is easy. Doesn't mean we should do it >> just for the heck of it. If we don't need a member now, and we think >> there's a chance we won't need in the future, then we probably shouldn't >> add it now. >> >> I believe the chance of not needing the QOM path is effectively zero. >> >> Moreover, we'd add not just a member in this case, we'd add a *trigger*. >> >> Before: the event gets emitted only for devices with a qdev ID. >> >> After: the event gets emitted for all devices. >> >> I very much prefer the latter, because it's simpler. >> >> [...] > > I still don't see why it's useful for anyone. For now I hear from the > libvirt guys that this patch does exactly what they need so I'll keep it > simple. You're keeping it simple only in the sense of keeping it as it is. I think it's not as simple as it easily could be. Specifically, I want you to simplify the event's trigger, and add a path member. I'm convinced we'll have to do this anyway, so why not do it right right away, and simplify the future. This is based on my limited understanding of qdev and QOM. If the QOM folks tell me that we shouldn't use QOM paths to name the device, I'll gladly admit to be wrong, and retract my request. > You are welcome to send a follow-up patch adding a path > and more triggers, I won't object. The usual protocol for patch submission is maintainer (that would be me, until Luiz is back) reviews, submitter (that would be you) addresses review comments, within reason. I believe my request is well within reason. As a patch submitter, I've jumped through hoops I found less reasonable (to put it charitably) many times. You, as a maintainer, should know that asking the maintainer to address his review comments himself doesn't scale. If you think I'm an unreasonable maintainer, feel free to ignore me and try again when Luiz is back.
On Thu, Mar 07, 2013 at 09:18:48PM +0100, Markus Armbruster wrote: > "Michael S. Tsirkin" <mst@redhat.com> writes: > > > On Thu, Mar 07, 2013 at 06:23:46PM +0100, Markus Armbruster wrote: > >> "Michael S. Tsirkin" <mst@redhat.com> writes: > >> > >> > On Thu, Mar 07, 2013 at 03:14:15PM +0100, Markus Armbruster wrote: > >> >> Andreas Färber <afaerber@suse.de> writes: > >> >> > >> >> > Am 07.03.2013 11:07, schrieb Michael S. Tsirkin: > >> >> >> On Thu, Mar 07, 2013 at 10:55:23AM +0100, Markus Armbruster wrote: > >> >> >>> "Michael S. Tsirkin" <mst@redhat.com> writes: > >> >> >>> > >> >> >>>> On Wed, Mar 06, 2013 at 02:57:22PM +0100, Andreas Färber wrote: > >> >> >>>>> Am 06.03.2013 14:00, schrieb Michael S. Tsirkin: > >> >> >>>>>> libvirt has a long-standing bug: when removing the device, > >> >> >>>>>> it can request removal but does not know when does the > >> >> >>>>>> removal complete. Add an event so we can fix this in a robust way. > >> >> >>>>>> > >> >> >>>>>> Signed-off-by: Michael S. Tsirkin <mst@redhat.com> > >> >> >>>>> > >> >> >>>>> Sounds like a good idea to me. :) > >> >> >>>>> > >> >> >>>>> [...] > >> >> >>>>>> diff --git a/hw/qdev.c b/hw/qdev.c > >> >> >>>>>> index 689cd54..f30d251 100644 > >> >> >>>>>> --- a/hw/qdev.c > >> >> >>>>>> +++ b/hw/qdev.c > >> >> >>>>>> @@ -29,6 +29,7 @@ > >> >> >>>>>> #include "sysemu/sysemu.h" > >> >> >>>>>> #include "qapi/error.h" > >> >> >>>>>> #include "qapi/visitor.h" > >> >> >>>>>> +#include "qapi/qmp/qjson.h" > >> >> >>>>>> > >> >> >>>>>> int qdev_hotplug = 0; > >> >> >>>>>> static bool qdev_hot_added = false; > >> >> >>>>>> @@ -267,6 +268,11 @@ void qdev_init_nofail(DeviceState *dev) > >> >> >>>>>> /* Unlink device from bus and free the structure. */ > >> >> >>>>>> void qdev_free(DeviceState *dev) > >> >> >>>>>> { > >> >> >>>>>> + if (dev->id) { > >> >> >>>>>> + QObject *data = qobject_from_jsonf("{ 'device': %s }", dev->id); > >> >> >>>>>> + monitor_protocol_event(QEVENT_DEVICE_DELETED, data); > >> >> >>>>>> + qobject_decref(data); > >> >> >>>>>> + } > >> >> >>>>>> object_unparent(OBJECT(dev)); > >> >> >>>>>> } > >> >> >>>>>> > >> >> >>>>> > >> >> >>>>> I'm pretty sure this is the wrong place to fire the notification. We > >> >> >>>>> should rather do this when the device is actually deleted - which > >> >> >>>>> qdev_free() does *not* actually guarantee, as criticized in the s390x > >> >> >>>>> and unref'ing contexts. > >> >> >>>>> I would suggest to place your code into device_unparent() instead. > >> >> >>>>> > >> >> >>>>> Another thing to consider is what data to pass to the event: Not all > >> >> >>>>> devices have an ID. > >> >> >>>> > >> >> >>>> If they don't they were not created by management so management is > >> >> >>>> probably not interested in them being removed. > >> >> >>>> > >> >> >>>> We could always add a 'path' key later if this assumption > >> >> >>>> proves incorrect. > >> >> >>> > >> >> >>> In old qdev, ID was all we had, because paths were busted. Thus, > >> >> >>> management had no choice but use IDs. > >> >> >>> > >> >> >>> If I understand modern qdev correctly, we got a canonical path. Old > >> >> >>> APIs like device_del still accept only ID. Should new APIs still be > >> >> >>> designed that way? Or should they always accept / provide > >> >> >>> the canonical > >> >> >>> path, plus optional ID for convenience? > >> >> >> > >> >> >> What are advantages of exposing the path to users in this way? > >> >> > >> >> The path is the device's canonical name. Canonical means path:device is > >> >> 1:1. Path always works. Qdev ID only works when the user assigned one. > >> >> > >> >> Funny case: board creates a hot-pluggable device by default (thus no > >> >> qdev ID), guest ejects it, what do you put into the event? Your code > >> >> simply doesn't emit one. > >> >> > >> >> You could blame the user; after all he could've used -nodefaults, and > >> >> added the device himself, with an ID. > >> >> > >> >> I blame your design instead, which needlessly complicates the event's > >> >> semantics: it gets emitted only for devices with a qdev ID. Which you > >> >> neglected to document clearly, by the way. > >> > > >> > Good point, I'll document this. > >> > > >> >> If you put the path into the event, you can emit it always, which is > >> >> simpler. Feel free to throw in the qdev ID. > >> > > >> > I don't blame anyone. User not assigning an id is a clear indication > >> > that user does not care about the lifetime of this device. > >> > > >> >> >> Looks like maintainance hassle without real benefits? > >> >> > >> >> I can't see path being a greater maintenance hassle than ID. > >> > > >> > Sure, the less events we emit the less we need to support. > >> > You want to expose all kind of internal events, > >> > then management will come to depend on it and > >> > we'll have to maintain them forever. > >> > >> Misunderstanding. I'm *not* asking for more events. I'm asking for the > >> DEVICE_DELETED event to carry the device's canonical name: its QOM path. > >> > >> >> > Anthony had rejected earlier QOM patches by Paolo related to qdev id, > >> >> > saying it was deprecated in favor of those QOM paths. > >> >> > >> >> More reason to put the path into the event, not just the qdev ID. > >> > > >> > libvirt does not seems to want it there. We'll always be able to > >> > add info but will never be able to remove info, keep it minimal. > >> > >> Yes, adding members to an event is easy. Doesn't mean we should do it > >> just for the heck of it. If we don't need a member now, and we think > >> there's a chance we won't need in the future, then we probably shouldn't > >> add it now. > >> > >> I believe the chance of not needing the QOM path is effectively zero. > >> > >> Moreover, we'd add not just a member in this case, we'd add a *trigger*. > >> > >> Before: the event gets emitted only for devices with a qdev ID. > >> > >> After: the event gets emitted for all devices. > >> > >> I very much prefer the latter, because it's simpler. > >> > >> [...] > > > > I still don't see why it's useful for anyone. For now I hear from the > > libvirt guys that this patch does exactly what they need so I'll keep it > > simple. > > You're keeping it simple only in the sense of keeping it as it is. I > think it's not as simple as it easily could be. Specifically, I want > you to simplify the event's trigger, and add a path member. I'm > convinced we'll have to do this anyway, so why not do it right right > away, and simplify the future. > > This is based on my limited understanding of qdev and QOM. If the QOM > folks tell me that we shouldn't use QOM paths to name the device, I'll > gladly admit to be wrong, and retract my request. > > > You are welcome to send a follow-up patch adding a path > > and more triggers, I won't object. > > The usual protocol for patch submission is maintainer (that would be me, > until Luiz is back) reviews, submitter (that would be you) addresses > review comments, within reason. > > I believe my request is well within reason. As a patch submitter, I've > jumped through hoops I found less reasonable (to put it charitably) many > times. > > You, as a maintainer, should know that asking the maintainer to address > his review comments himself doesn't scale. > > If you think I'm an unreasonable maintainer, feel free to ignore me and > try again when Luiz is back. I think it's a reasonable request, just not sure it's the right thing to do and don't see why it should block this patch when we can add functionality in a follow-up patch.
On 2013年03月08日 03:15, Michael S. Tsirkin wrote: > On Thu, Mar 07, 2013 at 08:00:29PM +0100, Andreas Färber wrote: >> Am 07.03.2013 19:12, schrieb Michael S. Tsirkin: >>> On Thu, Mar 07, 2013 at 06:23:46PM +0100, Markus Armbruster wrote: >>>> "Michael S. Tsirkin"<mst@redhat.com> writes: >>>> >>>>> On Thu, Mar 07, 2013 at 03:14:15PM +0100, Markus Armbruster wrote: >>>>>> Andreas Färber<afaerber@suse.de> writes: >>>>>> >>>>>>> Am 07.03.2013 11:07, schrieb Michael S. Tsirkin: >>>>>>>> On Thu, Mar 07, 2013 at 10:55:23AM +0100, Markus Armbruster wrote: >>>>>>>>> "Michael S. Tsirkin"<mst@redhat.com> writes: >>>>>>>>> >>>>>>>>>> On Wed, Mar 06, 2013 at 02:57:22PM +0100, Andreas Färber wrote: >>>>>>>>>>> Am 06.03.2013 14:00, schrieb Michael S. Tsirkin: >>>>>>>>>>>> libvirt has a long-standing bug: when removing the device, >>>>>>>>>>>> it can request removal but does not know when does the >>>>>>>>>>>> removal complete. Add an event so we can fix this in a robust way. >>>>>>>>>>>> >>>>>>>>>>>> Signed-off-by: Michael S. Tsirkin<mst@redhat.com> >>>>>>>>>>> >>>>>>>>>>> Sounds like a good idea to me. :) >>>>>>>>>>> >>>>>>>>>>> [...] >>>>>>>>>>>> diff --git a/hw/qdev.c b/hw/qdev.c >>>>>>>>>>>> index 689cd54..f30d251 100644 >>>>>>>>>>>> --- a/hw/qdev.c >>>>>>>>>>>> +++ b/hw/qdev.c >>>>>>>>>>>> @@ -29,6 +29,7 @@ >>>>>>>>>>>> #include "sysemu/sysemu.h" >>>>>>>>>>>> #include "qapi/error.h" >>>>>>>>>>>> #include "qapi/visitor.h" >>>>>>>>>>>> +#include "qapi/qmp/qjson.h" >>>>>>>>>>>> >>>>>>>>>>>> int qdev_hotplug = 0; >>>>>>>>>>>> static bool qdev_hot_added = false; >>>>>>>>>>>> @@ -267,6 +268,11 @@ void qdev_init_nofail(DeviceState *dev) >>>>>>>>>>>> /* Unlink device from bus and free the structure. */ >>>>>>>>>>>> void qdev_free(DeviceState *dev) >>>>>>>>>>>> { >>>>>>>>>>>> + if (dev->id) { >>>>>>>>>>>> + QObject *data = qobject_from_jsonf("{ 'device': %s }", dev->id); >>>>>>>>>>>> + monitor_protocol_event(QEVENT_DEVICE_DELETED, data); >>>>>>>>>>>> + qobject_decref(data); >>>>>>>>>>>> + } >>>>>>>>>>>> object_unparent(OBJECT(dev)); >>>>>>>>>>>> } >>>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> I'm pretty sure this is the wrong place to fire the notification. We >>>>>>>>>>> should rather do this when the device is actually deleted - which >>>>>>>>>>> qdev_free() does *not* actually guarantee, as criticized in the s390x >>>>>>>>>>> and unref'ing contexts. >>>>>>>>>>> I would suggest to place your code into device_unparent() instead. >>>>>>>>>>> >>>>>>>>>>> Another thing to consider is what data to pass to the event: Not all >>>>>>>>>>> devices have an ID. >>>>>>>>>> >>>>>>>>>> If they don't they were not created by management so management is >>>>>>>>>> probably not interested in them being removed. >>>>>>>>>> >>>>>>>>>> We could always add a 'path' key later if this assumption >>>>>>>>>> proves incorrect. >>>>>>>>> >>>>>>>>> In old qdev, ID was all we had, because paths were busted. Thus, >>>>>>>>> management had no choice but use IDs. >>>>>>>>> >>>>>>>>> If I understand modern qdev correctly, we got a canonical path. Old >>>>>>>>> APIs like device_del still accept only ID. Should new APIs still be >>>>>>>>> designed that way? Or should they always accept / provide the canonical >>>>>>>>> path, plus optional ID for convenience? >>>>>>>> >>>>>>>> What are advantages of exposing the path to users in this way? >>>>>> >>>>>> The path is the device's canonical name. Canonical means path:device is >>>>>> 1:1. Path always works. Qdev ID only works when the user assigned one. >>>>>> >>>>>> Funny case: board creates a hot-pluggable device by default (thus no >>>>>> qdev ID), guest ejects it, what do you put into the event? Your code >>>>>> simply doesn't emit one. >>>>>> >>>>>> You could blame the user; after all he could've used -nodefaults, and >>>>>> added the device himself, with an ID. >>>>>> >>>>>> I blame your design instead, which needlessly complicates the event's >>>>>> semantics: it gets emitted only for devices with a qdev ID. Which you >>>>>> neglected to document clearly, by the way. >>>>> >>>>> Good point, I'll document this. >>>>> >>>>>> If you put the path into the event, you can emit it always, which is >>>>>> simpler. Feel free to throw in the qdev ID. >>>>> >>>>> I don't blame anyone. User not assigning an id is a clear indication >>>>> that user does not care about the lifetime of this device. >>>>> >>>>>>>> Looks like maintainance hassle without real benefits? >>>>>> >>>>>> I can't see path being a greater maintenance hassle than ID. >>>>> >>>>> Sure, the less events we emit the less we need to support. >>>>> You want to expose all kind of internal events, >>>>> then management will come to depend on it and >>>>> we'll have to maintain them forever. >>>> >>>> Misunderstanding. I'm *not* asking for more events. I'm asking for the >>>> DEVICE_DELETED event to carry the device's canonical name: its QOM path. >>>> >>>>>>> Anthony had rejected earlier QOM patches by Paolo related to qdev id, >>>>>>> saying it was deprecated in favor of those QOM paths. >>>>>> >>>>>> More reason to put the path into the event, not just the qdev ID. >>>>> >>>>> libvirt does not seems to want it there. We'll always be able to >>>>> add info but will never be able to remove info, keep it minimal. >>>> >>>> Yes, adding members to an event is easy. Doesn't mean we should do it >>>> just for the heck of it. If we don't need a member now, and we think >>>> there's a chance we won't need in the future, then we probably shouldn't >>>> add it now. >>>> >>>> I believe the chance of not needing the QOM path is effectively zero. >>>> >>>> Moreover, we'd add not just a member in this case, we'd add a *trigger*. >>>> >>>> Before: the event gets emitted only for devices with a qdev ID. >>>> >>>> After: the event gets emitted for all devices. >>>> >>>> I very much prefer the latter, because it's simpler. >>>> >>>> [...] >>> >>> I still don't see why it's useful for anyone. For now I hear from the >>> libvirt guys that this patch does exactly what they need so I'll keep it >>> simple. You are welcome to send a follow-up patch adding a path >>> and more triggers, I won't object. >> >> Well, the libvirt guys have been told to poll using qom-list, which >> needs the path, not an ID. Using it in both places would make it >> symmetrical - that may qualify as useful. >> (I'm not aware of any id -> path lookup QMP command.) >> >> Nontheless, you can retain my Reviewed-by on v4+ as long as the code in >> hw/qdev.c doesn't change. >> >> Andreas > > I suggested retrying device_del, this has an advantage of working > on more qemu version. I'm wondering if it could be long time to wait for the device_del completes (AFAIK from previous bugs, it can be, though it should be fine for most of the cases). If it's too long, it will be a problem for management, because it looks like hanging. We can have a timeout for the device_del in libvirt, but the problem is the device_del can be still in progress by qemu, which could cause the inconsistency. Unless qemu has some command to cancel the device_del. Osier
Osier Yang <jyang@redhat.com> writes: > I'm wondering if it could be long time to wait for the device_del > completes (AFAIK from previous bugs, it can be, though it should be > fine for most of the cases). If it's too long, it will be a problem > for management, because it looks like hanging. We can have a timeout > for the device_del in libvirt, but the problem is the device_del > can be still in progress by qemu, which could cause the inconsistency. > Unless qemu has some command to cancel the device_del. I'm afraid cancelling isn't possible, at least not for PCI. Here's how device_del works for PCI when it works, roughly: 1. device_del asks the device model to unplug itself. 2. PCI device models delegate the job to the device model providing their PCI bus. Let's assume it's our PIIX3/PIIX4 mongrel. That one puts an unplug request into PIIX4 function 3 where guest ACPI can see it, and triggers its interrupt. Then it immediately sends the QMP success reply. 3. Guest ACPI (SeaBIOS) services the interrupt. It finds the unplug request, and asks the guest OS nicely to give up the device. 4. If the guest OS has a working ACPI driver, and it feels like giving up the device, it does so, and tells ACPI when it's done. 5. Guest ACPI cleans up whatever it needs cleaned up, and signals successful unplug by writing the slog number to a PIIX4 function 3 register. 6. The PIIX device destroys the device in that slot. I call this the ACPI unplug dance. We don't control steps 3..5. There's no way for the guest to tell us "I got your unplug request, but I'm not going to honor it". Even if their was, a guest without a working ACPI driver wouldn't use it, so we couldn't rely on it anyway. There's no way for us to tell the guest "I changed my mind on this unplug". All we can do is wait and see. Either the device goes away, or it stays. Native PCIe is different, I'm told, but I know even less of that than I know of PCI/ACPI.
On Fri, Mar 08, 2013 at 09:50:55 +0100, Markus Armbruster wrote: > Osier Yang <jyang@redhat.com> writes: > > > I'm wondering if it could be long time to wait for the device_del > > completes (AFAIK from previous bugs, it can be, though it should be > > fine for most of the cases). If it's too long, it will be a problem > > for management, because it looks like hanging. We can have a timeout > > for the device_del in libvirt, but the problem is the device_del > > can be still in progress by qemu, which could cause the inconsistency. > > Unless qemu has some command to cancel the device_del. > > I'm afraid cancelling isn't possible, at least not for PCI. I don't think we need anything like that. We just need the device deletion API to return immediately without actually removing stuff from domain definition (unless the device was really removed fast enough, e.g., USB devices are removed before device_del returns) and then remove the device from domain definition when we get the event from QEMU or when libvirtd reconnects to a domain and sees a particular device is no longer present. After all, devices may be removed even if we didn't ask for it (when the removal is initiated by a guest OS). And we should also provide similar event for higher level apps. The question is whether we can make use of our existing API or if we need to introduce a new one. But that's of little relevance to qemu-devel I guess. Jirka
On 2013年03月08日 17:25, Jiri Denemark wrote: > On Fri, Mar 08, 2013 at 09:50:55 +0100, Markus Armbruster wrote: >> Osier Yang<jyang@redhat.com> writes: >> >>> I'm wondering if it could be long time to wait for the device_del >>> completes (AFAIK from previous bugs, it can be, though it should be >>> fine for most of the cases). If it's too long, it will be a problem >>> for management, because it looks like hanging. We can have a timeout >>> for the device_del in libvirt, but the problem is the device_del >>> can be still in progress by qemu, which could cause the inconsistency. >>> Unless qemu has some command to cancel the device_del. >> >> I'm afraid cancelling isn't possible, at least not for PCI. > > I don't think we need anything like that. We just need the device > deletion API to return immediately without actually removing stuff from > domain definition (unless the device was really removed fast enough, > e.g., USB devices are removed before device_del returns) and then remove > the device from domain definition when we get the event from QEMU or > when libvirtd reconnects to a domain and sees a particular device is no > longer present. After all, devices may be removed even if we didn't ask > for it (when the removal is initiated by a guest OS). And we should also > provide similar event for higher level apps. Removing the device from domain config unless we get the event from qemu or find the device disappeared by polling makes sense. That's the mainly reason for we want the event and polling actually. But the problem is our APIs don't want to have long time hanging. If we don't change the APIs and return quickly just like what we do currently, it's confused for user, because when he wants to attach the device again while the device_del is still in progress, he will get the error like "Device ID *** is in used", however, our detaching APIs return success prior to that. I.E, if device_del needs long time to complete in some cases? can we live with it? Osier
On 2013年03月08日 16:50, Markus Armbruster wrote: > Osier Yang<jyang@redhat.com> writes: > >> I'm wondering if it could be long time to wait for the device_del >> completes (AFAIK from previous bugs, it can be, though it should be >> fine for most of the cases). If it's too long, it will be a problem >> for management, because it looks like hanging. We can have a timeout >> for the device_del in libvirt, but the problem is the device_del >> can be still in progress by qemu, which could cause the inconsistency. >> Unless qemu has some command to cancel the device_del. > > I'm afraid cancelling isn't possible, at least not for PCI. > > Here's how device_del works for PCI when it works, roughly: > > 1. device_del asks the device model to unplug itself. > > 2. PCI device models delegate the job to the device model providing > their PCI bus. Let's assume it's our PIIX3/PIIX4 mongrel. That one > puts an unplug request into PIIX4 function 3 where guest ACPI can see > it, and triggers its interrupt. Then it immediately sends the QMP > success reply. > > 3. Guest ACPI (SeaBIOS) services the interrupt. It finds the unplug > request, and asks the guest OS nicely to give up the device. > > 4. If the guest OS has a working ACPI driver, and it feels like giving > up the device, it does so, and tells ACPI when it's done. > > 5. Guest ACPI cleans up whatever it needs cleaned up, and signals > successful unplug by writing the slog number to a PIIX4 function 3 > register. > > 6. The PIIX device destroys the device in that slot. > > I call this the ACPI unplug dance. > > We don't control steps 3..5. > > There's no way for the guest to tell us "I got your unplug request, but > I'm not going to honor it". Even if their was, a guest without a > working ACPI driver wouldn't use it, so we couldn't rely on it anyway. > > There's no way for us to tell the guest "I changed my mind on this > unplug". All we can do is wait and see. Either the device goes away, > or it stays. Hum, as I replied to Jirka in later mail, IMHO it needs to change libvirt detaching APIs to either wait for the event or find the device is really removed by polling before returning success. But it sounds to me that how long it takes to wait or polling is really depended? Osier
Osier Yang <jyang@redhat.com> writes: > On 2013年03月08日 16:50, Markus Armbruster wrote: >> Osier Yang<jyang@redhat.com> writes: >> >>> I'm wondering if it could be long time to wait for the device_del >>> completes (AFAIK from previous bugs, it can be, though it should be >>> fine for most of the cases). If it's too long, it will be a problem >>> for management, because it looks like hanging. We can have a timeout >>> for the device_del in libvirt, but the problem is the device_del >>> can be still in progress by qemu, which could cause the inconsistency. >>> Unless qemu has some command to cancel the device_del. >> >> I'm afraid cancelling isn't possible, at least not for PCI. >> >> Here's how device_del works for PCI when it works, roughly: >> >> 1. device_del asks the device model to unplug itself. >> >> 2. PCI device models delegate the job to the device model providing >> their PCI bus. Let's assume it's our PIIX3/PIIX4 mongrel. That one >> puts an unplug request into PIIX4 function 3 where guest ACPI can see >> it, and triggers its interrupt. Then it immediately sends the QMP >> success reply. >> >> 3. Guest ACPI (SeaBIOS) services the interrupt. It finds the unplug >> request, and asks the guest OS nicely to give up the device. >> >> 4. If the guest OS has a working ACPI driver, and it feels like giving >> up the device, it does so, and tells ACPI when it's done. >> >> 5. Guest ACPI cleans up whatever it needs cleaned up, and signals >> successful unplug by writing the slog number to a PIIX4 function 3 >> register. >> >> 6. The PIIX device destroys the device in that slot. >> >> I call this the ACPI unplug dance. >> >> We don't control steps 3..5. >> >> There's no way for the guest to tell us "I got your unplug request, but >> I'm not going to honor it". Even if their was, a guest without a >> working ACPI driver wouldn't use it, so we couldn't rely on it anyway. >> >> There's no way for us to tell the guest "I changed my mind on this >> unplug". All we can do is wait and see. Either the device goes away, >> or it stays. > > Hum, as I replied to Jirka in later mail, IMHO it needs to change > libvirt detaching APIs to either wait for the event or find the > device is really removed by polling before returning success. But > it sounds to me that how long it takes to wait or polling is really > depended? Time between device_del and event DEVICE_DELETED event is *unbounded*. Could be instantaneous, could be never, could be anything in between. I'd expect it to be either fairly short or never most of the time in practice.
diff --git a/QMP/qmp-events.txt b/QMP/qmp-events.txt index b2698e4..ec5b810 100644 --- a/QMP/qmp-events.txt +++ b/QMP/qmp-events.txt @@ -136,6 +136,21 @@ Example: Note: The "ready to complete" status is always reset by a BLOCK_JOB_ERROR event. +DEVICE_DELETED +----------------- + +It's emitted whenever the device removal completion is acknowledged +by the guest. At this point, it's safe to reuse the specified device ID. +Device removal can be initiated by the guest or by HMP/QMP commands. + +Data: + +- "device": device name (json-string) + +{ "event": "DEVICE_DELETED", + "data": { "device": "virtio-net-pci-0" }, + "timestamp": { "seconds": 1265044230, "microseconds": 450486 } } + DEVICE_TRAY_MOVED ----------------- diff --git a/hw/qdev.c b/hw/qdev.c index 689cd54..bc24405 100644 --- a/hw/qdev.c +++ b/hw/qdev.c @@ -29,6 +29,7 @@ #include "sysemu/sysemu.h" #include "qapi/error.h" #include "qapi/visitor.h" +#include "qapi/qmp/qjson.h" int qdev_hotplug = 0; static bool qdev_hot_added = false; @@ -267,6 +268,11 @@ void qdev_init_nofail(DeviceState *dev) /* Unlink device from bus and free the structure. */ void qdev_free(DeviceState *dev) { + if (dev->id) { + QObject *data = qobject_from_jsonf("{ 'device': %s }", dev->id); + monitor_protocol_event(QEVENT_DEVICE_DELETED, data); + qobject_decref(data); + } object_unparent(OBJECT(dev)); } diff --git a/include/monitor/monitor.h b/include/monitor/monitor.h index 87fb49c..bf0436e 100644 --- a/include/monitor/monitor.h +++ b/include/monitor/monitor.h @@ -39,6 +39,7 @@ typedef enum MonitorEvent { QEVENT_BLOCK_JOB_CANCELLED, QEVENT_BLOCK_JOB_ERROR, QEVENT_BLOCK_JOB_READY, + QEVENT_DEVICE_DELETED, QEVENT_DEVICE_TRAY_MOVED, QEVENT_SUSPEND, QEVENT_SUSPEND_DISK, diff --git a/monitor.c b/monitor.c index 32a6e74..f3e06a8 100644 --- a/monitor.c +++ b/monitor.c @@ -457,6 +457,7 @@ static const char *monitor_event_names[] = { [QEVENT_BLOCK_JOB_CANCELLED] = "BLOCK_JOB_CANCELLED", [QEVENT_BLOCK_JOB_ERROR] = "BLOCK_JOB_ERROR", [QEVENT_BLOCK_JOB_READY] = "BLOCK_JOB_READY", + [QEVENT_DEVICE_DELETED] = "DEVICE_DELETED", [QEVENT_DEVICE_TRAY_MOVED] = "DEVICE_TRAY_MOVED", [QEVENT_SUSPEND] = "SUSPEND", [QEVENT_SUSPEND_DISK] = "SUSPEND_DISK", diff --git a/qapi-schema.json b/qapi-schema.json index 28b070f..9a23864 100644 --- a/qapi-schema.json +++ b/qapi-schema.json @@ -2354,7 +2354,9 @@ # Notes: When this command completes, the device may not be removed from the # guest. Hot removal is an operation that requires guest cooperation. # This command merely requests that the guest begin the hot removal -# process. +# process. Completion of the device removal process is signaled with a +# DEVICE_DELETED event. Guest reset will automatically complete removal +# for all devices. # # Since: 0.14.0 ## --- QMP/qmp-events.txt | 15 +++++++++++++++ hw/qdev.c | 6 ++++++ include/monitor/monitor.h | 1 + monitor.c | 1 + qapi-schema.json | 4 +++- 5 files changed, 26 insertions(+), 1 deletion(-) diff --git a/QMP/qmp-events.txt b/QMP/qmp-events.txt index b2698e4..74e0245 100644 --- a/QMP/qmp-events.txt +++ b/QMP/qmp-events.txt @@ -136,6 +136,21 @@ Example: Note: The "ready to complete" status is always reset by a BLOCK_JOB_ERROR event. +DEVICE_DELETED +----------------- + +It's emitted whenever the device removal completion is acknowledged +by the guest. At this point, it's safe to reuse the specified device ID. +Device removal can be initiated by the guest or by HMP/QMP commands. + +Data: + +- "device": device name (json-string) + +{ "event": "DEVICE_DELETED", + "data": { "device": "virtio-net-pci-0" }, + "timestamp": { "seconds": 1265044230, "microseconds": 450486 } } + DEVICE_TRAY_MOVED ----------------- diff --git a/hw/qdev.c b/hw/qdev.c index 689cd54..f30d251 100644 --- a/hw/qdev.c +++ b/hw/qdev.c @@ -29,6 +29,7 @@ #include "sysemu/sysemu.h" #include "qapi/error.h" #include "qapi/visitor.h" +#include "qapi/qmp/qjson.h" int qdev_hotplug = 0; static bool qdev_hot_added = false; @@ -267,6 +268,11 @@ void qdev_init_nofail(DeviceState *dev) /* Unlink device from bus and free the structure. */ void qdev_free(DeviceState *dev) { + if (dev->id) { + QObject *data = qobject_from_jsonf("{ 'device': %s }", dev->id); + monitor_protocol_event(QEVENT_DEVICE_DELETED, data); + qobject_decref(data); + } object_unparent(OBJECT(dev)); } diff --git a/include/monitor/monitor.h b/include/monitor/monitor.h index 87fb49c..b868760 100644 --- a/include/monitor/monitor.h +++ b/include/monitor/monitor.h @@ -39,6 +39,7 @@ typedef enum MonitorEvent { QEVENT_BLOCK_JOB_CANCELLED, QEVENT_BLOCK_JOB_ERROR, QEVENT_BLOCK_JOB_READY, + QEVENT_DEVICE_DELETED, QEVENT_DEVICE_TRAY_MOVED, QEVENT_SUSPEND, QEVENT_SUSPEND_DISK, diff --git a/monitor.c b/monitor.c index 32a6e74..2a5e7b6 100644 --- a/monitor.c +++ b/monitor.c @@ -457,6 +457,7 @@ static const char *monitor_event_names[] = { [QEVENT_BLOCK_JOB_CANCELLED] = "BLOCK_JOB_CANCELLED", [QEVENT_BLOCK_JOB_ERROR] = "BLOCK_JOB_ERROR", [QEVENT_BLOCK_JOB_READY] = "BLOCK_JOB_READY", + [QEVENT_DEVICE_DELETED] = "DEVICE_DELETED", [QEVENT_DEVICE_TRAY_MOVED] = "DEVICE_TRAY_MOVED", [QEVENT_SUSPEND] = "SUSPEND", [QEVENT_SUSPEND_DISK] = "SUSPEND_DISK", diff --git a/qapi-schema.json b/qapi-schema.json index 28b070f..bb361e1 100644 --- a/qapi-schema.json +++ b/qapi-schema.json @@ -2354,7 +2354,9 @@ # Notes: When this command completes, the device may not be removed from the # guest. Hot removal is an operation that requires guest cooperation. # This command merely requests that the guest begin the hot removal -# process. +# process. Completion of the device removal process is signaled with a +# DEVICE_DELETED event. Guest reset will automatically complete removal +# for all devices. # # Since: 0.14.0 ##
libvirt has a long-standing bug: when removing the device, it can request removal but does not know when does the removal complete. Add an event so we can fix this in a robust way. Signed-off-by: Michael S. Tsirkin <mst@redhat.com>