From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:60964) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1avOcW-0006s6-6S for qemu-devel@nongnu.org; Wed, 27 Apr 2016 08:31:11 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1avOcV-00034U-4B for qemu-devel@nongnu.org; Wed, 27 Apr 2016 08:31:08 -0400 References: From: Max Reitz Message-ID: <5720B0FF.6070605@redhat.com> Date: Wed, 27 Apr 2016 14:30:55 +0200 MIME-Version: 1.0 In-Reply-To: Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="2NEpKnRvmb9XnFgopWlC2FJkGQAugsQPc" Subject: Re: [Qemu-devel] [PATCH v9 05/11] block: allow block jobs in any arbitrary node List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Alberto Garcia , qemu-devel@nongnu.org Cc: qemu-block@nongnu.org, Kevin Wolf , Eric Blake , Stefan Hajnoczi This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --2NEpKnRvmb9XnFgopWlC2FJkGQAugsQPc Content-Type: multipart/mixed; boundary="48mJcNpUaP7H0FLdk9Flj6KVA3fANbr4X" From: Max Reitz To: Alberto Garcia , qemu-devel@nongnu.org Cc: qemu-block@nongnu.org, Kevin Wolf , Eric Blake , Stefan Hajnoczi Message-ID: <5720B0FF.6070605@redhat.com> Subject: Re: [PATCH v9 05/11] block: allow block jobs in any arbitrary node References: In-Reply-To: --48mJcNpUaP7H0FLdk9Flj6KVA3fANbr4X Content-Type: text/plain; charset=iso-8859-15 Content-Transfer-Encoding: quoted-printable On 04.04.2016 15:43, Alberto Garcia wrote: > Currently, block jobs can only be owned by root nodes. This patch > allows block jobs to be in any arbitrary node, by making the following > changes: >=20 > - Block jobs can now be identified by the node name of their > BlockDriverState in addition to the device name. Since both device > and node names live in the same namespace there's no ambiguity. >=20 > - The "device" parameter used by all commands that operate on block > jobs can also be a node name now. >=20 > - The node name is used as a fallback to fill in the BlockJobInfo > structure and all BLOCK_JOB_* events if there is no device name for > that job. >=20 > Signed-off-by: Alberto Garcia > --- > blockdev.c | 18 ++++++++++-------- > blockjob.c | 5 +++-- > docs/qmp-events.txt | 8 ++++---- > qapi/block-core.json | 20 ++++++++++---------- > 4 files changed, 27 insertions(+), 24 deletions(-) >=20 > diff --git a/blockdev.c b/blockdev.c > index edbcc19..d1f5dfb 100644 > --- a/blockdev.c > +++ b/blockdev.c > @@ -3685,8 +3685,10 @@ void qmp_blockdev_mirror(const char *device, con= st char *target, > aio_context_release(aio_context); > } > =20 > -/* Get the block job for a given device name and acquire its AioContex= t */ > -static BlockJob *find_block_job(const char *device, AioContext **aio_c= ontext, > +/* Get the block job for a given device or node name > + * and acquire its AioContext */ > +static BlockJob *find_block_job(const char *device_or_node, > + AioContext **aio_context, > Error **errp) > { > BlockBackend *blk; > @@ -3694,18 +3696,18 @@ static BlockJob *find_block_job(const char *dev= ice, AioContext **aio_context, > =20 > *aio_context =3D NULL; > =20 > - blk =3D blk_by_name(device); > - if (!blk) { > + bs =3D bdrv_lookup_bs(device_or_node, device_or_node, errp); > + if (!bs) { If we get here, *errp is already set... [1] > goto notfound; > } > =20 > - *aio_context =3D blk_get_aio_context(blk); > + *aio_context =3D bdrv_get_aio_context(bs); > aio_context_acquire(*aio_context); > =20 > - if (!blk_is_available(blk)) { > + blk =3D blk_by_name(device_or_node); > + if (blk && !blk_is_available(blk)) { I'd just drop this. The reason blk_is_available() was added here because it became possible for BBs not to have a BDS. Now that you get the BDS directly through bdrv_lookup_bs(), it's no longer necessary. > goto notfound; > } > - bs =3D blk_bs(blk); > =20 > if (!bs->job) { > goto notfound; > @@ -3715,7 +3717,7 @@ static BlockJob *find_block_job(const char *devic= e, AioContext **aio_context, > =20 > notfound: > error_set(errp, ERROR_CLASS_DEVICE_NOT_ACTIVE, > - "No active block job on device '%s'", device); > + "No active block job on node '%s'", device_or_node); [1] ...and then we'll get a failed assertion here (through error_setv()).= > if (*aio_context) { > aio_context_release(*aio_context); > *aio_context =3D NULL; > diff --git a/blockjob.c b/blockjob.c > index 3557048..2ab4794 100644 > --- a/blockjob.c > +++ b/blockjob.c > @@ -67,7 +67,8 @@ void *block_job_create(const BlockJobDriver *driver, = BlockDriverState *bs, > BlockJob *job; > =20 > if (bs->job) { > - error_setg(errp, QERR_DEVICE_IN_USE, bdrv_get_device_name(bs))= ; > + error_setg(errp, "Node '%s' is in use", > + bdrv_get_device_or_node_name(bs)); > return NULL; > } > bdrv_ref(bs); > @@ -78,7 +79,7 @@ void *block_job_create(const BlockJobDriver *driver, = BlockDriverState *bs, > bdrv_op_unblock(bs, BLOCK_OP_TYPE_DATAPLANE, job->blocker); > =20 > job->driver =3D driver; > - job->id =3D g_strdup(bdrv_get_device_name(bs)); > + job->id =3D g_strdup(bdrv_get_device_or_node_name(bs));= Hm, since this is only used for events, it's not too bad that a block job will have a different name if it was created on a root BDS by specifying its node name. But it still is kind of strange. But as I said, it should be fine as long as one can still use the node name for controlling it (which is the case). Max --48mJcNpUaP7H0FLdk9Flj6KVA3fANbr4X-- --2NEpKnRvmb9XnFgopWlC2FJkGQAugsQPc 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 iQEcBAEBCAAGBQJXILD/AAoJEDuxQgLoOKytrOkH/RNnhtejlfuc+gowRHv4+iGD TMxZjbDGPftRe+IupjE8iJX60ssSUwNT/ujzn5J8NhyPkcVdsmSYGBN8rZn8wfeI xehphatTictO6tRrI7jnKkHXLF0vMKBspLOsBpx44TlK5vh7ng4cwPn0Sl0mxD7c uVK3bwxwAKMk5OS/K2hr5NbNRZswg6vdkNfDFGvbVid4RvCNACEB5YYXWL42vU8n iV/Y+f066/98TAgRfCSv7UOryCvWYOAZr4Cb4LBNWNB0cL1C8Cy3jjCvbl9FTz3b YIPT6c5m718SKEC/fXXhnnyKHRFOp8/AqDvdkiLXZgYiV5EcIz3gl3zxxHIvqAI= =gnoo -----END PGP SIGNATURE----- --2NEpKnRvmb9XnFgopWlC2FJkGQAugsQPc--