Re: [Qemu-devel] [PATCH] qdev: DEVICE_DELETED event

["Michael S. Tsirkin" <mst@redhat.com>]

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.

-- 
MST