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

[Markus Armbruster]

"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.

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.

[...]