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

[Michael S. Tsirkin]

On Fri, Mar 08, 2013 at 08:58:43AM +0100, Markus Armbruster wrote:
> "Michael S. Tsirkin" <address@hidden> writes:
> 
> > On Thu, Mar 07, 2013 at 08:57:52PM +0100, Markus Armbruster wrote:
> >> "Michael S. Tsirkin" <address@hidden> writes:
> >> 
> >> > libvirt has a long-standing bug: when removing the device,
> >> > it can request removal but does not know when the
> >> > removal completes. Add an event so we can fix this in a robust way.
> >> >
> >> > Signed-off-by: Michael S. Tsirkin <address@hidden>
> >> 
> >> Speaking as the acting QMP maintainer, just to avoid misunderstandings:
> >> there's disagreement on the event's design, namely when it should fire,
> >> and how it should name the device.  I don't want the discussion
> >> preempted by a commit.
> >
> > Yes, you are asking for more functionality, but can I add this in a
> > follow-up commit please?  I prefer this patch as is, as it can be
> > backported to stable branches and downstreams.  Upstream a follow up
> > patch can add fields and more triggers which won't apply to any
> > downstreams.
> 
> If you want to address my review comments in a separate patch, go right
> ahead.  Please post both together as a series, for coherent review and
> to simplify patch tracking.

Sure.

> I'm asking for two things:
> 
> 1. Event member path.  Fair to call this "more functionality".  I agree
>    that backporting it to pre-QOM versions isn't practical.
> 
> 2. Sane event trigger condition: on any device deletion, not just when
>    the device happens to have a qdev ID.  This isn't "more", it's
>    "different".
> 
>    I'd definitely backport this part, because:
> 
>    * I abhor subtle semantic differences to upstream like a different
>      event trigger.
> 
>    * Backporting it reduces the difference to event member path missing.
>      Syntactic and in-your-face.
> 
>    * Without member path, the event triggered by deleting a device
>      without a qdev ID can't tell us which device went away.  But you
>      can find out using the polling code you need anyway.  Thus, the
>      event trigger is not only simpler and consistent with upstream, it
>      can also be more useful.
> 
> [...]

Will do, thanks for the comments.

-- 
MST