From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:51479) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZnVsG-0001wZ-TC for qemu-devel@nongnu.org; Sat, 17 Oct 2015 14:06:34 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZnVsF-0001TI-DP for qemu-devel@nongnu.org; Sat, 17 Oct 2015 14:06:32 -0400 References: <8dc2b9b2a8c5307eba7fafa2dee8f6e8ef17a26c.1444743418.git.berto@igalia.com> From: Max Reitz Message-ID: <56228E1B.2020307@redhat.com> Date: Sat, 17 Oct 2015 20:06:19 +0200 MIME-Version: 1.0 In-Reply-To: <8dc2b9b2a8c5307eba7fafa2dee8f6e8ef17a26c.1444743418.git.berto@igalia.com> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="soJTOHHSwb2GJA4nxIKF77FKdj3GcuNOL" Subject: Re: [Qemu-devel] [PATCH 2/3] block: Add 'blockdev-del' QMP command List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Alberto Garcia , qemu-devel@nongnu.org Cc: Kevin Wolf , Stefan Hajnoczi , Markus Armbruster , qemu-block@nongnu.org This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --soJTOHHSwb2GJA4nxIKF77FKdj3GcuNOL Content-Type: text/plain; charset=iso-8859-15 Content-Transfer-Encoding: quoted-printable On 13.10.2015 15:48, Alberto Garcia wrote: > This is the companion to 'blockdev-add'. It allows deleting a > BlockBackend with its associated BlockDriverState tree, or a > BlockDriverState that is not attached to any backend. >=20 > In either case, the command fails if the reference count is greater > than 1 or the BlockDriverState has any parents. >=20 > Signed-off-by: Alberto Garcia > --- > blockdev.c | 42 ++++++++++++++++++++++++++++++++++++++++++ > qapi/block-core.json | 21 +++++++++++++++++++++ > qmp-commands.hx | 36 ++++++++++++++++++++++++++++++++++++ > 3 files changed, 99 insertions(+) >=20 > diff --git a/blockdev.c b/blockdev.c > index 2360c1f..c448a0b 100644 > --- a/blockdev.c > +++ b/blockdev.c > @@ -3402,6 +3402,48 @@ fail: > qmp_output_visitor_cleanup(ov); > } > =20 > +void qmp_blockdev_del(const char *device, Error **errp) > +{ > + AioContext *aio_context; > + BlockBackend *blk; > + BlockDriverState *bs; > + > + blk =3D blk_by_name(device); > + if (blk) { > + bs =3D blk_bs(blk); > + aio_context =3D blk_get_aio_context(blk); > + } else { > + bs =3D bdrv_find_node(device); > + if (!bs) { > + error_setg(errp, "Cannot find block device %s", device); > + return; > + } > + blk =3D bs->blk; > + aio_context =3D bdrv_get_aio_context(bs); > + } > + > + aio_context_acquire(aio_context); > + > + if (bs && bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_DRIVE_DEL, errp)) {= > + goto out; > + } > + > + if (blk_get_refcnt(blk) > 1 || > + (bs && (bs->refcnt > 1 || !QLIST_EMPTY(&bs->parents)))) { > + error_setg(errp, "Block device %s is being used", device); > + goto out; > + } I can't think of a way off the top of my head that a BB with a refcount of one or a BDS with a refcount of one without any parents would not be monitor-owned, but I don't quite like assuming it just from the refcount and the parent list anyway. Especially since I can have two series on hold which are pretty strongly tied to this patch: http://lists.nongnu.org/archive/html/qemu-block/2015-03/msg00044.html (block: Rework bdrv_close_all()) and http://lists.nongnu.org/archive/html/qemu-block/2015-02/msg00021.html (blockdev: Further BlockBackend work) Both have been on hold for more than half a year, since they depend on the "BlockBackend and media" series, and, well, I guess you know well about that one's status. The two patches which are significant for this patch are: - "blockdev: Keep track of monitor-owned BDS" from the first series - "blockdev: Add list of monitor-owned BlockBackends" from the second Explaining what they do and why they are relevant to this patch doesn't really seem necessary, but anyway: They add one list for the monitor-owned BDSs and one for the monitor-owned BBs. In combination with this patch, that means two things: (1) We should only *_unref() BDSs/BBs if their refcount is exactly one *and* they are in their respective list, and (2) We have to remove the BDS/BB from its respective list. You don't really have to care about (2), because that's my own problem for having introduced these lists. But (1)... It would definitely be a better check than just comparing the refcount. It's up to you. It probably works just how you implemented it, and considering the pace of the "BB and media" series (and other series of mine in the past year), getting those two series merged may be a matter of decades. So it's probably best to do it like this for now and I'll fix it up then... Max > + > + if (blk) { > + blk_unref(blk); > + } else { > + bdrv_unref(bs); > + } > + > +out: > + aio_context_release(aio_context); > +} > + > BlockJobInfoList *qmp_query_block_jobs(Error **errp) > { > BlockJobInfoList *head =3D NULL, **p_next =3D &head; > diff --git a/qapi/block-core.json b/qapi/block-core.json > index 5f12af7..20897eb 100644 > --- a/qapi/block-core.json > +++ b/qapi/block-core.json > @@ -1877,6 +1877,27 @@ > { 'command': 'blockdev-add', 'data': { 'options': 'BlockdevOptions' } = } > =20 > ## > +# @blockdev-del: > +# > +# Deletes a block device. The selected device can be either a block > +# backend or a graph node. > +# > +# In the former case the backend will be destroyed, along with its > +# inserted medium if there's any. > +# > +# In the latter case the node will be destroyed, along with the > +# backend it is attached to if there's any. > +# > +# The command will fail if the selected block device is still being > +# used. > +# > +# @device: Name of the block backend or the graph node to be deleted. > +# > +# Since: 2.5 > +## > +{ 'command': 'blockdev-del', 'data': { 'device': 'str' } } > + > +## > # @blockdev-open-tray: > # > # Opens a block device's tray. If there is a block driver state tree i= nserted as > diff --git a/qmp-commands.hx b/qmp-commands.hx > index 4f03d11..b8b133e 100644 > --- a/qmp-commands.hx > +++ b/qmp-commands.hx > @@ -3921,6 +3921,42 @@ Example (2): > EQMP > =20 > { > + .name =3D "blockdev-del", > + .args_type =3D "device:s", > + .mhandler.cmd_new =3D qmp_marshal_blockdev_del, > + }, > + > +SQMP > +blockdev-del > +------------ > +Since 2.5 > + > +Deletes a block device. The selected device can be either a block > +backend or a graph node. > + > +In the former case the backend will be destroyed, along with its > +inserted medium if there's any. > + > +In the latter case the node will be destroyed, along with the > +backend it is attached to if there's any. > + > +The command will fail if the selected block device is still being > +used. > + > +Arguments: > + > +- "device": Identifier of the block device or graph node name (json-st= ring) > + > +Example: > + > +-> { "execute": "blockdev-del", > + "arguments": { "device": "virtio0" } > + } > +<- { "return": {} } > + > +EQMP > + > + { > .name =3D "blockdev-open-tray", > .args_type =3D "device:s,force:b?", > .mhandler.cmd_new =3D qmp_marshal_blockdev_open_tray, >=20 --soJTOHHSwb2GJA4nxIKF77FKdj3GcuNOL 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 iQEcBAEBCAAGBQJWIo4bAAoJEDuxQgLoOKytk8EH/RyevIwTiPSjg7m5gLBgiQGX izL8n8czTVXHIWD8q2DPD+9fQW4yFzEdp4U1e8SoOq1uOa8yCfN9BfExAz/7qlTK E+j0MeWQ3CoFRZbwOjfk6Rjom0O9cAXGKQMFe7m8DFiq+aRuX4VETuE1ZERZODIk Y9DQOv5L868SnujfMJ1oHXU2lqi5xLyZst1Oww5iJE3UnWTIVYaE2lWfRzaIOf5e zzVFDV6E9d1DlXR6Z+lK1JpgMrGN0Gw/l47af8qtmkVd2+sZ2g66iHWmqAf6ggZN JRDTkD25VCCqStJPuY8nQ5asyUPNEzpIOo+4WybpOHKM91ASR0y8eAfEgC3ZDGo= =mQIz -----END PGP SIGNATURE----- --soJTOHHSwb2GJA4nxIKF77FKdj3GcuNOL--