Re: [Qemu-block] [PATCH v3 04/11] block: Use block_job_get() in find_block_job()

[Max Reitz]

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 <address@hidden>
> ---
>  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 <address@hidden>

> +        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