[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>