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

[Fam Zheng]

On Tue, 01/26 18:54, John Snow wrote:
> 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, 37 insertions(+), 28 deletions(-)
> 
> diff --git a/block/backup.c b/block/backup.c
> index 735cbe6..dd7e532 100644
> --- a/block/backup.c
> +++ b/block/backup.c
> @@ -492,7 +492,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 446a3ae..aca4b84 100644
> --- a/block/commit.c
> +++ b/block/commit.c
> @@ -203,7 +203,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 b193e36..8016834 100644
> --- a/block/mirror.c
> +++ b/block/mirror.c
> @@ -715,7 +715,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)
> @@ -802,7 +802,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;
> @@ -827,7 +827,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 26a2990..0c8ae20 100644
> --- a/block/stream.c
> +++ b/block/stream.c
> @@ -217,7 +217,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 ad46aa8..9851405 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -2854,28 +2854,24 @@ 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;
>      const char *msg = NULL;
>  
> -    trace_block_job_cb(bs, bs->job, ret);
> -
> -    assert(bs->job);
> +    trace_block_job_cb(job->bs, job, ret);
>  
>      if (ret < 0) {
>          msg = strerror(-ret);
>      }
>  
> -    if (block_job_is_cancelled(bs->job)) {
> -        block_job_event_cancelled(bs->job);
> +    if (block_job_is_cancelled(job)) {
> +        block_job_event_cancelled(job);
>      } else {
> -        block_job_event_completed(bs->job, msg);
> +        block_job_event_completed(job, msg);
>      }
>  }
>  
> diff --git a/blockjob.c b/blockjob.c
> index 80adb9d..fe6873c 100644
> --- a/blockjob.c
> +++ b/blockjob.c
> @@ -51,7 +51,7 @@ struct BlockJobTxn {
>  };
>  
>  void *block_job_create(const BlockJobDriver *driver, BlockDriverState *bs,
> -                       int64_t speed, BlockCompletionFunc *cb,
> +                       int64_t speed, BlockJobCompletionFunc *cb,
>                         void *opaque, Error **errp)
>  {
>      BlockJob *job;
> @@ -90,6 +90,15 @@ void *block_job_create(const BlockJobDriver *driver, 
> BlockDriverState *bs,
>      return job;
>  }
>  
> +/**
> + * For generic callbacks to retrieve the data they submitted
> + * during callback registration.
> + */
> +void *block_job_data(BlockJob *job)
> +{
> +    return job->opaque;
> +}
> +
>  void block_job_ref(BlockJob *job)
>  {
>      ++job->refcnt;
> @@ -118,7 +127,7 @@ static void block_job_completed_single(BlockJob *job)
>              job->driver->abort(job);
>          }
>      }
> -    job->cb(job->opaque, job->ret);
> +    job->cb(job, job->ret);
>      if (job->txn) {
>          block_job_txn_unref(job->txn);
>      }
> diff --git a/include/block/block.h b/include/block/block.h
> index 25f36dc..ed6f2ee 100644
> --- a/include/block/block.h
> +++ b/include/block/block.h
> @@ -16,6 +16,8 @@ typedef struct BdrvChild BdrvChild;
>  typedef struct BdrvChildRole BdrvChildRole;
>  typedef struct BlockJobTxn BlockJobTxn;
>  
> +typedef void BlockJobCompletionFunc(BlockJob *job, int ret);
> +
>  typedef struct BlockDriverInfo {
>      /* in bytes, 0 if irrelevant */
>      int cluster_size;
> diff --git a/include/block/block_int.h b/include/block/block_int.h
> index 9a5cc39..271e922 100644
> --- a/include/block/block_int.h
> +++ b/include/block/block_int.h
> @@ -601,7 +601,7 @@ int is_windows_drive(const char *filename);
>  BlockJob *stream_start(BlockDriverState *bs, BlockDriverState *base,
>                         const char *base_id, int64_t speed,
>                         BlockdevOnError on_error,
> -                       BlockCompletionFunc *cb,
> +                       BlockJobCompletionFunc *cb,
>                         void *opaque, Error **errp);
>  
>  /**
> @@ -619,7 +619,7 @@ BlockJob *stream_start(BlockDriverState *bs, 
> BlockDriverState *base,
>   */
>  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);
>  /**
>   * commit_active_start:
> @@ -635,7 +635,7 @@ void commit_start(BlockDriverState *bs, BlockDriverState 
> *base,
>  BlockJob *commit_active_start(BlockDriverState *bs, BlockDriverState *base,
>                                int64_t speed,
>                                BlockdevOnError on_error,
> -                              BlockCompletionFunc *cb,
> +                              BlockJobCompletionFunc *cb,
>                                void *opaque, Error **errp);
>  /*
>   * mirror_start:
> @@ -665,7 +665,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);
>  
>  /*
> @@ -689,7 +689,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);
>  
>  void blk_set_bs(BlockBackend *blk, BlockDriverState *bs);
> diff --git a/include/block/blockjob.h b/include/block/blockjob.h
> index d84ccd8..b233d27 100644
> --- a/include/block/blockjob.h
> +++ b/include/block/blockjob.h
> @@ -143,7 +143,7 @@ struct BlockJob {
>      int64_t speed;
>  
>      /** The completion function that will be called when the job completes.  
> */
> -    BlockCompletionFunc *cb;
> +    BlockJobCompletionFunc *cb;
>  
>      /** Block other operations when block job is running */
>      Error *blocker;
> @@ -186,9 +186,11 @@ struct BlockJob {
>   * called from a wrapper that is specific to the job type.
>   */
>  void *block_job_create(const BlockJobDriver *driver, BlockDriverState *bs,
> -                       int64_t speed, BlockCompletionFunc *cb,
> +                       int64_t speed, BlockJobCompletionFunc *cb,
>                         void *opaque, Error **errp);
>  
> +void *block_job_data(BlockJob *job);
> +
>  /**
>   * block_job_sleep_ns:
>   * @job: The job that calls the function.
> diff --git a/qemu-img.c b/qemu-img.c
> index 8d11708..cc96cc8 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -635,9 +635,9 @@ typedef struct CommonBlockJobCBInfo {
>      Error **errp;
>  } CommonBlockJobCBInfo;
>  
> -static void common_block_job_cb(void *opaque, int ret)
> +static void common_block_job_cb(BlockJob *job, int ret)
>  {
> -    CommonBlockJobCBInfo *cbi = opaque;
> +    CommonBlockJobCBInfo *cbi = block_job_data(job);
>  
>      if (ret < 0) {
>          error_setg_errno(cbi->errp, -ret, "Block job failed");
> diff --git a/tests/test-blockjob-txn.c b/tests/test-blockjob-txn.c
> index 34747e9..3e8d17d 100644
> --- a/tests/test-blockjob-txn.c
> +++ b/tests/test-blockjob-txn.c
> @@ -66,9 +66,9 @@ typedef struct {
>      int *result;
>  } TestBlockJobCBData;
>  
> -static void test_block_job_cb(void *opaque, int ret)
> +static void test_block_job_cb(BlockJob *job, int ret)
>  {
> -    TestBlockJobCBData *data = opaque;
> +    TestBlockJobCBData *data = block_job_data(job);
>      if (!ret && block_job_is_cancelled(&data->job->common)) {
>          ret = -ECANCELED;
>      }
> -- 
> 2.4.3
> 
> 

Reviewed-by: Fam Zheng <address@hidden>