From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:41431) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bJLV1-0007J0-Hd for qemu-devel@nongnu.org; Sat, 02 Jul 2016 10:02:24 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bJLUz-0007N4-Gv for qemu-devel@nongnu.org; Sat, 02 Jul 2016 10:02:22 -0400 References: From: Max Reitz Message-ID: <05898d8b-4c7d-f2bc-d128-68566a8d6308@redhat.com> Date: Sat, 2 Jul 2016 16:02:11 +0200 MIME-Version: 1.0 In-Reply-To: Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="4B0xT7GXTm49WsoXSkBf3Eeo7nSfCcU8h" Subject: Re: [Qemu-devel] [PATCH v3 04/11] block: Use block_job_get() in find_block_job() 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 , Jeff Cody , John Snow This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --4B0xT7GXTm49WsoXSkBf3Eeo7nSfCcU8h From: Max Reitz To: Alberto Garcia , qemu-devel@nongnu.org Cc: qemu-block@nongnu.org, Kevin Wolf , Eric Blake , Jeff Cody , John Snow Message-ID: <05898d8b-4c7d-f2bc-d128-68566a8d6308@redhat.com> Subject: Re: [PATCH v3 04/11] block: Use block_job_get() in find_block_job() References: In-Reply-To: Content-Type: text/plain; charset=iso-8859-15 Content-Transfer-Encoding: quoted-printable On 01.07.2016 17:52, Alberto Garcia wrote: > find_block_job() looks for a block backend with a specified name, > checks whether it has a block job and acquires its AioContext. >=20 > We want to identify jobs by their ID and not by the block backend > they're attached to, so this patch ignores the backends altogether and > gets the job directly. Apart from making the code simpler, this will > allow us to find block jobs once they start having user-specified IDs. >=20 > To ensure backward compatibility we keep ERROR_CLASS_DEVICE_NOT_ACTIVE > as the error class if the job doesn't exist. In subsequent patches > we'll also need to keep the device name as the default job ID if the > user doesn't specify a different one. >=20 > Signed-off-by: Alberto Garcia > --- > blockdev.c | 43 ++++++++++++++++--------------------------- > 1 file changed, 16 insertions(+), 27 deletions(-) >=20 > diff --git a/blockdev.c b/blockdev.c > index 3a104a0..8cedb60 100644 > --- a/blockdev.c > +++ b/blockdev.c > @@ -3704,42 +3704,31 @@ void qmp_blockdev_mirror(const char *device, co= nst 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 a block job using its ID and acquire its AioContext */ > +static BlockJob *find_block_job(const char *id, AioContext **aio_conte= xt, > Error **errp) > { > - BlockBackend *blk; > - BlockDriverState *bs; > + BlockJob *job; > =20 > *aio_context =3D NULL; > =20 > - blk =3D blk_by_name(device); > - if (!blk) { > - goto notfound; > + if (!id) { > + error_setg(errp, "Unspecified job ID when looking for a block = job"); > + return NULL; > } Why no plain assertion? Do you expect callers who may pass a NULL ID? > =20 > - *aio_context =3D blk_get_aio_context(blk); > + job =3D block_job_get(id); > + > + if (!job) { > + error_set(errp, ERROR_CLASS_DEVICE_NOT_ACTIVE, > + "Block job '%s' not found", id); This error class seems a bit weird now... I know I advocated for it in v2, but that was because you could actually specifically pass a device name to find block jobs on that device, but now you're just looking for block jobs with a certain ID (that happens to default to the device name)= =2E Anyway, nothing's really wrong, so: Reviewed-by: Max Reitz > + return NULL; > + } > + > + *aio_context =3D blk_get_aio_context(job->blk); > aio_context_acquire(*aio_context); > =20 > - if (!blk_is_available(blk)) { > - goto notfound; > - } > - bs =3D blk_bs(blk); > - > - if (!bs->job) { > - goto notfound; > - } > - > - return bs->job; > - > -notfound: > - error_set(errp, ERROR_CLASS_DEVICE_NOT_ACTIVE, > - "No active block job on device '%s'", device); > - if (*aio_context) { > - aio_context_release(*aio_context); > - *aio_context =3D NULL; > - } > - return NULL; > + return job; > } > =20 > void qmp_block_job_set_speed(const char *device, int64_t speed, Error = **errp) >=20 --4B0xT7GXTm49WsoXSkBf3Eeo7nSfCcU8h 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 iQEvBAEBCAAZBQJXd8ljEhxtcmVpdHpAcmVkaGF0LmNvbQAKCRA7sUIC6DisrSGk B/4mZpK9tkuUZvI3z0WdSHKJoWlmkuMFXzmMbFJXCnuhIsysl3a4d0IOrB1Fo/e4 AevyF3rN7xtO9GpgpVlZNRx4GyxLSZAKz00JdZVuY4EnYtqP1Ka/hw23oj7SZ+6c 0p9lRc1bZyBtyXNC12PdWUZWtbUA4naPXGIZ6TKdRPQpw0ifTasKk9JGl6YgYO2l oM/+2IRYu0+uiQt6ar+EM4YToUH4tScuubqU8zUdKfnX+Wz+Pe5EYhXm0yqzKOR6 djfwhO7Ad6rwRT2kShZ1Alkceo5Tl3MS5Kx5jxKu/32TCKUv8+VNeO48DmhOQ5K7 e4mDSfUvCIiNaUYAAzO0CzbF =3Jyv -----END PGP SIGNATURE----- --4B0xT7GXTm49WsoXSkBf3Eeo7nSfCcU8h--