From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:40448) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aieGa-0001FL-SC for qemu-devel@nongnu.org; Wed, 23 Mar 2016 04:35:49 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aieGV-0004YQ-RF for qemu-devel@nongnu.org; Wed, 23 Mar 2016 04:35:48 -0400 Received: from mx1.redhat.com ([209.132.183.28]:38929) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aieGV-0004YA-Fr for qemu-devel@nongnu.org; Wed, 23 Mar 2016 04:35:43 -0400 Received: from int-mx14.intmail.prod.int.phx2.redhat.com (int-mx14.intmail.prod.int.phx2.redhat.com [10.5.11.27]) by mx1.redhat.com (Postfix) with ESMTPS id D6BD47F0AB for ; Wed, 23 Mar 2016 08:35:42 +0000 (UTC) From: Markus Armbruster References: <1456151945-11225-1-git-send-email-pbonzini@redhat.com> <1456151945-11225-2-git-send-email-pbonzini@redhat.com> <87a8lr27b7.fsf@blackfin.pond.sub.org> <56F013EF.1000401@redhat.com> <8737rj7npp.fsf@blackfin.pond.sub.org> <56F02F9B.9040305@redhat.com> Date: Wed, 23 Mar 2016 09:35:39 +0100 In-Reply-To: <56F02F9B.9040305@redhat.com> (Paolo Bonzini's message of "Mon, 21 Mar 2016 18:30:03 +0100") Message-ID: <87h9fxd21w.fsf@blackfin.pond.sub.org> MIME-Version: 1.0 Content-Type: text/plain Subject: Re: [Qemu-devel] [PATCH 1/3] block: detach devices from DriveInfo at unrealize time List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Paolo Bonzini Cc: Kevin Wolf , qemu-devel@nongnu.org, Max Reitz Paolo Bonzini writes: > On 21/03/2016 18:19, Markus Armbruster wrote: >> Paolo Bonzini writes: >> >>> On 21/03/2016 16:13, Markus Armbruster wrote: >>>> Before your patch, we leave finalization of the property to its >>>> release() callback release_drive(), as we should. All we do here is >>>> schedule warty deletion. And that we must do here, because only here we >>>> know that warty deletion is wanted. >>>> >>>> Your patch inserts a copy of release_drive() and hacks it up a bit. Two >>>> hunks down, release_drive() gets hacked up to conditionally avoid >>>> repeating the job. >>>> >>>> This feels rather dirty to me. >>> >>> The other possibility is to make blk_detach_dev do nothing if blk->dev >>> == NULL, i.e. make it idempotent. On one hand, who doesn't like >>> idempotency; on the other hand, removing an assertion is also dirty. >>> >>> I chose the easy way here (changing as fewer contracts as possible). >> >> Why can't we keep the work in the property release() method >> release_drive()? >> >> The only reason blockdev_mark_auto_del() isn't there is that the device s/isn't there/exists/ (oops) >> decides whether to call it, not the property. > > DEVICE_DELETED is currently sent right after setting unrealized to false > (see device_unparent), and you cannnot send it later than that. In > particular release_drive would mean sending the drive when properties > are removed in instance_finalize; by that time you don't have anymore a > QOM path to include in the event. I see. To delay DEVICE_DELETED, we'd have to save the QOM path, and that would be bothersome. Still, copying code from property to devices with that property is undesirable. It's not that bad in this patch, because we copy only to the devices that do warty backend deletion, and that's just the two places where we call blockdev_mark_auto_del() now. However, it gets worse if we decide to extend warty backend deletion to *all* devices: more places, and a new need to consistently copy it to every new user of the drive property. When you find yourself copying code from a property callback into every device using it, the real problem might be you're missing a callback. In this case, one that runs at unrealize time. The existing release() runs at finalize time.