From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:35156) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ai1fe-0001G1-Iv for qemu-devel@nongnu.org; Mon, 21 Mar 2016 11:23:10 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ai1fa-0001St-F1 for qemu-devel@nongnu.org; Mon, 21 Mar 2016 11:23:06 -0400 Received: from mx1.redhat.com ([209.132.183.28]:53277) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ai1fa-0001Sn-7d for qemu-devel@nongnu.org; Mon, 21 Mar 2016 11:23:02 -0400 Received: from int-mx11.intmail.prod.int.phx2.redhat.com (int-mx11.intmail.prod.int.phx2.redhat.com [10.5.11.24]) by mx1.redhat.com (Postfix) with ESMTPS id B4B9B46215 for ; Mon, 21 Mar 2016 15:23:01 +0000 (UTC) From: Markus Armbruster References: <1456151945-11225-1-git-send-email-pbonzini@redhat.com> <1456151945-11225-3-git-send-email-pbonzini@redhat.com> Date: Mon, 21 Mar 2016 16:22:59 +0100 In-Reply-To: <1456151945-11225-3-git-send-email-pbonzini@redhat.com> (Paolo Bonzini's message of "Mon, 22 Feb 2016 15:39:04 +0100") Message-ID: <874mbz26uk.fsf@blackfin.pond.sub.org> MIME-Version: 1.0 Content-Type: text/plain Subject: Re: [Qemu-devel] [PATCH 2/3] block: keep BlockBackend alive until device finalize 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: > While the next patch will anticipate the death of the DriveInfo > data structure, the BlockBackend must survive after unrealize, > for example in case there are outstanding operations on it. > The good thing is that we can just use reference counting to > do it. > > Signed-off-by: Paolo Bonzini > --- > hw/core/qdev-properties-system.c | 19 ++++++++++++------- > 1 file changed, 12 insertions(+), 7 deletions(-) > > diff --git a/hw/core/qdev-properties-system.c b/hw/core/qdev-properties-system.c > index 469ba8a..5e84b55 100644 > --- a/hw/core/qdev-properties-system.c > +++ b/hw/core/qdev-properties-system.c > @@ -93,6 +93,7 @@ static void parse_drive(DeviceState *dev, const char *str, void **ptr, if (blk_attach_dev(blk, dev) < 0) { DriveInfo *dinfo = blk_legacy_dinfo(blk); if (dinfo->type != IF_NONE) { error_setg(errp, "Drive '%s' is already in use because " "it has been automatically connected to another " "device (did you need 'if=none' in the drive options?)", str); } else { error_setg(errp, "Drive '%s' is already in use by another device", str); } > return; > } > *ptr = blk; > + blk_ref(blk); blk_attach_dev() already takes a reference. I'm not sure I understand why you need to take a second one. You say "in case there are outstanding operations on it." What operations could that be? And shouldn't they take their own reference? I hasten to add that I'm not going to demand you fix them to take their own references. It's okay to take a hacky second reference here, then fix "them" at our leisure. But I need to understand what exactly this second reference protects. It probably needs to be explained in the source, too. > } > > static void release_drive(Object *obj, const char *name, void *opaque) > @@ -101,13 +102,17 @@ static void release_drive(Object *obj, const char *name, void *opaque) > Property *prop = opaque; > BlockBackend **ptr = qdev_get_prop_ptr(dev, prop); > > - if (*ptr && blk_get_attached_dev(*ptr) != NULL) { > - /* Unrealize has already called blk_detach_dev and blockdev_del_drive > - * if the device has been realized; in that case, blk_get_attached_dev > - * returns NULL. Thus, we get here if the device failed to realize, > - * and the -drive must not be released. > - */ > - blk_detach_dev(*ptr, dev); > + if (*ptr) { > + if (blk_get_attached_dev(*ptr) != NULL) { > + /* Unrealize has already called blk_detach_dev and > + * blockdev_del_drive if the device has been realized; > + * in that case, blk_get_attached_dev returns NULL. Thus, > + * we get here if the device failed to realize, and the > + * -drive must not be released. > + */ > + blk_detach_dev(*ptr, dev); > + } > + blk_unref(*ptr); > } > }