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. > > 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. > > 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. > > Signed-off-by: Alberto Garcia <[email protected]> > --- > blockdev.c | 43 ++++++++++++++++--------------------------- > 1 file changed, 16 insertions(+), 27 deletions(-) > > 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, const > char *target, > aio_context_release(aio_context); > } > > -/* Get the block job for a given device name and acquire its AioContext */ > -static BlockJob *find_block_job(const char *device, AioContext **aio_context, > +/* Get a block job using its ID and acquire its AioContext */ > +static BlockJob *find_block_job(const char *id, AioContext **aio_context, > Error **errp) > { > - BlockBackend *blk; > - BlockDriverState *bs; > + BlockJob *job; > > *aio_context = NULL; > > - blk = 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?
>
> - *aio_context = blk_get_aio_context(blk);
> + job = 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).
Anyway, nothing's really wrong, so:
Reviewed-by: Max Reitz <[email protected]>
> + return NULL;
> + }
> +
> + *aio_context = blk_get_aio_context(job->blk);
> aio_context_acquire(*aio_context);
>
> - if (!blk_is_available(blk)) {
> - goto notfound;
> - }
> - bs = 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 = NULL;
> - }
> - return NULL;
> + return job;
> }
>
> void qmp_block_job_set_speed(const char *device, int64_t speed, Error **errp)
>
signature.asc
Description: OpenPGP digital signature
