Re: [Qemu-devel] [PATCH 5/5] blockjob: add Job parameter to BlockCompletionFunc

[Kevin Wolf]

Am 12.01.2016 um 01:36 hat John Snow geschrieben:
> It will no longer be sufficient to rely on the opaque parameter
> containing a BDS, and there's no way to reliably include a
> self-reference to the job we're creating, so always pass the Job
> object forward to any callbacks.
> 
> Signed-off-by: John Snow <address@hidden>
> ---
>  block/backup.c            |  2 +-
>  block/commit.c            |  2 +-
>  block/mirror.c            |  6 +++---
>  block/stream.c            |  2 +-
>  blockdev.c                | 14 +++++++-------
>  blockjob.c                | 13 +++++++++++--
>  include/block/block.h     |  2 ++
>  include/block/block_int.h | 10 +++++-----
>  include/block/blockjob.h  |  6 ++++--
>  qemu-img.c                |  4 ++--
>  tests/test-blockjob-txn.c |  4 ++--
>  11 files changed, 39 insertions(+), 26 deletions(-)
> 
> diff --git a/block/backup.c b/block/backup.c
> index 58c76be..cadb880 100644
> --- a/block/backup.c
> +++ b/block/backup.c
> @@ -506,7 +506,7 @@ BlockJob *backup_start(BlockDriverState *bs, 
> BlockDriverState *target,
>                         BdrvDirtyBitmap *sync_bitmap,
>                         BlockdevOnError on_source_error,
>                         BlockdevOnError on_target_error,
> -                       BlockCompletionFunc *cb, void *opaque,
> +                       BlockJobCompletionFunc *cb, void *opaque,
>                         BlockJobTxn *txn, Error **errp)
>  {
>      int64_t len;
> diff --git a/block/commit.c b/block/commit.c
> index a5d02aa..ef4fd5a 100644
> --- a/block/commit.c
> +++ b/block/commit.c
> @@ -202,7 +202,7 @@ static const BlockJobDriver commit_job_driver = {
>  
>  void commit_start(BlockDriverState *bs, BlockDriverState *base,
>                    BlockDriverState *top, int64_t speed,
> -                  BlockdevOnError on_error, BlockCompletionFunc *cb,
> +                  BlockdevOnError on_error, BlockJobCompletionFunc *cb,
>                    void *opaque, const char *backing_file_str, Error **errp)
>  {
>      CommitBlockJob *s;
> diff --git a/block/mirror.c b/block/mirror.c
> index 92706ab..18134e4 100644
> --- a/block/mirror.c
> +++ b/block/mirror.c
> @@ -714,7 +714,7 @@ static BlockJob *mirror_start_job(BlockDriverState *bs,
>                                    BlockdevOnError on_source_error,
>                                    BlockdevOnError on_target_error,
>                                    bool unmap,
> -                                  BlockCompletionFunc *cb,
> +                                  BlockJobCompletionFunc *cb,
>                                    void *opaque, Error **errp,
>                                    const BlockJobDriver *driver,
>                                    bool is_none_mode, BlockDriverState *base)
> @@ -801,7 +801,7 @@ void mirror_start(BlockDriverState *bs, BlockDriverState 
> *target,
>                    MirrorSyncMode mode, BlockdevOnError on_source_error,
>                    BlockdevOnError on_target_error,
>                    bool unmap,
> -                  BlockCompletionFunc *cb,
> +                  BlockJobCompletionFunc *cb,
>                    void *opaque, Error **errp)
>  {
>      bool is_none_mode;
> @@ -826,7 +826,7 @@ void mirror_start(BlockDriverState *bs, BlockDriverState 
> *target,
>  BlockJob *commit_active_start(BlockDriverState *bs, BlockDriverState *base,
>                                int64_t speed,
>                                BlockdevOnError on_error,
> -                              BlockCompletionFunc *cb,
> +                              BlockJobCompletionFunc *cb,
>                                void *opaque, Error **errp)
>  {
>      int64_t length, base_length;
> diff --git a/block/stream.c b/block/stream.c
> index 1dfeac0..1bd8220 100644
> --- a/block/stream.c
> +++ b/block/stream.c
> @@ -216,7 +216,7 @@ static const BlockJobDriver stream_job_driver = {
>  BlockJob *stream_start(BlockDriverState *bs, BlockDriverState *base,
>                    const char *backing_file_str, int64_t speed,
>                    BlockdevOnError on_error,
> -                  BlockCompletionFunc *cb,
> +                  BlockJobCompletionFunc *cb,
>                    void *opaque, Error **errp)
>  {
>      StreamBlockJob *s;
> diff --git a/blockdev.c b/blockdev.c
> index 9b37ace..6713ecb 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -2855,28 +2855,28 @@ out:
>      aio_context_release(aio_context);
>  }
>  
> -static void block_job_cb(void *opaque, int ret)
> +static void block_job_cb(BlockJob *job, int ret)
>  {
>      /* Note that this function may be executed from another AioContext 
> besides
>       * the QEMU main loop.  If you need to access anything that assumes the
>       * QEMU global mutex, use a BH or introduce a mutex.
>       */
>  
> -    BlockDriverState *bs = opaque;
> +    BlockDriverState *bs = job->bs;
>      const char *msg = NULL;
>  
> -    trace_block_job_cb(bs, bs->job, ret);
> +    trace_block_job_cb(bs, job, ret);

You could directly use job->bs here, then the last user of the local
variable bs is gone.

> -    assert(bs->job);
> +    assert(job);

That's a bit late. If job == NULL, we have already segfaulted. I'd
suggest to just remove the assertion now that the job is directly
passed.

Kevin