From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:39455) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UFoje-0007x8-LH for qemu-devel@nongnu.org; Wed, 13 Mar 2013 12:41:06 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1UFojU-0005ja-77 for qemu-devel@nongnu.org; Wed, 13 Mar 2013 12:41:02 -0400 Received: from mx1.redhat.com ([209.132.183.28]:25462) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UFojU-0005j7-0O for qemu-devel@nongnu.org; Wed, 13 Mar 2013 12:40:52 -0400 Date: Wed, 13 Mar 2013 18:41:10 +0200 From: "Michael S. Tsirkin" Message-ID: <20130313164110.GA16126@redhat.com> References: <87zjy7215c.fsf@blackfin.pond.sub.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <87zjy7215c.fsf@blackfin.pond.sub.org> Subject: Re: [Qemu-devel] [PATCHv5 0/3] DEVICE_DELETED event List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Markus Armbruster Cc: Kevin Wolf , Anthony Liguori , Eduardo Habkost , libvir-list@redhat.com, Stefan Hajnoczi , qemu-devel@nongnu.org, Luiz Capitulino , Gerd Hoffmann , Paolo Bonzini , Andreas =?iso-8859-1?Q?F=E4rber?= On Wed, Mar 13, 2013 at 08:45:51AM +0100, Markus Armbruster wrote: > Anthony asked for a space between "PATCH" and "v" in the subject. > Please try to remember next time. > > "Michael S. Tsirkin" 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 > > 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