Re: [Qemu-block] [PATCH 04/14] block: Convert block job core to BlockBackend

[Max Reitz]

On 04.05.2016 11:39, Kevin Wolf wrote:
> This adds a new BlockBackend field to the BlockJob struct, which
> coexists with the BlockDriverState while converting the individual jobs.
> 
> When creating a block job, a new BlockBackend is created on top of the
> given BlockDriverState, and it is destroyed when the BlockJob ends. The
> reference to the BDS is now held by the BlockBackend instead of calling
> bdrv_ref/unref manually.
> 
> We have to be careful when we use bdrv_replace_in_backing_chain() in
> block jobs because this changes the BDS that job->blk points to. At the
> moment block jobs are too tightly coupled with their BDS, so that moving
> a job to another BDS isn't easily possible; therefore, we need to just
> manually undo this change afterwards.
> 
> Signed-off-by: Kevin Wolf <address@hidden>
> ---
>  block/mirror.c           |  3 +++
>  blockjob.c               | 37 ++++++++++++++++++++-----------------
>  include/block/blockjob.h |  3 ++-
>  3 files changed, 25 insertions(+), 18 deletions(-)
> 
> diff --git a/block/mirror.c b/block/mirror.c
> index b9986d8..efca8fc 100644
> --- a/block/mirror.c
> +++ b/block/mirror.c
> @@ -478,6 +478,9 @@ static void mirror_exit(BlockJob *job, void *opaque)
>              bdrv_reopen(s->target, bdrv_get_flags(to_replace), NULL);
>          }
>          bdrv_replace_in_backing_chain(to_replace, s->target);
> +        /* We just changed the BDS the job BB refers to */
> +        blk_remove_bs(job->blk);
> +        blk_insert_bs(job->blk, src);
>      }
>  
>  out:
> diff --git a/blockjob.c b/blockjob.c
> index e916b41..01b58c7 100644
> --- a/blockjob.c
> +++ b/blockjob.c
> @@ -64,13 +64,17 @@ void *block_job_create(const BlockJobDriver *driver, 
> BlockDriverState *bs,
>                         int64_t speed, BlockCompletionFunc *cb,
>                         void *opaque, Error **errp)
>  {
> +    BlockBackend *blk;
>      BlockJob *job;
>  
>      if (bs->job) {
>          error_setg(errp, QERR_DEVICE_IN_USE, bdrv_get_device_name(bs));
>          return NULL;
>      }
> -    bdrv_ref(bs);
> +
> +    blk = blk_new(&error_abort);

You're just using &error_abort here because you know that blk_new()
cannot set errp right now. I don't think that's good.

Whenever a function takes an errp parameter, you should pass a valid
Error object to it and handle any potential errors. This is extremely
simple in this case: Just make it blk_new(errp) and return NULL if blk
is NULL.

&error_abort and such should only be used in places where it is
impossible to handle an error, but we can prove that no error can occur.
While we can prove this here, it is trivial to handle an error, so we
should do it.

Alternatively, blk_new()'s errp parameter can be dropped. I kept it
because of my old "But what if we want to reintroduce it later" habit. I
guess I should make a poster of your "Write code for now instead of the
future" mail and get it on my wall...

(See, my intention would break down here. If we reintroduced a way for
blk_new() to fail, this code here would break.)

The rest looks good.

Max

> +    blk_insert_bs(blk, bs);
> +
>      job = g_malloc0(driver->instance_size);
>      error_setg(&job->blocker, "block device is in use by block job: %s",
>                 BlockJobType_lookup[driver->job_type]);

signature.asc
Description: OpenPGP digital signature