qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Michael S. Tsirkin
Subject: Re: [Qemu-devel] [PATCH] qdev: DEVICE_DELETED event
Date: Thu, 7 Mar 2013 18:35:40 +0200

On Thu, Mar 07, 2013 at 03:14:15PM +0100, Markus Armbruster wrote:
> Andreas Färber <address@hidden> 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" <address@hidden> 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 <address@hidden>
> >>>>>
> >>>>> 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.



reply via email to

[Prev in Thread] Current Thread [Next in Thread]