Re: [Qemu-devel] [PATCH 04/15] block: Simplify find_block_job() and make it accept a job ID

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 2456 bytes --]

On 06/09/2016 02:20 AM, 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 <berto@igalia.com>
> ---
>  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 */
> +static BlockJob *find_block_job(const char *id, const char *device,
> +                                AioContext **aio_context, Error **errp)

Can this signature just be const char *id_or_device, rather than two
parameters,...

>  {
> -    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 "
> +                   "must be specified when looking for a block job");
> +        return NULL;
>      }

If you keep this check, you could simplify it to:

if (!id == !device) {

...at which point you don't need this check,...

>  
> -    *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 {

...and this would become:

job = block_job_get(id);
if (!job) {


-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]