qemu-devel
[Top][All Lists]
Advanced

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

Re: [Qemu-devel] [PATCHv5 0/3] DEVICE_DELETED event


From: Michael S. Tsirkin
Subject: Re: [Qemu-devel] [PATCHv5 0/3] DEVICE_DELETED event
Date: Wed, 13 Mar 2013 18:41:10 +0200

On Wed, Mar 13, 2013 at 08:45:51AM +0100, Markus Armbruster wrote:
> Anthony asked for a space between "PATCH" and "v<N>" in the subject.
> Please try to remember next time.
> 
> "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.
> >
> > First patch only adds the event for devices with ID, second path adds it
> > for all devices and adds a path fields. Split this way for ease of
> > backport (stable downstreams without QOM would want to only take the
> > first patch),
> 
> I'd *strongly* advice downstreams to take the first patch plus the part
> of the third patch that changes the event trigger.
> 
> Let's compare the difference to upstream for both approaches.
> 
>     Just the first patch: event fires only when device has an ID.
>     Upstream: event fires always.
>     Subtle semantic difference.
> 
>     First patch + changed trigger: event doesn't have member path.
>     Upstream: event has member path.
>     Blatantly obvious syntactic difference.
> 
> I'd take syntactic over semantic any day.
> 
> Note that even without member path a QMP client can still find which
> device is gone, by polling.  Any client designed to keep track of state
> accurately across disconnect/reconnect (such as libvirt) needs such
> polling code anyway.

Ah, good point. Empty event seemed useless to me, now I see it isn't.

> If you want to make backporters' lives easier, move the event trigger
> change from patch 3 to patch 1.
> >                and also because the 2nd/3rd patches might turn out to be
> > more controversial - if they really turn
> > out such I'd like just the 1st one to get applied while we discuss 2 and
> > 3.
> 
> I don't expect more controversy.  If I'm wrong, I don't want just patch
> 1 applied while we discuss, because that creates an longer stretch in
> git where the event is there, but doesn't work like we want it to, for
> no appreciable gain.  We're in no particular hurry here, so let's do it
> right.
> 
> > Signed-off-by: Michael S. Tsirkin <address@hidden>
> 
> Options in decreasing order of preference, pick one that suits you:
> 
> 1. Go the extra mile for backporters, and move the event trigger change
>    from PATCH 3 into 1.

OK will do.

> 2. Squash PATCH 1 into 3.  Backporting trouble is obvious and easy to
>    resolve: just drop member path.  Feel free to point that out in the
>    commit message.
> 
> 3. Let the series stand as is.  Backporting trouble is subtle and easy
>    to miss: backporting just PATCH 1 is tempting, but screws up the
>    event trigger.  I wouldn't do it that way myself, but I'm not going
>    to NAK usable upstream work because of such downstream concerns.
> 
> In case you pick 3:
> 
> Acked-by: Markus Armbruster <address@hidden>



reply via email to

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