From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:48374) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aOFnQ-0006Gr-DI for qemu-devel@nongnu.org; Tue, 26 Jan 2016 21:25:25 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aOFnO-0007Pd-Se for qemu-devel@nongnu.org; Tue, 26 Jan 2016 21:25:24 -0500 Date: Wed, 27 Jan 2016 10:25:10 +0800 From: Fam Zheng Message-ID: <20160127022510.GD2877@ad.usersys.redhat.com> References: <1453852499-25800-1-git-send-email-jsnow@redhat.com> <1453852499-25800-6-git-send-email-jsnow@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1453852499-25800-6-git-send-email-jsnow@redhat.com> Subject: Re: [Qemu-devel] [PATCH v2 5/5] blockjob: add Job parameter to BlockCompletionFunc List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: John Snow Cc: kwolf@redhat.com, jcody@redhat.com, armbru@redhat.com, qemu-block@nongnu.org, qemu-devel@nongnu.org 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 > --- > 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