Re: [Qemu-devel] [PATCH v4 2/6] blockjob: Introduce block_job_complete_sync()

[Fam Zheng]

On Wed, 04/16 23:43, Max Reitz wrote:
> On 16.04.2014 03:48, Fam Zheng wrote:
> >On Sat, 04/12 20:57, Max Reitz wrote:
> >>Implement block_job_complete_sync() by doing the exact same thing as
> >>block_job_cancel_sync() does, only with calling block_job_complete()
> >>instead of block_job_cancel().
> >>
> >>Signed-off-by: Max Reitz <address@hidden>
> >>---
> >>  blockjob.c               | 24 ++++++++++++++++++++++--
> >>  include/block/blockjob.h | 15 +++++++++++++++
> >>  2 files changed, 37 insertions(+), 2 deletions(-)
> >>
> >>diff --git a/blockjob.c b/blockjob.c
> >>index b3ce14c..d12f3ea 100644
> >>--- a/blockjob.c
> >>+++ b/blockjob.c
> >>@@ -165,7 +165,9 @@ static void block_job_cancel_cb(void *opaque, int ret)
> >>      data->cb(data->opaque, ret);
> >>  }
> >>-int block_job_cancel_sync(BlockJob *job)
> >>+static int block_job_finish_sync(BlockJob *job,
> >>+                                 void (*finish)(BlockJob *, Error **errp),
> >>+                                 Error **errp)
> >>  {
> >>      struct BlockCancelData data;
> >>      BlockDriverState *bs = job->bs;
> >>@@ -181,13 +183,31 @@ int block_job_cancel_sync(BlockJob *job)
> >>      data.ret = -EINPROGRESS;
> >>      job->cb = block_job_cancel_cb;
> >>      job->opaque = &data;
> >>-    block_job_cancel(job);
> >>+    finish(job, errp);
> >>      while (data.ret == -EINPROGRESS) {
> >>          qemu_aio_wait();
> >>      }
> >>      return (data.cancelled && data.ret == 0) ? -ECANCELED : data.ret;
> >>  }
> >>+/* A wrapper around block_job_cancel() taking an Error ** parameter so it 
> >>may be
> >>+ * used with block_job_finish_sync() without the need for (rather nasty)
> >>+ * function pointer casts there. */
> >Why not just change the prototype of block_job_cancel to have this error
> >pointer?
> 
> Well, there are external callers of block_job_cancel() which I would have to
> change and I don't really like calling a function with NULL for the Error **
> parameter if it has one; even though this version of block_job_cancel
> wouldn't actually make use of that parameter.
> 
> So, what I'd end up doing is implementing error handling for
> block_job_cancel() in these external callers without any actual need for it,
> which seems to me like more effort than implementing this wrapper.
> Furthermore, I personally like having this wrapper directly above
> block_job_cancel_sync() as it explains nicely why the latter may give NULL
> for the Error ** parameter of block_job_finish_sync().

Thanks for explanation!

I was in doubt about different parameter lists of block_job_complete_sync and
block_job_cancel_sync (only one has Error **). But it is not a big problem.

> 
> I'll change it, though, if you insist. ;-)

Not strongly here, your decision.

> >>+static void block_job_cancel_err(BlockJob *job, Error **errp)
> >>+{
> >>+    block_job_cancel(job);
> >>+}
> >>+
> >>+int block_job_cancel_sync(BlockJob *job)
> >>+{
> >>+    return block_job_finish_sync(job, &block_job_cancel_err, NULL);
> >>+}
> >>+
> >>+int block_job_complete_sync(BlockJob *job, Error **errp)
> >>+{
> >>+    return block_job_finish_sync(job, &block_job_complete, errp);
> >>+}
> >>+
> >>  void block_job_sleep_ns(BlockJob *job, QEMUClockType type, int64_t ns)
> >>  {
> >>      assert(job->busy);
> >>diff --git a/include/block/blockjob.h b/include/block/blockjob.h
> >>index d76de62..626ea42 100644
> >>--- a/include/block/blockjob.h
> >>+++ b/include/block/blockjob.h
> >>@@ -253,6 +253,21 @@ bool block_job_is_paused(BlockJob *job);
> >>  int block_job_cancel_sync(BlockJob *job);
> >>  /**
> >>+ * block_job_complete_sync:
> >>+ * @job: The job to be completed.
> >>+ * @errp: Error object which may be set by block_job_complete(); this is 
> >>not
> >>+ *        necessarily set on every error, the job return value has to be
> >>+ *        checked as well.
> >>+ *
> >>+ * Synchronously complete the job.  The completion callback is called 
> >>before the
> >>+ * function returns, unless it is NULL (which is permissible when using 
> >>this
> >>+ * function).
> >>+ *
> >>+ * Returns the return value from the job.
> >>+ */
> >>+int block_job_complete_sync(BlockJob *job, Error **errp);
> >>+
> >>+/**
> >>   * block_job_iostatus_reset:
> >>   * @job: The job whose I/O status should be reset.
> >>   *
> >>-- 
> >>1.9.2
> >>
>