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 <kw...@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 <mre...@redhat.com> > };
signature.asc
Description: OpenPGP digital signature
