Re: [Qemu-devel] [PATCH 32/42] job: Move completion and cancellation to Job

[Max Reitz]

On 2018-05-09 18:26, Kevin Wolf wrote:
> This moves the top-level job completion and cancellation functions from
> BlockJob to Job.
> 
> Signed-off-by: Kevin Wolf <address@hidden>
> ---
>  include/block/blockjob.h     | 55 -------------------------------
>  include/block/blockjob_int.h | 18 ----------
>  include/qemu/job.h           | 58 ++++++++++++++++++++++++++++----
>  block.c                      |  2 +-
>  block/backup.c               |  3 +-
>  block/commit.c               |  8 ++---
>  block/mirror.c               |  6 ++--
>  block/replication.c          |  4 +--
>  block/stream.c               |  2 +-
>  blockdev.c                   |  8 ++---
>  blockjob.c                   | 76 ------------------------------------------
>  job.c                        | 78 
> +++++++++++++++++++++++++++++++++++++++++---
>  qemu-img.c                   |  2 +-
>  tests/test-bdrv-drain.c      |  5 ++-
>  tests/test-blockjob-txn.c    | 14 ++++----
>  tests/test-blockjob.c        | 21 ++++++------
>  block/trace-events           |  3 --
>  trace-events                 |  1 +
>  18 files changed, 162 insertions(+), 202 deletions(-)

After this patch, there are a couple of places left that mention
block_job_enter(), those should be fixed.  Well, and those two
block_job_completed() mentions.

[...]

> diff --git a/block.c b/block.c
> index 676e57f562..7a149bfea9 100644
> --- a/block.c
> +++ b/block.c
> @@ -3362,7 +3362,7 @@ static void bdrv_close(BlockDriverState *bs)
>  
>  void bdrv_close_all(void)
>  {
> -    block_job_cancel_sync_all();
> +    job_cancel_sync_all();

Do we really want to cancel jobs that might have nothing to do with the
block layer?

>      nbd_export_close_all();
>  
>      /* Drop references from requests still in flight, such as canceled block

[...]

> diff --git a/block/commit.c b/block/commit.c
> index 02a8af9127..56c3810bad 100644
> --- a/block/commit.c
> +++ b/block/commit.c
> @@ -112,12 +112,12 @@ static void commit_complete(Job *job, void *opaque)
>      blk_unref(s->top);
>  
>      /* If there is more than one reference to the job (e.g. if called from
> -     * block_job_finish_sync()), block_job_completed() won't free it and
> -     * therefore the blockers on the intermediate nodes remain. This would
> -     * cause bdrv_set_backing_hd() to fail. */
> +     * block_job_finish_sync()), job_completed() won't free it and therefore

Nice start, but there is more to do in this line. :-)

(Though probably in some other patch.)

Max

> +     * the blockers on the intermediate nodes remain. This would cause
> +     * bdrv_set_backing_hd() to fail. */
>      block_job_remove_all_bdrv(bjob);
>  
> -    block_job_completed(&s->common, ret);
> +    job_completed(job, ret);
>      g_free(data);
>  
>      /* If bdrv_drop_intermediate() didn't already do that, remove the commit

signature.asc
Description: OpenPGP digital signature