qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH 04/15] block: Simplify find_block_job() and make


From: Max Reitz
Subject: Re: [Qemu-devel] [PATCH 04/15] block: Simplify find_block_job() and make it accept a job ID
Date: Mon, 20 Jun 2016 18:40:31 +0200
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.1.1

On 09.06.2016 10:20, 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. This
> patch uses block_job_next() and iterate directly over the block jobs.
> 
> In addition to that we want to identify jobs primarily by their ID, so
> this patch updates find_block_job() to allow IDs too. Only one of ID
> and device name can be specified when looking for a block job.
> 
> Signed-off-by: Alberto Garcia <address@hidden>
> ---
>  blockdev.c | 66 
> +++++++++++++++++++++++++++++++++-----------------------------
>  1 file changed, 35 insertions(+), 31 deletions(-)
> 
> diff --git a/blockdev.c b/blockdev.c
> index 52ec4ae..bd0d5a1 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -3689,48 +3689,52 @@ 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,
> -                                Error **errp)
> +/* Get a block job using its ID or device name and acquire its AioContext */

I think it would be good if this comment could mention that only either
of ID and device name may be specified. Not strictly necessary, though,
since that check is done right at the start of the function.

> +static BlockJob *find_block_job(const char *id, const char *device,
> +                                AioContext **aio_context, Error **errp)
>  {
> -    BlockBackend *blk;
> -    BlockDriverState *bs;
> +    BlockJob *job = NULL;
>  
>      *aio_context = NULL;
>  
> -    blk = blk_by_name(device);
> -    if (!blk) {
> -        goto notfound;
> +    if ((id && device) || (!id && !device)) {
> +        error_setg(errp, "Only one of ID or device name "

Maybe s/Only/Exactly/. Or it could just be an assertion.

> +                   "must be specified when looking for a block job");
> +        return NULL;
>      }
>  
> -    *aio_context = blk_get_aio_context(blk);
> -    aio_context_acquire(*aio_context);
> -
> -    if (!blk_is_available(blk)) {
> -        goto notfound;
> +    if (id) {
> +        job = block_job_get(id);
> +    } else {
> +        BlockJob *j;
> +        for (j = block_job_next(NULL); j; j = block_job_next(j)) {
> +            if (!strcmp(device, j->device)) {
> +                if (job) {
> +                    /* This scenario is currently not possible, but it
> +                     * could theoretically happen in the future. */
> +                    error_setg(errp, "More than one job on device '%s', "
> +                               "use the job ID instead", device);
> +                    return NULL;
> +                }
> +                job = j;
> +            }
> +        }
>      }
> -    bs = blk_bs(blk);
>  
> -    if (!bs->job) {
> -        goto notfound;
> +    if (job) {
> +        *aio_context = blk_get_aio_context(job->blk);
> +        aio_context_acquire(*aio_context);
> +    } else {
> +        error_setg(errp, "Block job '%s' not found", id ? id : device);

This changes the error class if device is set. Not sure if that is bad,
but keeping the old behavior should be simple (unless you're sure it's
fine).

Also, but this is a personal opinion, I'd make the error path more
explicit, i.e.:

if (!job) {
    /* ... */
    return NULL;
}

*aio_context = /* ... */
/* ...*/

>      }
>  
> -    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;
>  }
>  

What's keeping me from giving an R-b is that I don't know whether
changing the error class will be fine. If you say it is, OK then. But
just keeping the old behavior should be simple enough, too.

Max

Attachment: signature.asc
Description: OpenPGP digital signature


reply via email to

[Prev in Thread] Current Thread [Next in Thread]