Re: [Qemu-devel] [PATCH] blockjob: add block_job_release function

[John Snow]

On 06/26/2015 05:37 AM, Ting Wang wrote:
> There is job resource leak in function mirror_start_job, 
> although bdrv_create_dirty_bitmap is unlikely failed.
> Add block_job_release for each release when needed.
> 
> Signed-off-by: Ting Wang <address@hidden>
> ---
>  block/mirror.c           |  2 ++
>  blockjob.c               | 20 ++++++++++++--------
>  include/block/blockjob.h |  8 ++++++++
>  3 files changed, 22 insertions(+), 8 deletions(-)
> 
> diff --git a/block/mirror.c b/block/mirror.c
> index 048e452..05034a8 100644
> --- a/block/mirror.c
> +++ b/block/mirror.c
> @@ -689,6 +689,8 @@ static void mirror_start_job(BlockDriverState *bs, 
> BlockDriverState *target,
>  
>      s->dirty_bitmap = bdrv_create_dirty_bitmap(bs, granularity, NULL, errp);
>      if (!s->dirty_bitmap) {
> +        g_free(s->replaces);
> +        block_job_release(bs);
>          return;
>      }
>      bdrv_set_enable_write_cache(s->target, true);
> diff --git a/blockjob.c b/blockjob.c
> index ec46fad..62bb906 100644
> --- a/blockjob.c
> +++ b/blockjob.c
> @@ -66,10 +66,7 @@ void *block_job_create(const BlockJobDriver *driver, 
> BlockDriverState *bs,
>  
>          block_job_set_speed(job, speed, &local_err);
>          if (local_err) {
> -            bs->job = NULL;
> -            bdrv_op_unblock_all(bs, job->blocker);
> -            error_free(job->blocker);
> -            g_free(job);
> +            block_job_release(bs);
>              error_propagate(errp, local_err);
>              return NULL;
>          }
> @@ -77,18 +74,25 @@ void *block_job_create(const BlockJobDriver *driver, 
> BlockDriverState *bs,
>      return job;
>  }
>  
> -void block_job_completed(BlockJob *job, int ret)
> +void block_job_release(BlockDriverState *bs)
>  {
> -    BlockDriverState *bs = job->bs;
> +    BlockJob *job = bs->job;
>  
> -    assert(bs->job == job);
> -    job->cb(job->opaque, ret);
>      bs->job = NULL;
>      bdrv_op_unblock_all(bs, job->blocker);
>      error_free(job->blocker);
>      g_free(job);
>  }
>  
> +void block_job_completed(BlockJob *job, int ret)
> +{
> +    BlockDriverState *bs = job->bs;
> +
> +    assert(bs->job == job);
> +    job->cb(job->opaque, ret);
> +    block_job_release(bs);
> +}
> +
>  void block_job_set_speed(BlockJob *job, int64_t speed, Error **errp)
>  {
>      Error *local_err = NULL;
> diff --git a/include/block/blockjob.h b/include/block/blockjob.h
> index 57d8ef1..dd9d5e6 100644
> --- a/include/block/blockjob.h
> +++ b/include/block/blockjob.h
> @@ -166,6 +166,14 @@ void block_job_sleep_ns(BlockJob *job, QEMUClockType 
> type, int64_t ns);
>  void block_job_yield(BlockJob *job);
>  
>  /**
> + * block_job_release:
> + * @bs: The block device.
> + *
> + * Release job resources when an error occurred or job completed.
> + */
> +void block_job_release(BlockDriverState *bs);
> +
> +/**
>   * block_job_completed:
>   * @job: The job being completed.
>   * @ret: The status code.
> 

Reviewed-by: John Snow <address@hidden>