On 2018-05-09 18:26, Kevin Wolf wrote:
> block_job_drain() contains a blk_drain() call which cannot be moved to
> Job, so add a new JobDriver callback JobDriver.drain which has a common
> implementation for all BlockJobs. In addition to this we keep the
> existing BlockJobDriver.drain callback that is called by the common
> drain implementation for all block jobs.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  include/block/blockjob_int.h | 12 ++++++++++++
>  include/qemu/job.h           | 13 +++++++++++++
>  block/backup.c               |  1 +
>  block/commit.c               |  1 +
>  block/mirror.c               |  2 ++
>  block/stream.c               |  1 +
>  blockjob.c                   | 20 ++++++++++----------
>  job.c                        | 11 +++++++++++
>  tests/test-bdrv-drain.c      |  1 +
>  tests/test-blockjob-txn.c    |  1 +
>  tests/test-blockjob.c        |  2 ++
>  11 files changed, 55 insertions(+), 10 deletions(-)
> 
> diff --git a/include/block/blockjob_int.h b/include/block/blockjob_int.h
> index bf2b762808..38fe22d7e0 100644
> --- a/include/block/blockjob_int.h
> +++ b/include/block/blockjob_int.h
> @@ -65,6 +65,10 @@ struct BlockJobDriver {
>       * If the callback is not NULL, it will be invoked when the job has to be
>       * synchronously cancelled or completed; it should drain BlockDriverStates
>       * as required to ensure progress.
> +     *
> +     * Block jobs must use the default implementation for job_driver.drain,
> +     * which will in turn call this callback after doing generic block job
> +     * stuff.
>       */
>      void (*drain)(BlockJob *job);

I don't really see the point of having two drain callbacks for block
jobs.  Well, it allows an assert() that block_job_drain() is called at
some point, but still.  I'd like block jobs to be not very special, but
this makes them a bit more special than they need to be.

Maybe I'd like it a bit more if there was a macro to automatically set
these mandatory values for block jobs...

But mostly a question of style, so I'll grudgingly give a:

Reviewed-by: Max Reitz <mreitz@redhat.com>

>  };