[Qemu-devel] [PATCH v2 1/5] block: Allow mirror_start to return job references

[John Snow <jsnow@redhat.com>]

Pick up an extra reference in mirror_start_job to allow callers
of mirror_start and commit_active_start to get a reference to
the job they have created. Phase out callers from fishing the job
out of bs->job -- use the return value instead.

Callers of mirror_start_job and commit_active_start are now
responsible for putting down their reference to the job.

No callers of mirror_start yet seem to need the reference, so
that's left as a void return for now.

Ultimately, this patch fixes qemu-img's reliance on bs->job.

Reviewed-by: Kevin Wolf <kwolf@redhat.com>
Signed-off-by: John Snow <jsnow@redhat.com>
---
 block/mirror.c            | 72 ++++++++++++++++++++++++++---------------------
 blockdev.c                |  8 ++++--
 include/block/block_int.h | 10 +++----
 qemu-img.c                | 12 +++++---
 4 files changed, 59 insertions(+), 43 deletions(-)

diff --git a/block/mirror.c b/block/mirror.c
index e9e151c..b193e36 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -707,17 +707,18 @@ static const BlockJobDriver commit_active_job_driver = {
     .complete      = mirror_complete,
 };
 
-static void mirror_start_job(BlockDriverState *bs, BlockDriverState *target,
-                             const char *replaces,
-                             int64_t speed, uint32_t granularity,
-                             int64_t buf_size,
-                             BlockdevOnError on_source_error,
-                             BlockdevOnError on_target_error,
-                             bool unmap,
-                             BlockCompletionFunc *cb,
-                             void *opaque, Error **errp,
-                             const BlockJobDriver *driver,
-                             bool is_none_mode, BlockDriverState *base)
+static BlockJob *mirror_start_job(BlockDriverState *bs,
+                                  BlockDriverState *target,
+                                  const char *replaces,
+                                  int64_t speed, uint32_t granularity,
+                                  int64_t buf_size,
+                                  BlockdevOnError on_source_error,
+                                  BlockdevOnError on_target_error,
+                                  bool unmap,
+                                  BlockCompletionFunc *cb,
+                                  void *opaque, Error **errp,
+                                  const BlockJobDriver *driver,
+                                  bool is_none_mode, BlockDriverState *base)
 {
     MirrorBlockJob *s;
     BlockDriverState *replaced_bs;
@@ -732,12 +733,12 @@ static void mirror_start_job(BlockDriverState *bs, BlockDriverState *target,
          on_source_error == BLOCKDEV_ON_ERROR_ENOSPC) &&
         (!bs->blk || !blk_iostatus_is_enabled(bs->blk))) {
         error_setg(errp, QERR_INVALID_PARAMETER, "on-source-error");
-        return;
+        return NULL;
     }
 
     if (buf_size < 0) {
         error_setg(errp, "Invalid parameter 'buf-size'");
-        return;
+        return NULL;
     }
 
     if (buf_size == 0) {
@@ -749,19 +750,19 @@ static void mirror_start_job(BlockDriverState *bs, BlockDriverState *target,
     if (replaces) {
         replaced_bs = bdrv_lookup_bs(replaces, replaces, errp);
         if (replaced_bs == NULL) {
-            return;
+            return NULL;
         }
     } else {
         replaced_bs = bs;
     }
     if (replaced_bs->blk && target->blk) {
         error_setg(errp, "Can't create node with two BlockBackends");
-        return;
+        return NULL;
     }
 
     s = block_job_create(driver, bs, speed, cb, opaque, errp);
     if (!s) {
-        return;
+        return NULL;
     }
 
     s->replaces = g_strdup(replaces);
@@ -778,7 +779,7 @@ static void mirror_start_job(BlockDriverState *bs, BlockDriverState *target,
     if (!s->dirty_bitmap) {
         g_free(s->replaces);
         block_job_unref(&s->common);
-        return;
+        return NULL;
     }
 
     bdrv_op_block_all(s->target, s->common.blocker);
@@ -788,9 +789,11 @@ static void mirror_start_job(BlockDriverState *bs, BlockDriverState *target,
         blk_set_on_error(s->target->blk, on_target_error, on_target_error);
         blk_iostatus_enable(s->target->blk);
     }
+    block_job_ref(&s->common);
     s->common.co = qemu_coroutine_create(mirror_run);
     trace_mirror_start(bs, s, s->common.co, opaque);
     qemu_coroutine_enter(s->common.co, s);
+    return &s->common;
 }
 
 void mirror_start(BlockDriverState *bs, BlockDriverState *target,
@@ -804,6 +807,7 @@ void mirror_start(BlockDriverState *bs, BlockDriverState *target,
 {
     bool is_none_mode;
     BlockDriverState *base;
+    BlockJob *job;
 
     if (mode == MIRROR_SYNC_MODE_INCREMENTAL) {
         error_setg(errp, "Sync mode 'incremental' not supported");
@@ -811,27 +815,31 @@ void mirror_start(BlockDriverState *bs, BlockDriverState *target,
     }
     is_none_mode = mode == MIRROR_SYNC_MODE_NONE;
     base = mode == MIRROR_SYNC_MODE_TOP ? backing_bs(bs) : NULL;
-    mirror_start_job(bs, target, replaces,
-                     speed, granularity, buf_size,
-                     on_source_error, on_target_error, unmap, cb, opaque, errp,
-                     &mirror_job_driver, is_none_mode, base);
+    job = mirror_start_job(bs, target, replaces,
+                           speed, granularity, buf_size,
+                           on_source_error, on_target_error, unmap, cb, opaque,
+                           errp, &mirror_job_driver, is_none_mode, base);
+    if (job) {
+        block_job_unref(job);
+    }
 }
 
-void commit_active_start(BlockDriverState *bs, BlockDriverState *base,
-                         int64_t speed,
-                         BlockdevOnError on_error,
-                         BlockCompletionFunc *cb,
-                         void *opaque, Error **errp)
+BlockJob *commit_active_start(BlockDriverState *bs, BlockDriverState *base,
+                              int64_t speed,
+                              BlockdevOnError on_error,
+                              BlockCompletionFunc *cb,
+                              void *opaque, Error **errp)
 {
     int64_t length, base_length;
     int orig_base_flags;
     int ret;
     Error *local_err = NULL;
+    BlockJob *job;
 
     orig_base_flags = bdrv_get_flags(base);
 
     if (bdrv_reopen(base, bs->open_flags, errp)) {
-        return;
+        return NULL;
     }
 
     length = bdrv_getlength(bs);
@@ -860,19 +868,19 @@ void commit_active_start(BlockDriverState *bs, BlockDriverState *base,
     }
 
     bdrv_ref(base);
-    mirror_start_job(bs, base, NULL, speed, 0, 0,
-                     on_error, on_error, false, cb, opaque, &local_err,
-                     &commit_active_job_driver, false, base);
+    job = mirror_start_job(bs, base, NULL, speed, 0, 0,
+                           on_error, on_error, false, cb, opaque, &local_err,
+                           &commit_active_job_driver, false, base);
     if (local_err) {
         error_propagate(errp, local_err);
         goto error_restore_flags;
     }
 
-    return;
+    return job;
 
 error_restore_flags:
     /* ignore error and errp for bdrv_reopen, because we want to propagate
      * the original error */
     bdrv_reopen(base, orig_base_flags, NULL);
-    return;
+    return NULL;
 }
diff --git a/blockdev.c b/blockdev.c
index 07cfe25..c0b9b12 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -2955,6 +2955,7 @@ void qmp_block_commit(const char *device,
     BlockBackend *blk;
     BlockDriverState *bs;
     BlockDriverState *base_bs, *top_bs;
+    BlockJob *job = NULL;
     AioContext *aio_context;
     Error *local_err = NULL;
     /* This will be part of the QMP command, if/when the
@@ -3036,8 +3037,8 @@ void qmp_block_commit(const char *device,
                              " but 'top' is the active layer");
             goto out;
         }
-        commit_active_start(bs, base_bs, speed, on_error, block_job_cb,
-                            bs, &local_err);
+        job = commit_active_start(bs, base_bs, speed, on_error, block_job_cb,
+                                  bs, &local_err);
     } else {
         commit_start(bs, base_bs, top_bs, speed, on_error, block_job_cb, bs,
                      has_backing_file ? backing_file : NULL, &local_err);
@@ -3048,6 +3049,9 @@ void qmp_block_commit(const char *device,
     }
 
 out:
+    if (job) {
+        block_job_unref(job);
+    }
     aio_context_release(aio_context);
 }
 
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 428fa33..de4c3c6 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -631,11 +631,11 @@ void commit_start(BlockDriverState *bs, BlockDriverState *base,
  * @errp: Error object.
  *
  */
-void commit_active_start(BlockDriverState *bs, BlockDriverState *base,
-                         int64_t speed,
-                         BlockdevOnError on_error,
-                         BlockCompletionFunc *cb,
-                         void *opaque, Error **errp);
+BlockJob *commit_active_start(BlockDriverState *bs, BlockDriverState *base,
+                              int64_t speed,
+                              BlockdevOnError on_error,
+                              BlockCompletionFunc *cb,
+                              void *opaque, Error **errp);
 /*
  * mirror_start:
  * @bs: Block device to operate on.
diff --git a/qemu-img.c b/qemu-img.c
index 33e451c..8d11708 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -644,8 +644,9 @@ static void common_block_job_cb(void *opaque, int ret)
     }
 }
 
-static void run_block_job(BlockJob *job, Error **errp)
+static void run_block_job(BlockJob **pjob, Error **errp)
 {
+    BlockJob *job = *pjob;
     AioContext *aio_context = bdrv_get_aio_context(job->bs);
 
     do {
@@ -659,6 +660,8 @@ static void run_block_job(BlockJob *job, Error **errp)
     /* A block job may finish instantaneously without publishing any progress,
      * so just signal completion here */
     qemu_progress_print(100.f, 0);
+    block_job_unref(job);
+    *pjob = NULL;
 }
 
 static int img_commit(int argc, char **argv)
@@ -667,6 +670,7 @@ static int img_commit(int argc, char **argv)
     const char *filename, *fmt, *cache, *base;
     BlockBackend *blk;
     BlockDriverState *bs, *base_bs;
+    BlockJob *job;
     bool progress = false, quiet = false, drop = false;
     Error *local_err = NULL;
     CommonBlockJobCBInfo cbi;
@@ -755,8 +759,8 @@ static int img_commit(int argc, char **argv)
         .bs   = bs,
     };
 
-    commit_active_start(bs, base_bs, 0, BLOCKDEV_ON_ERROR_REPORT,
-                        common_block_job_cb, &cbi, &local_err);
+    job = commit_active_start(bs, base_bs, 0, BLOCKDEV_ON_ERROR_REPORT,
+                              common_block_job_cb, &cbi, &local_err);
     if (local_err) {
         goto done;
     }
@@ -769,7 +773,7 @@ static int img_commit(int argc, char **argv)
         bdrv_ref(bs);
     }
 
-    run_block_job(bs->job, &local_err);
+    run_block_job(&job, &local_err);
     if (local_err) {
         goto unref_backing;
     }
-- 
2.4.3