From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:44787) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZvpJ3-00058W-16 for qemu-devel@nongnu.org; Mon, 09 Nov 2015 11:28:33 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZvpJ2-0005Gv-3Q for qemu-devel@nongnu.org; Mon, 09 Nov 2015 11:28:32 -0500 References: <1446663467-22485-1-git-send-email-mreitz@redhat.com> <1446663467-22485-13-git-send-email-mreitz@redhat.com> From: Max Reitz Message-ID: <5640C9A2.7060908@redhat.com> Date: Mon, 9 Nov 2015 17:28:18 +0100 MIME-Version: 1.0 In-Reply-To: Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="xmVKTBjJ8CuosAVtG7H78Agrom4qogGVv" Subject: Re: [Qemu-devel] [PATCH v6 12/15] blockdev: Keep track of monitor-owned BDS List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Alberto Garcia , qemu-block@nongnu.org Cc: Kevin Wolf , qemu-devel@nongnu.org, Markus Armbruster , Stefan Hajnoczi , Paolo Bonzini This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --xmVKTBjJ8CuosAVtG7H78Agrom4qogGVv Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: quoted-printable On 09.11.2015 16:05, Alberto Garcia wrote: > On Wed 04 Nov 2015 07:57:44 PM CET, Max Reitz wrote: >> @@ -3519,11 +3537,18 @@ void qmp_x_blockdev_del(bool has_id, const cha= r *id, >> bdrv_get_device_or_node_name(bs)); >> goto out; >> } >> + >> + if (!blk && !bs->monitor_list.tqe_prev) { >> + error_setg(errp, "Node %s is not owned by the monitor", >> + bs->node_name); >> + goto out; >> + } >> } >> =20 >> if (blk) { >> blk_unref(blk); >> } else { >> + QTAILQ_REMOVE(&monitor_bdrv_states, bs, monitor_list); >> bdrv_unref(bs); >> } >=20 > blk_unref(blk) will also unref the BDS (if there's any), so you also > need to update monitor_bdrv_states in that case, don't you? If we get to blk_unref(blk), that means there is a BB attached to that BDS (if there is any). If that BDS is monitor-owned, that means its refcount is at least 2 (one from the BB, one from the monitor). Therefore, blockdev-del will fail before that point. > Anyway, wouldn't it make more sense to do this in bdrv_delete() ? No, I don't think so. The monitor_bdrv_states is not a plain list of BDSs, but actually contains strong references. Every time a BDS is entered there, that reference should be counted (which we don't actually have to call bdrv_ref() for since we're just keeping the initial refcount of 1 in qmp_blockdev_add()), and every time a BDS is removed, its reference should be dropped, just like is done here. That's the issue I had with this implementation of blockdev-del. It's just dropping references without actually having those references. Now we at least have the reference through monitor_bdrv_states, and in the future, we'll have such a list of monitor references for BBs, too. Max --xmVKTBjJ8CuosAVtG7H78Agrom4qogGVv Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iQEcBAEBCAAGBQJWQMmiAAoJEDuxQgLoOKyteGcIAKw8B5W1MBJ4gVvA2T2gSZKX LZEGPeTxtxVG3NoZ7NQOh7SBYVCsWhv0I3ZT3TYzrIe6OJMVY0lvo26QJSDeH6fn I8L39myOUcEMwWj6YjCSTZNUFDUItVEj+9HtG6RtfLpumuIbO1vpiLDcQ/Jb1JP7 fJ4ZXhSliWHwXz97UMTJ4vshJovNWUpM7cxEp5u7q06glyCc5e6Lt/351j9fQG7O zmc8ttGf7Km0FysjRRVWDmB0tIL7o6uVVRMlKuDD4cBDjJCr2Qdq0aSqAsx93vAR zPDKgXmp5TnxjdBqZqrBZM/2l0PqbmOn2fOENlABgREMqMxTL/XCdSE4Z/mtURw= =E3eI -----END PGP SIGNATURE----- --xmVKTBjJ8CuosAVtG7H78Agrom4qogGVv--