From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:54880) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bK3qR-0008H7-9w for qemu-devel@nongnu.org; Mon, 04 Jul 2016 09:23:28 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bK3qP-0008Ft-84 for qemu-devel@nongnu.org; Mon, 04 Jul 2016 09:23:26 -0400 Date: Mon, 4 Jul 2016 15:23:14 +0200 From: Kevin Wolf Message-ID: <20160704132314.GF5399@noname.redhat.com> References: <05898d8b-4c7d-f2bc-d128-68566a8d6308@redhat.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="8nsIa27JVQLqB7/C" Content-Disposition: inline In-Reply-To: <05898d8b-4c7d-f2bc-d128-68566a8d6308@redhat.com> 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: Max Reitz Cc: Alberto Garcia , qemu-devel@nongnu.org, qemu-block@nongnu.org, Eric Blake , Jeff Cody , John Snow --8nsIa27JVQLqB7/C Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable Am 02.07.2016 um 16:02 hat Max Reitz geschrieben: > 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; > > } >=20 > Why no plain assertion? Do you expect callers who may pass a NULL ID? >=20 > > =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); >=20 > 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). libvirt uses the error class, so I don't think we can drop it. Kevin --8nsIa27JVQLqB7/C Content-Type: application/pgp-signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v2.0.22 (GNU/Linux) iQIcBAEBAgAGBQJXemNCAAoJEH8JsnLIjy/WdxwQALHAdlvu/IyxcZgXbMoU4rbl mFjgcmN/utWRTSd0dE1u77qHvub1R5lA38fQTaj6rl+KPZVkDPTDInY1/1cxl0c0 6FJMo4VnZEItZSgodXtSZMetEjCPZ2ggXZUxceKYLLc+Ffv/OYyn2cXXLhecDlDX NfbfxetBI6fI24AuPR0uXEOkVKxGl1OSGaEvbSIYHJ72VTjdAMEfqzIfXmEsWaWe be+bYcVOp8PKivnlIGvgKosdDvhDiWF4+MT66Ikx0QA764Xht5xFqvn5YWu98JGA mlwUM9mbF4ifu+W8jBfQ+5kXosu8Cn+xA0bP0mqRqJxZ+XrtiU7O3KWww+XSBszk aEwJz3mbLGYEjvD45qi6tcs7XkA61vHaFLxi1PXCFp4ViJMDTYwO4ubZis25CEch w1TLQFaLFGE4ufIZAU2mPDYNt3UXlN13pqq53YsQuCHbPhTNaBX4H8XTivMfF3ww vjT7qx2k9UuZGZL2w4wp8BCRe9D1pQea25koZzBQs31qOxBsD9YJaPUwfMoA4iIc qZy16+Zq6vFbd/0PWxdedlPFOlNA0P5SOvgAfS7RmU8dbM/aWS7V09ROVoqpyO41 CcmGUOvGn6Ck6QDCmveR6L711fE5ekbUZ4/yy0wAsIIuffUywlJs5KseCBC9nyaj tH/gL7iWfDTw7T8BDzF4 =J/xO -----END PGP SIGNATURE----- --8nsIa27JVQLqB7/C--