Re: [libvirt] [Qemu-devel] [PATCHv5 0/3] DEVICE_DELETED event
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" <mst(a)redhat.com> 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 <mst(a)redhat.com>
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 <armbru(a)redhat.com>