[Qemu-devel] [PATCH 00/15] Add an 'id' field to block jobs

[Qemu-devel] [PATCH 00/15] Add an 'id' field to block jobs

[Alberto Garcia]

Hello all,

Block jobs are currently identified by the name of the block backend
of the BDS where the job was started.

This series adds a new 'id' field that is specific to the block job
and guaranteed to be unique and always present.

The patches can be summarized as follows:

- 1: Fix the prototype of stream_start(), that is going to be touched
     by a later patch in the series.
- 2-5: Split BlockJob's 'id' field into 'id' and 'device' and adjust
       the internal APIs to allow using the job ID.
- 6-9: Add parameters to allow setting the job ID to all QMP commands
       that create block jobs (block-stream, block-commit, ...)
- 10-14: Add parameters to allow using the job ID to all QMP command
         that manipulate block jobs (cancel, pause, complete, ...).
- 15: Expose the ID field in BlockJobInfo and all BLOCK_JOB_* events.

Things to do:

- Add new tests specific to job IDs.
- Add job ID support to HMP commands.

The series applies on top of Max's block branch (commit 3cc2f35a59)
but it should work on master as well.

Questions, comments, etc, are welcome.

Thanks!

Berto

Alberto Garcia (15):
  stream: Fix prototype of stream_start()
  blockjob: Decouple the ID from the device name in the BlockJob struct
  blockjob: Add block_job_get()
  block: Simplify find_block_job() and make it accept a job ID
  blockjob: Add 'job_id' parameter to block_job_create()
  mirror: Add 'job-id' parameter to 'blockdev-mirror' and 'drive-mirror'
  backup: Add 'job-id' parameter to 'blockdev-backup' and 'drive-backup'
  stream: Add 'job-id' parameter to 'block-stream'
  stream: Add 'job-id' parameter to 'block-commit'
  blockjob: Add 'id' parameter to 'block-job-set-speed'
  blockjob: Add 'id' parameter to 'block-job-cancel'
  blockjob: Add 'id' parameter to 'block-job-pause'
  blockjob: Add 'id' parameter to 'block-job-resume'
  blockjob: Add 'id' parameter to 'block-job-complete'
  blockjob: Add 'id' field to 'BlockJobInfo' and all BLOCK_JOB_* events

 block/backup.c             |   9 +--
 block/commit.c             |   7 +-
 block/mirror.c             |  18 ++---
 block/stream.c             |  12 ++--
 blockdev.c                 | 164 ++++++++++++++++++++++++++-------------------
 blockjob.c                 |  43 ++++++++++--
 docs/qmp-events.txt        |   4 ++
 hmp.c                      |  16 ++---
 include/block/block_int.h  |  47 ++++++++-----
 include/block/blockjob.h   |  30 +++++++--
 include/qemu/id.h          |   1 +
 qapi/block-core.json       |  91 +++++++++++++++++++------
 qemu-img.c                 |   2 +-
 qmp-commands.hx            |  32 +++++----
 tests/qemu-iotests/095     |   2 +-
 tests/qemu-iotests/095.out |   2 +-
 tests/qemu-iotests/124     |   3 +-
 tests/qemu-iotests/141     |   6 +-
 tests/qemu-iotests/141.out |  14 ++--
 tests/qemu-iotests/144     |   1 +
 tests/qemu-iotests/144.out |   4 +-
 tests/test-blockjob-txn.c  |   2 +-
 util/id.c                  |   1 +
 23 files changed, 334 insertions(+), 177 deletions(-)

-- 
2.8.1

[Qemu-devel] [PATCH 01/15] stream: Fix prototype of stream_start()

[Alberto Garcia]

'stream-start' has a parameter called 'backing-file', which is the
string to be written to bs->backing when the job finishes.

In the stream_start() implementation it is called 'backing_file_str',
but it the prototype in the header file it is called 'base_id'.

This patch fixes it so the name is the same in both cases and is
consistent with other cases (like commit_start()).

Signed-off-by: Alberto Garcia <berto@igalia.com>
---
 include/block/block_int.h | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/include/block/block_int.h b/include/block/block_int.h
index 8a4963c..3d95ab3 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -610,8 +610,8 @@ int is_windows_drive(const char *filename);
  * @bs: Block device to operate on.
  * @base: Block device that will become the new base, or %NULL to
  * flatten the whole backing file chain onto @bs.
- * @base_id: The file name that will be written to @bs as the new
- * backing file if the job completes.  Ignored if @base is %NULL.
+ * @backing_file_str: The file name that will be written to @bs as the
+ * the new backing file if the job completes. Ignored if @base is %NULL.
  * @speed: The maximum speed, in bytes per second, or 0 for unlimited.
  * @on_error: The action to take upon error.
  * @cb: Completion function for the job.
@@ -622,11 +622,12 @@ int is_windows_drive(const char *filename);
  * in @bs, but allocated in any image between @base and @bs (both
  * exclusive) will be written to @bs.  At the end of a successful
  * streaming job, the backing file of @bs will be changed to
- * @base_id in the written image and to @base in the live BlockDriverState.
+ * @backing_file_str in the written image and to @base in the live
+ * BlockDriverState.
  */
 void stream_start(BlockDriverState *bs, BlockDriverState *base,
-                  const char *base_id, int64_t speed, BlockdevOnError on_error,
-                  BlockCompletionFunc *cb,
+                  const char *backing_file_str, int64_t speed,
+                  BlockdevOnError on_error, BlockCompletionFunc *cb,
                   void *opaque, Error **errp);
 
 /**
-- 
2.8.1

[Qemu-devel] [PATCH 02/15] blockjob: Decouple the ID from the device name in the BlockJob struct

[Alberto Garcia]

Block jobs are identified by the name of the BlockBackend of the BDS
where the job was started.

We want block jobs to have unique, arbitrary identifiers that are not
tied to a block device, so this patch decouples the ID from the device
name in the BlockJob structure.

The ID is generated automatically for the moment, in later patches
we'll allow the user to set it.

Signed-off-by: Alberto Garcia <berto@igalia.com>
---
 blockjob.c               | 15 +++++++++------
 include/block/blockjob.h | 12 ++++++++----
 include/qemu/id.h        |  1 +
 util/id.c                |  1 +
 4 files changed, 19 insertions(+), 10 deletions(-)

diff --git a/blockjob.c b/blockjob.c
index 01b896b..4d42987 100644
--- a/blockjob.c
+++ b/blockjob.c
@@ -33,6 +33,7 @@
 #include "qapi/qmp/qerror.h"
 #include "qapi/qmp/qjson.h"
 #include "qemu/coroutine.h"
+#include "qemu/id.h"
 #include "qmp-commands.h"
 #include "qemu/timer.h"
 #include "qapi-event.h"
@@ -82,7 +83,8 @@ void *block_job_create(const BlockJobDriver *driver, BlockDriverState *bs,
     bdrv_op_unblock(bs, BLOCK_OP_TYPE_DATAPLANE, job->blocker);
 
     job->driver        = driver;
-    job->id            = g_strdup(bdrv_get_device_name(bs));
+    job->device        = g_strdup(bdrv_get_device_name(bs));
+    job->id            = id_generate(ID_JOB);
     job->blk           = blk;
     job->cb            = cb;
     job->opaque        = opaque;
@@ -119,6 +121,7 @@ void block_job_unref(BlockJob *job)
         bdrv_op_unblock_all(bs, job->blocker);
         blk_unref(job->blk);
         error_free(job->blocker);
+        g_free(job->device);
         g_free(job->id);
         QLIST_REMOVE(job, job_list);
         g_free(job);
@@ -389,7 +392,7 @@ BlockJobInfo *block_job_query(BlockJob *job)
 {
     BlockJobInfo *info = g_new0(BlockJobInfo, 1);
     info->type      = g_strdup(BlockJobType_lookup[job->driver->job_type]);
-    info->device    = g_strdup(job->id);
+    info->device    = g_strdup(job->device);
     info->len       = job->len;
     info->busy      = job->busy;
     info->paused    = job->pause_count > 0;
@@ -411,7 +414,7 @@ static void block_job_iostatus_set_err(BlockJob *job, int error)
 void block_job_event_cancelled(BlockJob *job)
 {
     qapi_event_send_block_job_cancelled(job->driver->job_type,
-                                        job->id,
+                                        job->device,
                                         job->len,
                                         job->offset,
                                         job->speed,
@@ -421,7 +424,7 @@ void block_job_event_cancelled(BlockJob *job)
 void block_job_event_completed(BlockJob *job, const char *msg)
 {
     qapi_event_send_block_job_completed(job->driver->job_type,
-                                        job->id,
+                                        job->device,
                                         job->len,
                                         job->offset,
                                         job->speed,
@@ -435,7 +438,7 @@ void block_job_event_ready(BlockJob *job)
     job->ready = true;
 
     qapi_event_send_block_job_ready(job->driver->job_type,
-                                    job->id,
+                                    job->device,
                                     job->len,
                                     job->offset,
                                     job->speed, &error_abort);
@@ -463,7 +466,7 @@ BlockErrorAction block_job_error_action(BlockJob *job, BlockdevOnError on_err,
     default:
         abort();
     }
-    qapi_event_send_block_job_error(job->id,
+    qapi_event_send_block_job_error(job->device,
                                     is_read ? IO_OPERATION_TYPE_READ :
                                     IO_OPERATION_TYPE_WRITE,
                                     action, &error_abort);
diff --git a/include/block/blockjob.h b/include/block/blockjob.h
index 86d2807..1533006 100644
--- a/include/block/blockjob.h
+++ b/include/block/blockjob.h
@@ -85,14 +85,18 @@ struct BlockJob {
     BlockBackend *blk;
 
     /**
-     * The ID of the block job. Currently the BlockBackend name of the BDS
-     * owning the job at the time when the job is started.
-     *
-     * TODO Decouple block job IDs from BlockBackend names
+     * The ID of the block job.
      */
     char *id;
 
     /**
+     * BlockBackend name of the BDS owning the job at the time when
+     * the job is started. For compatibility with clients that don't
+     * support the ID field.
+     */
+    char *device;
+
+    /**
      * The coroutine that executes the job.  If not NULL, it is
      * reentered when busy is false and the job is cancelled.
      */
diff --git a/include/qemu/id.h b/include/qemu/id.h
index 7d90335..c6c73ca 100644
--- a/include/qemu/id.h
+++ b/include/qemu/id.h
@@ -4,6 +4,7 @@
 typedef enum IdSubSystems {
     ID_QDEV,
     ID_BLOCK,
+    ID_JOB,
     ID_MAX      /* last element, used as array size */
 } IdSubSystems;
 
diff --git a/util/id.c b/util/id.c
index 6141352..eb5478b 100644
--- a/util/id.c
+++ b/util/id.c
@@ -34,6 +34,7 @@ bool id_wellformed(const char *id)
 static const char *const id_subsys_str[ID_MAX] = {
     [ID_QDEV]  = "qdev",
     [ID_BLOCK] = "block",
+    [ID_JOB]   = "job",
 };
 
 /*
-- 
2.8.1

[Qemu-devel] [PATCH 03/15] blockjob: Add block_job_get()

[Alberto Garcia]

Currently the way to look for a specific block job is to iterate the
list manually using block_job_next().

Since we want to be able to identify a job primarily by its ID it
makes sense to have a function that does just that.

Signed-off-by: Alberto Garcia <berto@igalia.com>
---
 blockjob.c               | 13 +++++++++++++
 include/block/blockjob.h | 10 ++++++++++
 2 files changed, 23 insertions(+)

diff --git a/blockjob.c b/blockjob.c
index 4d42987..a4a1caf 100644
--- a/blockjob.c
+++ b/blockjob.c
@@ -61,6 +61,19 @@ BlockJob *block_job_next(BlockJob *job)
     return QLIST_NEXT(job, job_list);
 }
 
+BlockJob *block_job_get(const char *id)
+{
+    BlockJob *job;
+
+    QLIST_FOREACH(job, &block_jobs, job_list) {
+        if (!strcmp(id, job->id)) {
+            return job;
+        }
+    }
+
+    return NULL;
+}
+
 void *block_job_create(const BlockJobDriver *driver, BlockDriverState *bs,
                        int64_t speed, BlockCompletionFunc *cb,
                        void *opaque, Error **errp)
diff --git a/include/block/blockjob.h b/include/block/blockjob.h
index 1533006..46d2af2 100644
--- a/include/block/blockjob.h
+++ b/include/block/blockjob.h
@@ -191,6 +191,16 @@ struct BlockJob {
 BlockJob *block_job_next(BlockJob *job);
 
 /**
+ * block_job_get:
+ * @id: The id of the block job.
+ *
+ * Get the block job identified by @id (which must not be %NULL).
+ *
+ * Returns the requested job, or %NULL if it doesn't exist.
+ */
+BlockJob *block_job_get(const char *id);
+
+/**
  * block_job_create:
  * @job_type: The class object for the newly-created job.
  * @bs: The block
-- 
2.8.1

[Qemu-devel] [PATCH 04/15] block: Simplify find_block_job() and make it accept a job ID

[Alberto Garcia]

find_block_job() looks for a block backend with a specified name,
checks whether it has a block job and acquires its AioContext. This
patch uses block_job_next() and iterate directly over the block jobs.

In addition to that we want to identify jobs primarily by their ID, so
this patch updates find_block_job() to allow IDs too. Only one of ID
and device name can be specified when looking for a block job.

Signed-off-by: Alberto Garcia <berto@igalia.com>
---
 blockdev.c | 66 +++++++++++++++++++++++++++++++++-----------------------------
 1 file changed, 35 insertions(+), 31 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index 52ec4ae..bd0d5a1 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -3689,48 +3689,52 @@ void qmp_blockdev_mirror(const char *device, const char *target,
     aio_context_release(aio_context);
 }
 
-/* Get the block job for a given device name and acquire its AioContext */
-static BlockJob *find_block_job(const char *device, AioContext **aio_context,
-                                Error **errp)
+/* Get a block job using its ID or device name and acquire its AioContext */
+static BlockJob *find_block_job(const char *id, const char *device,
+                                AioContext **aio_context, Error **errp)
 {
-    BlockBackend *blk;
-    BlockDriverState *bs;
+    BlockJob *job = NULL;
 
     *aio_context = NULL;
 
-    blk = blk_by_name(device);
-    if (!blk) {
-        goto notfound;
+    if ((id && device) || (!id && !device)) {
+        error_setg(errp, "Only one of ID or device name "
+                   "must be specified when looking for a block job");
+        return NULL;
     }
 
-    *aio_context = blk_get_aio_context(blk);
-    aio_context_acquire(*aio_context);
-
-    if (!blk_is_available(blk)) {
-        goto notfound;
+    if (id) {
+        job = block_job_get(id);
+    } else {
+        BlockJob *j;
+        for (j = block_job_next(NULL); j; j = block_job_next(j)) {
+            if (!strcmp(device, j->device)) {
+                if (job) {
+                    /* This scenario is currently not possible, but it
+                     * could theoretically happen in the future. */
+                    error_setg(errp, "More than one job on device '%s', "
+                               "use the job ID instead", device);
+                    return NULL;
+                }
+                job = j;
+            }
+        }
     }
-    bs = blk_bs(blk);
 
-    if (!bs->job) {
-        goto notfound;
+    if (job) {
+        *aio_context = blk_get_aio_context(job->blk);
+        aio_context_acquire(*aio_context);
+    } else {
+        error_setg(errp, "Block job '%s' not found", id ? id : device);
     }
 
-    return bs->job;
-
-notfound:
-    error_set(errp, ERROR_CLASS_DEVICE_NOT_ACTIVE,
-              "No active block job on device '%s'", device);
-    if (*aio_context) {
-        aio_context_release(*aio_context);
-        *aio_context = NULL;
-    }
-    return NULL;
+    return job;
 }
 
 void qmp_block_job_set_speed(const char *device, int64_t speed, Error **errp)
 {
     AioContext *aio_context;
-    BlockJob *job = find_block_job(device, &aio_context, errp);
+    BlockJob *job = find_block_job(NULL, device, &aio_context, errp);
 
     if (!job) {
         return;
@@ -3744,7 +3748,7 @@ void qmp_block_job_cancel(const char *device,
                           bool has_force, bool force, Error **errp)
 {
     AioContext *aio_context;
-    BlockJob *job = find_block_job(device, &aio_context, errp);
+    BlockJob *job = find_block_job(NULL, device, &aio_context, errp);
 
     if (!job) {
         return;
@@ -3769,7 +3773,7 @@ out:
 void qmp_block_job_pause(const char *device, Error **errp)
 {
     AioContext *aio_context;
-    BlockJob *job = find_block_job(device, &aio_context, errp);
+    BlockJob *job = find_block_job(NULL, device, &aio_context, errp);
 
     if (!job || job->user_paused) {
         return;
@@ -3784,7 +3788,7 @@ void qmp_block_job_pause(const char *device, Error **errp)
 void qmp_block_job_resume(const char *device, Error **errp)
 {
     AioContext *aio_context;
-    BlockJob *job = find_block_job(device, &aio_context, errp);
+    BlockJob *job = find_block_job(NULL, device, &aio_context, errp);
 
     if (!job || !job->user_paused) {
         return;
@@ -3799,7 +3803,7 @@ void qmp_block_job_resume(const char *device, Error **errp)
 void qmp_block_job_complete(const char *device, Error **errp)
 {
     AioContext *aio_context;
-    BlockJob *job = find_block_job(device, &aio_context, errp);
+    BlockJob *job = find_block_job(NULL, device, &aio_context, errp);
 
     if (!job) {
         return;
-- 
2.8.1

[Qemu-devel] [PATCH 05/15] blockjob: Add 'job_id' parameter to block_job_create()

[Alberto Garcia]

Job IDs are generated automatically when a new job is created. This
patch adds a new 'job_id' parameter to let the caller provide one
instead. In this case the ID is verified to be unique and well-formed.

Signed-off-by: Alberto Garcia <berto@igalia.com>
---
 block/backup.c            |  3 ++-
 block/commit.c            |  2 +-
 block/mirror.c            |  2 +-
 block/stream.c            |  2 +-
 blockjob.c                | 19 +++++++++++++++----
 include/block/blockjob.h  |  8 +++++---
 tests/test-blockjob-txn.c |  2 +-
 7 files changed, 26 insertions(+), 12 deletions(-)

diff --git a/block/backup.c b/block/backup.c
index feeb9f8..9245c0c 100644
--- a/block/backup.c
+++ b/block/backup.c
@@ -536,7 +536,8 @@ void backup_start(BlockDriverState *bs, BlockDriverState *target,
         goto error;
     }
 
-    job = block_job_create(&backup_job_driver, bs, speed, cb, opaque, errp);
+    job = block_job_create(NULL, &backup_job_driver, bs, speed,
+                           cb, opaque, errp);
     if (!job) {
         goto error;
     }
diff --git a/block/commit.c b/block/commit.c
index 444333b..3535ba7 100644
--- a/block/commit.c
+++ b/block/commit.c
@@ -236,7 +236,7 @@ void commit_start(BlockDriverState *bs, BlockDriverState *base,
         return;
     }
 
-    s = block_job_create(&commit_job_driver, bs, speed, cb, opaque, errp);
+    s = block_job_create(NULL, &commit_job_driver, bs, speed, cb, opaque, errp);
     if (!s) {
         return;
     }
diff --git a/block/mirror.c b/block/mirror.c
index 80fd3c7..28ed447 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -824,7 +824,7 @@ static void mirror_start_job(BlockDriverState *bs, BlockDriverState *target,
         buf_size = DEFAULT_MIRROR_BUF_SIZE;
     }
 
-    s = block_job_create(driver, bs, speed, cb, opaque, errp);
+    s = block_job_create(NULL, driver, bs, speed, cb, opaque, errp);
     if (!s) {
         return;
     }
diff --git a/block/stream.c b/block/stream.c
index c0efbda..e4319d3 100644
--- a/block/stream.c
+++ b/block/stream.c
@@ -226,7 +226,7 @@ void stream_start(BlockDriverState *bs, BlockDriverState *base,
 {
     StreamBlockJob *s;
 
-    s = block_job_create(&stream_job_driver, bs, speed, cb, opaque, errp);
+    s = block_job_create(NULL, &stream_job_driver, bs, speed, cb, opaque, errp);
     if (!s) {
         return;
     }
diff --git a/blockjob.c b/blockjob.c
index a4a1caf..dd0eb7f 100644
--- a/blockjob.c
+++ b/blockjob.c
@@ -74,9 +74,9 @@ BlockJob *block_job_get(const char *id)
     return NULL;
 }
 
-void *block_job_create(const BlockJobDriver *driver, BlockDriverState *bs,
-                       int64_t speed, BlockCompletionFunc *cb,
-                       void *opaque, Error **errp)
+void *block_job_create(const char *job_id, const BlockJobDriver *driver,
+                       BlockDriverState *bs, int64_t speed,
+                       BlockCompletionFunc *cb, void *opaque, Error **errp)
 {
     BlockBackend *blk;
     BlockJob *job;
@@ -86,6 +86,17 @@ void *block_job_create(const BlockJobDriver *driver, BlockDriverState *bs,
         return NULL;
     }
 
+    if (job_id) {
+        if (!id_wellformed(job_id)) {
+            error_setg(errp, "Invalid job ID '%s'", job_id);
+            return NULL;
+        }
+        if (block_job_get(job_id)) {
+            error_setg(errp, "Job ID '%s' already in use", job_id);
+            return NULL;
+        }
+    }
+
     blk = blk_new();
     blk_insert_bs(blk, bs);
 
@@ -97,7 +108,7 @@ void *block_job_create(const BlockJobDriver *driver, BlockDriverState *bs,
 
     job->driver        = driver;
     job->device        = g_strdup(bdrv_get_device_name(bs));
-    job->id            = id_generate(ID_JOB);
+    job->id            = job_id ? g_strdup(job_id) : id_generate(ID_JOB);
     job->blk           = blk;
     job->cb            = cb;
     job->opaque        = opaque;
diff --git a/include/block/blockjob.h b/include/block/blockjob.h
index 46d2af2..0f2134d 100644
--- a/include/block/blockjob.h
+++ b/include/block/blockjob.h
@@ -202,6 +202,8 @@ BlockJob *block_job_get(const char *id);
 
 /**
  * block_job_create:
+ * @job_id: The id of the newly-created job, or %NULL to have one
+ * generated automatically.
  * @job_type: The class object for the newly-created job.
  * @bs: The block
  * @speed: The maximum speed, in bytes per second, or 0 for unlimited.
@@ -218,9 +220,9 @@ BlockJob *block_job_get(const char *id);
  * This function is not part of the public job interface; it should be
  * 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,
-                       void *opaque, Error **errp);
+void *block_job_create(const char *job_id, const BlockJobDriver *driver,
+                       BlockDriverState *bs, int64_t speed,
+                       BlockCompletionFunc *cb, void *opaque, Error **errp);
 
 /**
  * block_job_sleep_ns:
diff --git a/tests/test-blockjob-txn.c b/tests/test-blockjob-txn.c
index 828389b..3330d18 100644
--- a/tests/test-blockjob-txn.c
+++ b/tests/test-blockjob-txn.c
@@ -95,7 +95,7 @@ static BlockJob *test_block_job_start(unsigned int iterations,
 
     data = g_new0(TestBlockJobCBData, 1);
     bs = bdrv_new();
-    s = block_job_create(&test_block_job_driver, bs, 0, test_block_job_cb,
+    s = block_job_create(NULL, &test_block_job_driver, bs, 0, test_block_job_cb,
                          data, &error_abort);
     s->iterations = iterations;
     s->use_timer = use_timer;
-- 
2.8.1

[Qemu-devel] [PATCH 06/15] mirror: Add 'job-id' parameter to 'blockdev-mirror' and 'drive-mirror'

[Alberto Garcia]

This patch adds a new optional 'job-id' parameter to 'blockdev-mirror'
and 'drive-mirror', allowing the user to specify the ID of the block
job to be created.

The HMP 'drive_mirror' command remains unchanged.

Signed-off-by: Alberto Garcia <berto@igalia.com>
---
 block/mirror.c            | 14 +++++++-------
 blockdev.c                | 15 ++++++++-------
 hmp.c                     |  2 +-
 include/block/block_int.h |  6 ++++--
 qapi/block-core.json      | 10 +++++++---
 qmp-commands.hx           |  8 +++++---
 6 files changed, 32 insertions(+), 23 deletions(-)

diff --git a/block/mirror.c b/block/mirror.c
index 28ed447..ab08da8 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -795,8 +795,8 @@ static const BlockJobDriver commit_active_job_driver = {
     .complete      = mirror_complete,
 };
 
-static void mirror_start_job(BlockDriverState *bs, BlockDriverState *target,
-                             const char *replaces,
+static void mirror_start_job(const char *job_id, BlockDriverState *bs,
+                             BlockDriverState *target, const char *replaces,
                              int64_t speed, uint32_t granularity,
                              int64_t buf_size,
                              BlockdevOnError on_source_error,
@@ -824,7 +824,7 @@ static void mirror_start_job(BlockDriverState *bs, BlockDriverState *target,
         buf_size = DEFAULT_MIRROR_BUF_SIZE;
     }
 
-    s = block_job_create(NULL, driver, bs, speed, cb, opaque, errp);
+    s = block_job_create(job_id, driver, bs, speed, cb, opaque, errp);
     if (!s) {
         return;
     }
@@ -856,8 +856,8 @@ static void mirror_start_job(BlockDriverState *bs, BlockDriverState *target,
     qemu_coroutine_enter(s->common.co, s);
 }
 
-void mirror_start(BlockDriverState *bs, BlockDriverState *target,
-                  const char *replaces,
+void mirror_start(const char *job_id, BlockDriverState *bs,
+                  BlockDriverState *target, const char *replaces,
                   int64_t speed, uint32_t granularity, int64_t buf_size,
                   MirrorSyncMode mode, BlockdevOnError on_source_error,
                   BlockdevOnError on_target_error,
@@ -874,7 +874,7 @@ 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,
+    mirror_start_job(job_id, bs, target, replaces,
                      speed, granularity, buf_size,
                      on_source_error, on_target_error, unmap, cb, opaque, errp,
                      &mirror_job_driver, is_none_mode, base);
@@ -922,7 +922,7 @@ void commit_active_start(BlockDriverState *bs, BlockDriverState *base,
         }
     }
 
-    mirror_start_job(bs, base, NULL, speed, 0, 0,
+    mirror_start_job(NULL, bs, base, NULL, speed, 0, 0,
                      on_error, on_error, false, cb, opaque, &local_err,
                      &commit_active_job_driver, false, base);
     if (local_err) {
diff --git a/blockdev.c b/blockdev.c
index bd0d5a1..b05dfff 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -3412,7 +3412,7 @@ void qmp_blockdev_backup(const char *device, const char *target,
 /* Parameter check and block job starting for drive mirroring.
  * Caller should hold @device and @target's aio context (must be the same).
  **/
-static void blockdev_mirror_common(BlockDriverState *bs,
+static void blockdev_mirror_common(const char *job_id, BlockDriverState *bs,
                                    BlockDriverState *target,
                                    bool has_replaces, const char *replaces,
                                    enum MirrorSyncMode sync,
@@ -3471,15 +3471,15 @@ static void blockdev_mirror_common(BlockDriverState *bs,
     /* pass the node name to replace to mirror start since it's loose coupling
      * and will allow to check whether the node still exist at mirror completion
      */
-    mirror_start(bs, target,
+    mirror_start(job_id, bs, target,
                  has_replaces ? replaces : NULL,
                  speed, granularity, buf_size, sync,
                  on_source_error, on_target_error, unmap,
                  block_job_cb, bs, errp);
 }
 
-void qmp_drive_mirror(const char *device, const char *target,
-                      bool has_format, const char *format,
+void qmp_drive_mirror(bool has_job_id, const char *job_id, const char *device,
+                      const char *target, bool has_format, const char *format,
                       bool has_node_name, const char *node_name,
                       bool has_replaces, const char *replaces,
                       enum MirrorSyncMode sync,
@@ -3616,7 +3616,7 @@ void qmp_drive_mirror(const char *device, const char *target,
 
     bdrv_set_aio_context(target_bs, aio_context);
 
-    blockdev_mirror_common(bs, target_bs,
+    blockdev_mirror_common(has_job_id ? job_id : NULL, bs, target_bs,
                            has_replaces, replaces, sync,
                            has_speed, speed,
                            has_granularity, granularity,
@@ -3633,7 +3633,8 @@ out:
     aio_context_release(aio_context);
 }
 
-void qmp_blockdev_mirror(const char *device, const char *target,
+void qmp_blockdev_mirror(bool has_job_id, const char *job_id,
+                         const char *device, const char *target,
                          bool has_replaces, const char *replaces,
                          MirrorSyncMode sync,
                          bool has_speed, int64_t speed,
@@ -3673,7 +3674,7 @@ void qmp_blockdev_mirror(const char *device, const char *target,
 
     bdrv_set_aio_context(target_bs, aio_context);
 
-    blockdev_mirror_common(bs, target_bs,
+    blockdev_mirror_common(has_job_id ? job_id : NULL, bs, target_bs,
                            has_replaces, replaces, sync,
                            has_speed, speed,
                            has_granularity, granularity,
diff --git a/hmp.c b/hmp.c
index a4b1d3d..7dab1b9 100644
--- a/hmp.c
+++ b/hmp.c
@@ -1093,7 +1093,7 @@ void hmp_drive_mirror(Monitor *mon, const QDict *qdict)
         mode = NEW_IMAGE_MODE_ABSOLUTE_PATHS;
     }
 
-    qmp_drive_mirror(device, filename, !!format, format,
+    qmp_drive_mirror(false, NULL, device, filename, !!format, format,
                      false, NULL, false, NULL,
                      full ? MIRROR_SYNC_MODE_FULL : MIRROR_SYNC_MODE_TOP,
                      true, mode, false, 0, false, 0, false, 0,
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 3d95ab3..e1f5643 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -665,6 +665,8 @@ void commit_active_start(BlockDriverState *bs, BlockDriverState *base,
                          void *opaque, Error **errp);
 /*
  * mirror_start:
+ * @job_id: The id of the newly-created job, or %NULL to have one
+ * generated automatically.
  * @bs: Block device to operate on.
  * @target: Block device to write to.
  * @replaces: Block graph node name to replace once the mirror is done. Can
@@ -685,8 +687,8 @@ void commit_active_start(BlockDriverState *bs, BlockDriverState *base,
  * manually completed.  At the end of a successful mirroring job,
  * @bs will be switched to read from @target.
  */
-void mirror_start(BlockDriverState *bs, BlockDriverState *target,
-                  const char *replaces,
+void mirror_start(const char *job_id, BlockDriverState *bs,
+                  BlockDriverState *target, const char *replaces,
                   int64_t speed, uint32_t granularity, int64_t buf_size,
                   MirrorSyncMode mode, BlockdevOnError on_source_error,
                   BlockdevOnError on_target_error,
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 98a20d2..6f015c8 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -1108,6 +1108,8 @@
 #
 # Start mirroring a block device's writes to a new destination.
 #
+# @job-id: #optional identifier for the newly-created block job (Since 2.7)
+#
 # @device:  the name of the device whose writes should be mirrored.
 #
 # @target: the target of the new image. If the file exists, or if it
@@ -1160,8 +1162,8 @@
 # Since 1.3
 ##
 { 'command': 'drive-mirror',
-  'data': { 'device': 'str', 'target': 'str', '*format': 'str',
-            '*node-name': 'str', '*replaces': 'str',
+  'data': { '*job-id': 'str', 'device': 'str', 'target': 'str',
+            '*format': 'str', '*node-name': 'str', '*replaces': 'str',
             'sync': 'MirrorSyncMode', '*mode': 'NewImageMode',
             '*speed': 'int', '*granularity': 'uint32',
             '*buf-size': 'int', '*on-source-error': 'BlockdevOnError',
@@ -1243,6 +1245,8 @@
 #
 # Start mirroring a block device's writes to a new destination.
 #
+# @job-id: #optional identifier for the newly-created block job (Since 2.7)
+#
 # @device: the name of the device whose writes should be mirrored.
 #
 # @target: the id or node-name of the block device to mirror to. This mustn't be
@@ -1279,7 +1283,7 @@
 # Since 2.6
 ##
 { 'command': 'blockdev-mirror',
-  'data': { 'device': 'str', 'target': 'str',
+  'data': { '*job-id': 'str', 'device': 'str', 'target': 'str',
             '*replaces': 'str',
             'sync': 'MirrorSyncMode',
             '*speed': 'int', '*granularity': 'uint32',
diff --git a/qmp-commands.hx b/qmp-commands.hx
index 28801a2..fd99468 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -1629,8 +1629,8 @@ EQMP
 
     {
         .name       = "drive-mirror",
-        .args_type  = "sync:s,device:B,target:s,speed:i?,mode:s?,format:s?,"
-                      "node-name:s?,replaces:s?,"
+        .args_type  = "job-id:s?,sync:s,device:B,target:s,speed:i?,mode:s?,"
+                      "format:s?,node-name:s?,replaces:s?,"
                       "on-source-error:s?,on-target-error:s?,"
                       "unmap:b?,"
                       "granularity:i?,buf-size:i?",
@@ -1650,6 +1650,7 @@ of the source.
 
 Arguments:
 
+- "job-id": identifier for the newly-created block job (json-string, optional)
 - "device": device name to operate on (json-string)
 - "target": name of new image file (json-string)
 - "format": format of new image (json-string, optional)
@@ -1693,7 +1694,7 @@ EQMP
 
     {
         .name       = "blockdev-mirror",
-        .args_type  = "sync:s,device:B,target:B,replaces:s?,speed:i?,"
+        .args_type  = "job-id:s?,sync:s,device:B,target:B,replaces:s?,speed:i?,"
                       "on-source-error:s?,on-target-error:s?,"
                       "granularity:i?,buf-size:i?",
         .mhandler.cmd_new = qmp_marshal_blockdev_mirror,
@@ -1708,6 +1709,7 @@ specifies the target of mirror operation.
 
 Arguments:
 
+- "job-id": identifier for the newly-created block job (json-string, optional)
 - "device": device name to operate on (json-string)
 - "target": device name to mirror to (json-string)
 - "replaces": the block driver node name to replace when finished
-- 
2.8.1

[Qemu-devel] [PATCH 07/15] backup: Add 'job-id' parameter to 'blockdev-backup' and 'drive-backup'

[Alberto Garcia]

This patch adds a new optional 'job-id' parameter to 'blockdev-backup'
and 'drive-backup', allowing the user to specify the ID of the block
job to be created.

The HMP 'drive_backup' command remains unchanged.

Signed-off-by: Alberto Garcia <berto@igalia.com>
---
 block/backup.c            |  8 ++++----
 blockdev.c                | 43 ++++++++++++++++++++++++-------------------
 hmp.c                     |  2 +-
 include/block/block_int.h |  8 +++++---
 qapi/block-core.json      | 10 +++++++---
 qmp-commands.hx           |  8 +++++---
 6 files changed, 46 insertions(+), 33 deletions(-)

diff --git a/block/backup.c b/block/backup.c
index 9245c0c..501ae76 100644
--- a/block/backup.c
+++ b/block/backup.c
@@ -468,9 +468,9 @@ static void coroutine_fn backup_run(void *opaque)
     block_job_defer_to_main_loop(&job->common, backup_complete, data);
 }
 
-void backup_start(BlockDriverState *bs, BlockDriverState *target,
-                  int64_t speed, MirrorSyncMode sync_mode,
-                  BdrvDirtyBitmap *sync_bitmap,
+void backup_start(const char *job_id, BlockDriverState *bs,
+                  BlockDriverState *target, int64_t speed,
+                  MirrorSyncMode sync_mode, BdrvDirtyBitmap *sync_bitmap,
                   BlockdevOnError on_source_error,
                   BlockdevOnError on_target_error,
                   BlockCompletionFunc *cb, void *opaque,
@@ -536,7 +536,7 @@ void backup_start(BlockDriverState *bs, BlockDriverState *target,
         goto error;
     }
 
-    job = block_job_create(NULL, &backup_job_driver, bs, speed,
+    job = block_job_create(job_id, &backup_job_driver, bs, speed,
                            cb, opaque, errp);
     if (!job) {
         goto error;
diff --git a/blockdev.c b/blockdev.c
index b05dfff..4bf1cb3 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1834,9 +1834,9 @@ typedef struct DriveBackupState {
     BlockJob *job;
 } DriveBackupState;
 
-static void do_drive_backup(const char *device, const char *target,
-                            bool has_format, const char *format,
-                            enum MirrorSyncMode sync,
+static void do_drive_backup(const char *job_id, const char *device,
+                            const char *target, bool has_format,
+                            const char *format, enum MirrorSyncMode sync,
                             bool has_mode, enum NewImageMode mode,
                             bool has_speed, int64_t speed,
                             bool has_bitmap, const char *bitmap,
@@ -1874,7 +1874,8 @@ static void drive_backup_prepare(BlkActionState *common, Error **errp)
     bdrv_drained_begin(blk_bs(blk));
     state->bs = blk_bs(blk);
 
-    do_drive_backup(backup->device, backup->target,
+    do_drive_backup(backup->has_job_id ? backup->job_id : NULL,
+                    backup->device, backup->target,
                     backup->has_format, backup->format,
                     backup->sync,
                     backup->has_mode, backup->mode,
@@ -1919,8 +1920,8 @@ typedef struct BlockdevBackupState {
     AioContext *aio_context;
 } BlockdevBackupState;
 
-static void do_blockdev_backup(const char *device, const char *target,
-                               enum MirrorSyncMode sync,
+static void do_blockdev_backup(const char *job_id, const char *device,
+                               const char *target, enum MirrorSyncMode sync,
                                bool has_speed, int64_t speed,
                                bool has_on_source_error,
                                BlockdevOnError on_source_error,
@@ -1966,8 +1967,8 @@ static void blockdev_backup_prepare(BlkActionState *common, Error **errp)
     state->bs = blk_bs(blk);
     bdrv_drained_begin(state->bs);
 
-    do_blockdev_backup(backup->device, backup->target,
-                       backup->sync,
+    do_blockdev_backup(backup->has_job_id ? backup->job_id : NULL,
+                       backup->device, backup->target, backup->sync,
                        backup->has_speed, backup->speed,
                        backup->has_on_source_error, backup->on_source_error,
                        backup->has_on_target_error, backup->on_target_error,
@@ -3175,9 +3176,9 @@ out:
     aio_context_release(aio_context);
 }
 
-static void do_drive_backup(const char *device, const char *target,
-                            bool has_format, const char *format,
-                            enum MirrorSyncMode sync,
+static void do_drive_backup(const char *job_id, const char *device,
+                            const char *target, bool has_format,
+                            const char *format, enum MirrorSyncMode sync,
                             bool has_mode, enum NewImageMode mode,
                             bool has_speed, int64_t speed,
                             bool has_bitmap, const char *bitmap,
@@ -3296,7 +3297,7 @@ static void do_drive_backup(const char *device, const char *target,
         }
     }
 
-    backup_start(bs, target_bs, speed, sync, bmap,
+    backup_start(job_id, bs, target_bs, speed, sync, bmap,
                  on_source_error, on_target_error,
                  block_job_cb, bs, txn, &local_err);
     bdrv_unref(target_bs);
@@ -3309,7 +3310,8 @@ out:
     aio_context_release(aio_context);
 }
 
-void qmp_drive_backup(const char *device, const char *target,
+void qmp_drive_backup(bool has_job_id, const char *job_id,
+                      const char *device, const char *target,
                       bool has_format, const char *format,
                       enum MirrorSyncMode sync,
                       bool has_mode, enum NewImageMode mode,
@@ -3319,7 +3321,8 @@ void qmp_drive_backup(const char *device, const char *target,
                       bool has_on_target_error, BlockdevOnError on_target_error,
                       Error **errp)
 {
-    return do_drive_backup(device, target, has_format, format, sync,
+    return do_drive_backup(has_job_id ? job_id : NULL, device, target,
+                           has_format, format, sync,
                            has_mode, mode, has_speed, speed,
                            has_bitmap, bitmap,
                            has_on_source_error, on_source_error,
@@ -3332,8 +3335,8 @@ BlockDeviceInfoList *qmp_query_named_block_nodes(Error **errp)
     return bdrv_named_nodes_list(errp);
 }
 
-void do_blockdev_backup(const char *device, const char *target,
-                         enum MirrorSyncMode sync,
+void do_blockdev_backup(const char *job_id, const char *device,
+                        const char *target, enum MirrorSyncMode sync,
                          bool has_speed, int64_t speed,
                          bool has_on_source_error,
                          BlockdevOnError on_source_error,
@@ -3385,7 +3388,7 @@ void do_blockdev_backup(const char *device, const char *target,
     target_bs = blk_bs(target_blk);
 
     bdrv_set_aio_context(target_bs, aio_context);
-    backup_start(bs, target_bs, speed, sync, NULL, on_source_error,
+    backup_start(job_id, bs, target_bs, speed, sync, NULL, on_source_error,
                  on_target_error, block_job_cb, bs, txn, &local_err);
     if (local_err != NULL) {
         error_propagate(errp, local_err);
@@ -3394,7 +3397,8 @@ out:
     aio_context_release(aio_context);
 }
 
-void qmp_blockdev_backup(const char *device, const char *target,
+void qmp_blockdev_backup(bool has_job_id, const char *job_id,
+                         const char *device, const char *target,
                          enum MirrorSyncMode sync,
                          bool has_speed, int64_t speed,
                          bool has_on_source_error,
@@ -3403,7 +3407,8 @@ void qmp_blockdev_backup(const char *device, const char *target,
                          BlockdevOnError on_target_error,
                          Error **errp)
 {
-    do_blockdev_backup(device, target, sync, has_speed, speed,
+    do_blockdev_backup(has_job_id ? job_id : NULL, device, target,
+                       sync, has_speed, speed,
                        has_on_source_error, on_source_error,
                        has_on_target_error, on_target_error,
                        NULL, errp);
diff --git a/hmp.c b/hmp.c
index 7dab1b9..1918750 100644
--- a/hmp.c
+++ b/hmp.c
@@ -1123,7 +1123,7 @@ void hmp_drive_backup(Monitor *mon, const QDict *qdict)
         mode = NEW_IMAGE_MODE_ABSOLUTE_PATHS;
     }
 
-    qmp_drive_backup(device, filename, !!format, format,
+    qmp_drive_backup(false, NULL, device, filename, !!format, format,
                      full ? MIRROR_SYNC_MODE_FULL : MIRROR_SYNC_MODE_TOP,
                      true, mode, false, 0, false, NULL,
                      false, 0, false, 0, &err);
diff --git a/include/block/block_int.h b/include/block/block_int.h
index e1f5643..aa67d29 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -698,6 +698,8 @@ void mirror_start(const char *job_id, BlockDriverState *bs,
 
 /*
  * backup_start:
+ * @job_id: The id of the newly-created job, or %NULL to have one
+ * generated automatically.
  * @bs: Block device to operate on.
  * @target: Block device to write to.
  * @speed: The maximum speed, in bytes per second, or 0 for unlimited.
@@ -712,9 +714,9 @@ void mirror_start(const char *job_id, BlockDriverState *bs,
  * Start a backup operation on @bs.  Clusters in @bs are written to @target
  * until the job is cancelled or manually completed.
  */
-void backup_start(BlockDriverState *bs, BlockDriverState *target,
-                  int64_t speed, MirrorSyncMode sync_mode,
-                  BdrvDirtyBitmap *sync_bitmap,
+void backup_start(const char *job_id, BlockDriverState *bs,
+                  BlockDriverState *target, int64_t speed,
+                  MirrorSyncMode sync_mode, BdrvDirtyBitmap *sync_bitmap,
                   BlockdevOnError on_source_error,
                   BlockdevOnError on_target_error,
                   BlockCompletionFunc *cb, void *opaque,
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 6f015c8..e30e38d 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -866,6 +866,8 @@
 ##
 # @DriveBackup
 #
+# @job-id: #optional identifier for the newly-created block job (Since 2.7)
+#
 # @device: the name of the device which should be copied.
 #
 # @target: the target of the new image. If the file exists, or if it
@@ -903,8 +905,8 @@
 # Since: 1.6
 ##
 { 'struct': 'DriveBackup',
-  'data': { 'device': 'str', 'target': 'str', '*format': 'str',
-            'sync': 'MirrorSyncMode', '*mode': 'NewImageMode',
+  'data': { '*job-id': 'str', 'device': 'str', 'target': 'str',
+            '*format': 'str', 'sync': 'MirrorSyncMode', '*mode': 'NewImageMode',
             '*speed': 'int', '*bitmap': 'str',
             '*on-source-error': 'BlockdevOnError',
             '*on-target-error': 'BlockdevOnError' } }
@@ -912,6 +914,8 @@
 ##
 # @BlockdevBackup
 #
+# @job-id: #optional identifier for the newly-created block job (Since 2.7)
+#
 # @device: the name of the device which should be copied.
 #
 # @target: the name of the backup target device.
@@ -938,7 +942,7 @@
 # Since: 2.3
 ##
 { 'struct': 'BlockdevBackup',
-  'data': { 'device': 'str', 'target': 'str',
+  'data': { '*job-id': 'str', 'device': 'str', 'target': 'str',
             'sync': 'MirrorSyncMode',
             '*speed': 'int',
             '*on-source-error': 'BlockdevOnError',
diff --git a/qmp-commands.hx b/qmp-commands.hx
index fd99468..870afdb 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -1185,8 +1185,8 @@ EQMP
 
     {
         .name       = "drive-backup",
-        .args_type  = "sync:s,device:B,target:s,speed:i?,mode:s?,format:s?,"
-                      "bitmap:s?,on-source-error:s?,on-target-error:s?",
+        .args_type  = "job-id:s?,sync:s,device:B,target:s,speed:i?,mode:s?,"
+                      "format:s?,bitmap:s?,on-source-error:s?,on-target-error:s?",
         .mhandler.cmd_new = qmp_marshal_drive_backup,
     },
 
@@ -1202,6 +1202,7 @@ block-job-cancel command.
 
 Arguments:
 
+- "job-id": identifier for the newly-created block job (json-string, optional)
 - "device": the name of the device which should be copied.
             (json-string)
 - "target": the target of the new image. If the file exists, or if it is a
@@ -1239,7 +1240,7 @@ EQMP
 
     {
         .name       = "blockdev-backup",
-        .args_type  = "sync:s,device:B,target:B,speed:i?,"
+        .args_type  = "job-id:s?,sync:s,device:B,target:B,speed:i?,"
                       "on-source-error:s?,on-target-error:s?",
         .mhandler.cmd_new = qmp_marshal_blockdev_backup,
     },
@@ -1253,6 +1254,7 @@ as backup target.
 
 Arguments:
 
+- "job-id": identifier for the newly-created block job (json-string, optional)
 - "device": the name of the device which should be copied.
             (json-string)
 - "target": the name of the backup target device. (json-string)
-- 
2.8.1

[Qemu-devel] [PATCH 08/15] stream: Add 'job-id' parameter to 'block-stream'

[Alberto Garcia]

This patch adds a new optional 'job-id' parameter to 'block-stream',
allowing the user to specify the ID of the block job to be created.

The HMP 'block_stream' command remains unchanged.

Signed-off-by: Alberto Garcia <berto@igalia.com>
---
 block/stream.c            | 12 ++++++------
 blockdev.c                |  6 +++---
 hmp.c                     |  2 +-
 include/block/block_int.h | 10 ++++++----
 qapi/block-core.json      |  7 +++++--
 qmp-commands.hx           |  3 ++-
 6 files changed, 23 insertions(+), 17 deletions(-)

diff --git a/block/stream.c b/block/stream.c
index e4319d3..54c8cd8 100644
--- a/block/stream.c
+++ b/block/stream.c
@@ -218,15 +218,15 @@ static const BlockJobDriver stream_job_driver = {
     .set_speed     = stream_set_speed,
 };
 
-void stream_start(BlockDriverState *bs, BlockDriverState *base,
-                  const char *backing_file_str, int64_t speed,
-                  BlockdevOnError on_error,
-                  BlockCompletionFunc *cb,
-                  void *opaque, Error **errp)
+void stream_start(const char *job_id, BlockDriverState *bs,
+                  BlockDriverState *base, const char *backing_file_str,
+                  int64_t speed, BlockdevOnError on_error,
+                  BlockCompletionFunc *cb, void *opaque, Error **errp)
 {
     StreamBlockJob *s;
 
-    s = block_job_create(NULL, &stream_job_driver, bs, speed, cb, opaque, errp);
+    s = block_job_create(job_id, &stream_job_driver, bs, speed,
+                         cb, opaque, errp);
     if (!s) {
         return;
     }
diff --git a/blockdev.c b/blockdev.c
index 4bf1cb3..f4ff025 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -2998,7 +2998,7 @@ static void block_job_cb(void *opaque, int ret)
     }
 }
 
-void qmp_block_stream(const char *device,
+void qmp_block_stream(bool has_job_id, const char *job_id, const char *device,
                       bool has_base, const char *base,
                       bool has_backing_file, const char *backing_file,
                       bool has_speed, int64_t speed,
@@ -3057,8 +3057,8 @@ void qmp_block_stream(const char *device,
     /* backing_file string overrides base bs filename */
     base_name = has_backing_file ? backing_file : base_name;
 
-    stream_start(bs, base_bs, base_name, has_speed ? speed : 0,
-                 on_error, block_job_cb, bs, &local_err);
+    stream_start(has_job_id ? job_id : NULL, bs, base_bs, base_name,
+                 has_speed ? speed : 0, on_error, block_job_cb, bs, &local_err);
     if (local_err) {
         error_propagate(errp, local_err);
         goto out;
diff --git a/hmp.c b/hmp.c
index 1918750..414a41a 100644
--- a/hmp.c
+++ b/hmp.c
@@ -1481,7 +1481,7 @@ void hmp_block_stream(Monitor *mon, const QDict *qdict)
     const char *base = qdict_get_try_str(qdict, "base");
     int64_t speed = qdict_get_try_int(qdict, "speed", 0);
 
-    qmp_block_stream(device, base != NULL, base, false, NULL,
+    qmp_block_stream(false, NULL, device, base != NULL, base, false, NULL,
                      qdict_haskey(qdict, "speed"), speed,
                      true, BLOCKDEV_ON_ERROR_REPORT, &error);
 
diff --git a/include/block/block_int.h b/include/block/block_int.h
index aa67d29..9693cf4 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -607,6 +607,8 @@ int is_windows_drive(const char *filename);
 
 /**
  * stream_start:
+ * @job_id: The id of the newly-created job, or %NULL to have one
+ * generated automatically.
  * @bs: Block device to operate on.
  * @base: Block device that will become the new base, or %NULL to
  * flatten the whole backing file chain onto @bs.
@@ -625,10 +627,10 @@ int is_windows_drive(const char *filename);
  * @backing_file_str in the written image and to @base in the live
  * BlockDriverState.
  */
-void stream_start(BlockDriverState *bs, BlockDriverState *base,
-                  const char *backing_file_str, int64_t speed,
-                  BlockdevOnError on_error, BlockCompletionFunc *cb,
-                  void *opaque, Error **errp);
+void stream_start(const char *job_id, BlockDriverState *bs,
+                  BlockDriverState *base, const char *backing_file_str,
+                  int64_t speed, BlockdevOnError on_error,
+                  BlockCompletionFunc *cb, void *opaque, Error **errp);
 
 /**
  * commit_start:
diff --git a/qapi/block-core.json b/qapi/block-core.json
index e30e38d..14073f7 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -1421,6 +1421,8 @@
 # On successful completion the image file is updated to drop the backing file
 # and the BLOCK_JOB_COMPLETED event is emitted.
 #
+# @job-id: #optional identifier for the newly-created block job (Since 2.7)
+#
 # @device: the device name
 #
 # @base:   #optional the common backing file name
@@ -1452,8 +1454,9 @@
 # Since: 1.1
 ##
 { 'command': 'block-stream',
-  'data': { 'device': 'str', '*base': 'str', '*backing-file': 'str',
-            '*speed': 'int', '*on-error': 'BlockdevOnError' } }
+  'data': { '*job-id': 'str', 'device': 'str', '*base': 'str',
+            '*backing-file': 'str', '*speed': 'int',
+            '*on-error': 'BlockdevOnError' } }
 
 ##
 # @block-job-set-speed:
diff --git a/qmp-commands.hx b/qmp-commands.hx
index 870afdb..1818e43 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -1079,7 +1079,7 @@ EQMP
 
     {
         .name       = "block-stream",
-        .args_type  = "device:B,base:s?,speed:o?,backing-file:s?,on-error:s?",
+        .args_type  = "job-id:s?,device:B,base:s?,speed:o?,backing-file:s?,on-error:s?",
         .mhandler.cmd_new = qmp_marshal_block_stream,
     },
 
@@ -1091,6 +1091,7 @@ Copy data from a backing file into a block device.
 
 Arguments:
 
+- "job-id": identifier for the newly-created block job (json-string, optional)
 - "device": The device's ID, must be unique (json-string)
 - "base": The file name of the backing image above which copying starts
           (json-string, optional)
-- 
2.8.1

[Qemu-devel] [PATCH 09/15] stream: Add 'job-id' parameter to 'block-commit'

[Alberto Garcia]

This patch adds a new optional 'job-id' parameter to 'block-commit',
allowing the user to specify the ID of the block job to be created.

Signed-off-by: Alberto Garcia <berto@igalia.com>
---
 block/commit.c            |  7 ++++---
 block/mirror.c            |  6 +++---
 blockdev.c                |  9 +++++----
 include/block/block_int.h | 16 ++++++++++------
 qapi/block-core.json      |  4 +++-
 qemu-img.c                |  2 +-
 qmp-commands.hx           |  3 ++-
 7 files changed, 28 insertions(+), 19 deletions(-)

diff --git a/block/commit.c b/block/commit.c
index 3535ba7..6e576d9 100644
--- a/block/commit.c
+++ b/block/commit.c
@@ -211,8 +211,8 @@ static const BlockJobDriver commit_job_driver = {
     .set_speed     = commit_set_speed,
 };
 
-void commit_start(BlockDriverState *bs, BlockDriverState *base,
-                  BlockDriverState *top, int64_t speed,
+void commit_start(const char *job_id, BlockDriverState *bs,
+                  BlockDriverState *base, BlockDriverState *top, int64_t speed,
                   BlockdevOnError on_error, BlockCompletionFunc *cb,
                   void *opaque, const char *backing_file_str, Error **errp)
 {
@@ -236,7 +236,8 @@ void commit_start(BlockDriverState *bs, BlockDriverState *base,
         return;
     }
 
-    s = block_job_create(NULL, &commit_job_driver, bs, speed, cb, opaque, errp);
+    s = block_job_create(job_id, &commit_job_driver, bs, speed,
+                         cb, opaque, errp);
     if (!s) {
         return;
     }
diff --git a/block/mirror.c b/block/mirror.c
index ab08da8..f70d6b6 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -880,8 +880,8 @@ void mirror_start(const char *job_id, BlockDriverState *bs,
                      &mirror_job_driver, is_none_mode, base);
 }
 
-void commit_active_start(BlockDriverState *bs, BlockDriverState *base,
-                         int64_t speed,
+void commit_active_start(const char *job_id, BlockDriverState *bs,
+                         BlockDriverState *base, int64_t speed,
                          BlockdevOnError on_error,
                          BlockCompletionFunc *cb,
                          void *opaque, Error **errp)
@@ -922,7 +922,7 @@ void commit_active_start(BlockDriverState *bs, BlockDriverState *base,
         }
     }
 
-    mirror_start_job(NULL, bs, base, NULL, speed, 0, 0,
+    mirror_start_job(job_id, bs, base, NULL, speed, 0, 0,
                      on_error, on_error, false, cb, opaque, &local_err,
                      &commit_active_job_driver, false, base);
     if (local_err) {
diff --git a/blockdev.c b/blockdev.c
index f4ff025..0f4c433 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -3070,7 +3070,7 @@ out:
     aio_context_release(aio_context);
 }
 
-void qmp_block_commit(const char *device,
+void qmp_block_commit(bool has_job_id, const char *job_id, const char *device,
                       bool has_base, const char *base,
                       bool has_top, const char *top,
                       bool has_backing_file, const char *backing_file,
@@ -3161,10 +3161,11 @@ 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);
+        commit_active_start(has_job_id ? job_id : NULL, 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,
+        commit_start(has_job_id ? job_id : NULL, bs, base_bs, top_bs, speed,
+                     on_error, block_job_cb, bs,
                      has_backing_file ? backing_file : NULL, &local_err);
     }
     if (local_err != NULL) {
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 9693cf4..14dc9fc 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -634,6 +634,8 @@ void stream_start(const char *job_id, BlockDriverState *bs,
 
 /**
  * commit_start:
+ * @job_id: The id of the newly-created job, or %NULL to have one
+ * generated automatically.
  * @bs: Active block device.
  * @top: Top block device to be committed.
  * @base: Block device that will be written into, and become the new top.
@@ -645,12 +647,14 @@ void stream_start(const char *job_id, BlockDriverState *bs,
  * @errp: Error object.
  *
  */
-void commit_start(BlockDriverState *bs, BlockDriverState *base,
-                 BlockDriverState *top, int64_t speed,
-                 BlockdevOnError on_error, BlockCompletionFunc *cb,
-                 void *opaque, const char *backing_file_str, Error **errp);
+void commit_start(const char *job_id, BlockDriverState *bs,
+                  BlockDriverState *base, BlockDriverState *top, int64_t speed,
+                  BlockdevOnError on_error, BlockCompletionFunc *cb,
+                  void *opaque, const char *backing_file_str, Error **errp);
 /**
  * commit_active_start:
+ * @job_id: The id of the newly-created job, or %NULL to have one
+ * generated automatically.
  * @bs: Active block device to be committed.
  * @base: Block device that will be written into, and become the new top.
  * @speed: The maximum speed, in bytes per second, or 0 for unlimited.
@@ -660,8 +664,8 @@ void commit_start(BlockDriverState *bs, BlockDriverState *base,
  * @errp: Error object.
  *
  */
-void commit_active_start(BlockDriverState *bs, BlockDriverState *base,
-                         int64_t speed,
+void commit_active_start(const char *job_id, BlockDriverState *bs,
+                         BlockDriverState *base, int64_t speed,
                          BlockdevOnError on_error,
                          BlockCompletionFunc *cb,
                          void *opaque, Error **errp);
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 14073f7..f754c29 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -1008,6 +1008,8 @@
 # Live commit of data from overlay image nodes into backing nodes - i.e.,
 # writes data between 'top' and 'base' into 'base'.
 #
+# @job-id: #optional identifier for the newly-created block job (Since 2.7)
+#
 # @device:  the name of the device
 #
 # @base:   #optional The file name of the backing image to write data into.
@@ -1059,7 +1061,7 @@
 #
 ##
 { 'command': 'block-commit',
-  'data': { 'device': 'str', '*base': 'str', '*top': 'str',
+  'data': { '*job-id': 'str', 'device': 'str', '*base': 'str', '*top': 'str',
             '*backing-file': 'str', '*speed': 'int' } }
 
 ##
diff --git a/qemu-img.c b/qemu-img.c
index 4b56ad3..1fc01eb 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -910,7 +910,7 @@ static int img_commit(int argc, char **argv)
         .bs   = bs,
     };
 
-    commit_active_start(bs, base_bs, 0, BLOCKDEV_ON_ERROR_REPORT,
+    commit_active_start(NULL, bs, base_bs, 0, BLOCKDEV_ON_ERROR_REPORT,
                         common_block_job_cb, &cbi, &local_err);
     if (local_err) {
         goto done;
diff --git a/qmp-commands.hx b/qmp-commands.hx
index 1818e43..5f801a9 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -1123,7 +1123,7 @@ EQMP
 
     {
         .name       = "block-commit",
-        .args_type  = "device:B,base:s?,top:s?,backing-file:s?,speed:o?",
+        .args_type  = "job-id:s?,device:B,base:s?,top:s?,backing-file:s?,speed:o?",
         .mhandler.cmd_new = qmp_marshal_block_commit,
     },
 
@@ -1136,6 +1136,7 @@ data between 'top' and 'base' into 'base'.
 
 Arguments:
 
+- "job-id": identifier for the newly-created block job (json-string, optional)
 - "device": The device's ID, must be unique (json-string)
 - "base": The file name of the backing image to write data into.
           If not specified, this is the deepest backing image
-- 
2.8.1

[Qemu-devel] [PATCH 10/15] blockjob: Add 'id' parameter to 'block-job-set-speed'

[Alberto Garcia]

This patch allows the 'block-job-set-speed' command to identify the
job by either its ID or its device name. The latter becomes now
optional since the ID alone is enough to identify the job.

The HMP 'block_job_set_speed' command remains unchanged.

Signed-off-by: Alberto Garcia <berto@igalia.com>
---
 blockdev.c           | 7 +++++--
 hmp.c                | 2 +-
 qapi/block-core.json | 9 +++++++--
 qmp-commands.hx      | 2 +-
 4 files changed, 14 insertions(+), 6 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index 0f4c433..5aaa429 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -3738,10 +3738,13 @@ static BlockJob *find_block_job(const char *id, const char *device,
     return job;
 }
 
-void qmp_block_job_set_speed(const char *device, int64_t speed, Error **errp)
+void qmp_block_job_set_speed(bool has_id, const char *id, bool has_device,
+                             const char *device, int64_t speed, Error **errp)
 {
     AioContext *aio_context;
-    BlockJob *job = find_block_job(NULL, device, &aio_context, errp);
+    BlockJob *job = find_block_job(has_id ? id : NULL,
+                                   has_device ? device : NULL,
+                                   &aio_context, errp);
 
     if (!job) {
         return;
diff --git a/hmp.c b/hmp.c
index 414a41a..0bf5558 100644
--- a/hmp.c
+++ b/hmp.c
@@ -1494,7 +1494,7 @@ void hmp_block_job_set_speed(Monitor *mon, const QDict *qdict)
     const char *device = qdict_get_str(qdict, "device");
     int64_t value = qdict_get_int(qdict, "speed");
 
-    qmp_block_job_set_speed(device, value, &error);
+    qmp_block_job_set_speed(false, NULL, true, device, value, &error);
 
     hmp_handle_error(mon, &error);
 }
diff --git a/qapi/block-core.json b/qapi/block-core.json
index f754c29..64038cc 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -1469,7 +1469,12 @@
 #
 # Throttling can be disabled by setting the speed to 0.
 #
-# @device: the device name
+# The job must be identified with the @id or @device parameters, but
+# only one of them must be set.
+#
+# @id: #optional the job identifier. (Since 2.7)
+#
+# @device: #optional the device name.
 #
 # @speed:  the maximum speed, in bytes per second, or 0 for unlimited.
 #          Defaults to 0.
@@ -1480,7 +1485,7 @@
 # Since: 1.1
 ##
 { 'command': 'block-job-set-speed',
-  'data': { 'device': 'str', 'speed': 'int' } }
+  'data': { '*id': 'str', '*device': 'str', 'speed': 'int' } }
 
 ##
 # @block-job-cancel:
diff --git a/qmp-commands.hx b/qmp-commands.hx
index 5f801a9..bbbab53 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -1284,7 +1284,7 @@ EQMP
 
     {
         .name       = "block-job-set-speed",
-        .args_type  = "device:B,speed:o",
+        .args_type  = "id:s?,device:B?,speed:o",
         .mhandler.cmd_new = qmp_marshal_block_job_set_speed,
     },
 
-- 
2.8.1

[Qemu-devel] [PATCH 11/15] blockjob: Add 'id' parameter to 'block-job-cancel'

[Alberto Garcia]

This patch allows the 'block-job-cancel' command to identify the
job by either its ID or its device name. The latter becomes now
optional since the ID alone is enough to identify the job.

The HMP 'block_job_cancel' command remains unchanged.

Signed-off-by: Alberto Garcia <berto@igalia.com>
---
 blockdev.c           |  7 +++++--
 hmp.c                |  2 +-
 qapi/block-core.json | 10 ++++++++--
 qmp-commands.hx      |  2 +-
 4 files changed, 15 insertions(+), 6 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index 5aaa429..97041b4 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -3754,11 +3754,14 @@ void qmp_block_job_set_speed(bool has_id, const char *id, bool has_device,
     aio_context_release(aio_context);
 }
 
-void qmp_block_job_cancel(const char *device,
+void qmp_block_job_cancel(bool has_id, const char *id,
+                          bool has_device, const char *device,
                           bool has_force, bool force, Error **errp)
 {
     AioContext *aio_context;
-    BlockJob *job = find_block_job(NULL, device, &aio_context, errp);
+    BlockJob *job = find_block_job(has_id ? id : NULL,
+                                   has_device ? device : NULL,
+                                   &aio_context, errp);
 
     if (!job) {
         return;
diff --git a/hmp.c b/hmp.c
index 0bf5558..adeb4de 100644
--- a/hmp.c
+++ b/hmp.c
@@ -1505,7 +1505,7 @@ void hmp_block_job_cancel(Monitor *mon, const QDict *qdict)
     const char *device = qdict_get_str(qdict, "device");
     bool force = qdict_get_try_bool(qdict, "force", false);
 
-    qmp_block_job_cancel(device, true, force, &error);
+    qmp_block_job_cancel(false, NULL, true, device, true, force, &error);
 
     hmp_handle_error(mon, &error);
 }
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 64038cc..38dd3f2 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -1505,7 +1505,12 @@
 # operation can be started at a later time to finish copying all data from the
 # backing file.
 #
-# @device: the device name
+# The job must be identified with the @id or @device parameters, but
+# only one of them must be set.
+#
+# @id: #optional the job identifier (Since 2.7)
+#
+# @device: #optional the device name
 #
 # @force: #optional whether to allow cancellation of a paused job (default
 #         false).  Since 1.3.
@@ -1515,7 +1520,8 @@
 #
 # Since: 1.1
 ##
-{ 'command': 'block-job-cancel', 'data': { 'device': 'str', '*force': 'bool' } }
+{ 'command': 'block-job-cancel',
+  'data': { '*id': 'str', '*device': 'str', '*force': 'bool' } }
 
 ##
 # @block-job-pause:
diff --git a/qmp-commands.hx b/qmp-commands.hx
index bbbab53..6bf8c3d 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -1290,7 +1290,7 @@ EQMP
 
     {
         .name       = "block-job-cancel",
-        .args_type  = "device:B,force:b?",
+        .args_type  = "id:s?,device:B?,force:b?",
         .mhandler.cmd_new = qmp_marshal_block_job_cancel,
     },
     {
-- 
2.8.1

[Qemu-devel] [PATCH 12/15] blockjob: Add 'id' parameter to 'block-job-pause'

[Alberto Garcia]

This patch allows the 'block-job-pause' command to identify the
job by either its ID or its device name. The latter becomes now
optional since the ID alone is enough to identify the job.

The HMP 'block_job_pause' command remains unchanged.

Signed-off-by: Alberto Garcia <berto@igalia.com>
---
 blockdev.c           | 7 +++++--
 hmp.c                | 2 +-
 qapi/block-core.json | 9 +++++++--
 qmp-commands.hx      | 2 +-
 4 files changed, 14 insertions(+), 6 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index 97041b4..591f163 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -3783,10 +3783,13 @@ out:
     aio_context_release(aio_context);
 }
 
-void qmp_block_job_pause(const char *device, Error **errp)
+void qmp_block_job_pause(bool has_id, const char *id,
+                         bool has_device, const char *device, Error **errp)
 {
     AioContext *aio_context;
-    BlockJob *job = find_block_job(NULL, device, &aio_context, errp);
+    BlockJob *job = find_block_job(has_id ? id : NULL,
+                                   has_device ? device : NULL,
+                                   &aio_context, errp);
 
     if (!job || job->user_paused) {
         return;
diff --git a/hmp.c b/hmp.c
index adeb4de..783c3af 100644
--- a/hmp.c
+++ b/hmp.c
@@ -1515,7 +1515,7 @@ void hmp_block_job_pause(Monitor *mon, const QDict *qdict)
     Error *error = NULL;
     const char *device = qdict_get_str(qdict, "device");
 
-    qmp_block_job_pause(device, &error);
+    qmp_block_job_pause(false, NULL, true, device, &error);
 
     hmp_handle_error(mon, &error);
 }
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 38dd3f2..0eaf531 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -1537,14 +1537,19 @@
 # the operation is actually paused.  Cancelling a paused job automatically
 # resumes it.
 #
-# @device: the device name
+# The job must be identified with the @id or @device parameters, but
+# only one of them must be set.
+#
+# @id: #optional the job identifier. (Since 2.7)
+#
+# @device: #optional the device name
 #
 # Returns: Nothing on success
 #          If no background operation is active on this device, DeviceNotActive
 #
 # Since: 1.3
 ##
-{ 'command': 'block-job-pause', 'data': { 'device': 'str' } }
+{ 'command': 'block-job-pause', 'data': { '*id': 'str', '*device': 'str' } }
 
 ##
 # @block-job-resume:
diff --git a/qmp-commands.hx b/qmp-commands.hx
index 6bf8c3d..571feb6 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -1295,7 +1295,7 @@ EQMP
     },
     {
         .name       = "block-job-pause",
-        .args_type  = "device:B",
+        .args_type  = "id:s?,device:B?",
         .mhandler.cmd_new = qmp_marshal_block_job_pause,
     },
     {
-- 
2.8.1

[Qemu-devel] [PATCH 13/15] blockjob: Add 'id' parameter to 'block-job-resume'

[Alberto Garcia]

This patch allows the 'block-job-resume' command to identify the
job by either its ID or its device name. The latter becomes now
optional since the ID alone is enough to identify the job.

The HMP 'block_job_resume' command remains unchanged.

Signed-off-by: Alberto Garcia <berto@igalia.com>
---
 blockdev.c           | 7 +++++--
 hmp.c                | 2 +-
 qapi/block-core.json | 7 ++++++-
 qmp-commands.hx      | 2 +-
 4 files changed, 13 insertions(+), 5 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index 591f163..97e5fef 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -3801,10 +3801,13 @@ void qmp_block_job_pause(bool has_id, const char *id,
     aio_context_release(aio_context);
 }
 
-void qmp_block_job_resume(const char *device, Error **errp)
+void qmp_block_job_resume(bool has_id, const char *id,
+                          bool has_device, const char *device, Error **errp)
 {
     AioContext *aio_context;
-    BlockJob *job = find_block_job(NULL, device, &aio_context, errp);
+    BlockJob *job = find_block_job(has_id ? id : NULL,
+                                   has_device ? device : NULL,
+                                   &aio_context, errp);
 
     if (!job || !job->user_paused) {
         return;
diff --git a/hmp.c b/hmp.c
index 783c3af..dae1735 100644
--- a/hmp.c
+++ b/hmp.c
@@ -1525,7 +1525,7 @@ void hmp_block_job_resume(Monitor *mon, const QDict *qdict)
     Error *error = NULL;
     const char *device = qdict_get_str(qdict, "device");
 
-    qmp_block_job_resume(device, &error);
+    qmp_block_job_resume(false, NULL, true, device, &error);
 
     hmp_handle_error(mon, &error);
 }
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 0eaf531..0efa73d 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -1562,6 +1562,11 @@
 #
 # This command also clears the error status of the job.
 #
+# The job must be identified with the @id or @device parameters, but
+# only one of them must be set.
+#
+# @id: #optional the job identifier. (Since 2.7)
+#
 # @device: the device name
 #
 # Returns: Nothing on success
@@ -1569,7 +1574,7 @@
 #
 # Since: 1.3
 ##
-{ 'command': 'block-job-resume', 'data': { 'device': 'str' } }
+{ 'command': 'block-job-resume', 'data': { '*id': 'str', '*device': 'str' } }
 
 ##
 # @block-job-complete:
diff --git a/qmp-commands.hx b/qmp-commands.hx
index 571feb6..716d493 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -1300,7 +1300,7 @@ EQMP
     },
     {
         .name       = "block-job-resume",
-        .args_type  = "device:B",
+        .args_type  = "id:s?,device:B?",
         .mhandler.cmd_new = qmp_marshal_block_job_resume,
     },
     {
-- 
2.8.1

[Qemu-devel] [PATCH 14/15] blockjob: Add 'id' parameter to 'block-job-complete'

[Alberto Garcia]

This patch allows the 'block-job-complete' command to identify the
job by either its ID or its device name. The latter becomes now
optional since the ID alone is enough to identify the job.

The HMP 'block_job_complete' command remains unchanged.

Signed-off-by: Alberto Garcia <berto@igalia.com>
---
 blockdev.c           | 7 +++++--
 hmp.c                | 2 +-
 qapi/block-core.json | 7 ++++++-
 qmp-commands.hx      | 2 +-
 4 files changed, 13 insertions(+), 5 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index 97e5fef..63135d1 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -3819,10 +3819,13 @@ void qmp_block_job_resume(bool has_id, const char *id,
     aio_context_release(aio_context);
 }
 
-void qmp_block_job_complete(const char *device, Error **errp)
+void qmp_block_job_complete(bool has_id, const char *id,
+                            bool has_device, const char *device, Error **errp)
 {
     AioContext *aio_context;
-    BlockJob *job = find_block_job(NULL, device, &aio_context, errp);
+    BlockJob *job = find_block_job(has_id ? id : NULL,
+                                   has_device ? device : NULL,
+                                   &aio_context, errp);
 
     if (!job) {
         return;
diff --git a/hmp.c b/hmp.c
index dae1735..a2548f2 100644
--- a/hmp.c
+++ b/hmp.c
@@ -1535,7 +1535,7 @@ void hmp_block_job_complete(Monitor *mon, const QDict *qdict)
     Error *error = NULL;
     const char *device = qdict_get_str(qdict, "device");
 
-    qmp_block_job_complete(device, &error);
+    qmp_block_job_complete(false, NULL, true, device, &error);
 
     hmp_handle_error(mon, &error);
 }
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 0efa73d..f15e62f 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -1593,6 +1593,11 @@
 #
 # A cancelled or paused job cannot be completed.
 #
+# The job must be identified with the @id or @device parameters, but
+# only one of them must be set.
+#
+# @id: #optional the job identifier. (Since 2.7)
+#
 # @device: the device name
 #
 # Returns: Nothing on success
@@ -1600,7 +1605,7 @@
 #
 # Since: 1.3
 ##
-{ 'command': 'block-job-complete', 'data': { 'device': 'str' } }
+{ 'command': 'block-job-complete', 'data': { '*id': 'str', '*device': 'str' } }
 
 ##
 # @BlockdevDiscardOptions
diff --git a/qmp-commands.hx b/qmp-commands.hx
index 716d493..d0242dc 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -1305,7 +1305,7 @@ EQMP
     },
     {
         .name       = "block-job-complete",
-        .args_type  = "device:B",
+        .args_type  = "id:s?,device:B?",
         .mhandler.cmd_new = qmp_marshal_block_job_complete,
     },
     {
-- 
2.8.1

[Qemu-devel] [PATCH 15/15] blockjob: Add 'id' field to 'BlockJobInfo' and all BLOCK_JOB_* events

[Alberto Garcia]

Now that all jobs have an ID and all QMP commands that create and
operate on block jobs can specify one we can finally expose the ID in
all BLOCK_JOB_* events and the BlockJobInfo structure.

This modifies the output of several iotests, but now we have full
control of the job IDs so this patch updates the affected ones.

Signed-off-by: Alberto Garcia <berto@igalia.com>
---
 blockjob.c                 |  6 +++++-
 docs/qmp-events.txt        |  4 ++++
 qapi/block-core.json       | 18 ++++++++++++++++--
 tests/qemu-iotests/095     |  2 +-
 tests/qemu-iotests/095.out |  2 +-
 tests/qemu-iotests/124     |  3 ++-
 tests/qemu-iotests/141     |  6 +++++-
 tests/qemu-iotests/141.out | 14 +++++++-------
 tests/qemu-iotests/144     |  1 +
 tests/qemu-iotests/144.out |  4 ++--
 10 files changed, 44 insertions(+), 16 deletions(-)

diff --git a/blockjob.c b/blockjob.c
index dd0eb7f..0b7fe50 100644
--- a/blockjob.c
+++ b/blockjob.c
@@ -416,6 +416,7 @@ BlockJobInfo *block_job_query(BlockJob *job)
 {
     BlockJobInfo *info = g_new0(BlockJobInfo, 1);
     info->type      = g_strdup(BlockJobType_lookup[job->driver->job_type]);
+    info->id        = g_strdup(job->id);
     info->device    = g_strdup(job->device);
     info->len       = job->len;
     info->busy      = job->busy;
@@ -438,6 +439,7 @@ static void block_job_iostatus_set_err(BlockJob *job, int error)
 void block_job_event_cancelled(BlockJob *job)
 {
     qapi_event_send_block_job_cancelled(job->driver->job_type,
+                                        job->id,
                                         job->device,
                                         job->len,
                                         job->offset,
@@ -448,6 +450,7 @@ void block_job_event_cancelled(BlockJob *job)
 void block_job_event_completed(BlockJob *job, const char *msg)
 {
     qapi_event_send_block_job_completed(job->driver->job_type,
+                                        job->id,
                                         job->device,
                                         job->len,
                                         job->offset,
@@ -462,6 +465,7 @@ void block_job_event_ready(BlockJob *job)
     job->ready = true;
 
     qapi_event_send_block_job_ready(job->driver->job_type,
+                                    job->id,
                                     job->device,
                                     job->len,
                                     job->offset,
@@ -490,7 +494,7 @@ BlockErrorAction block_job_error_action(BlockJob *job, BlockdevOnError on_err,
     default:
         abort();
     }
-    qapi_event_send_block_job_error(job->device,
+    qapi_event_send_block_job_error(job->id, job->device,
                                     is_read ? IO_OPERATION_TYPE_READ :
                                     IO_OPERATION_TYPE_WRITE,
                                     action, &error_abort);
diff --git a/docs/qmp-events.txt b/docs/qmp-events.txt
index fa7574d..2b2af0f 100644
--- a/docs/qmp-events.txt
+++ b/docs/qmp-events.txt
@@ -92,6 +92,7 @@ Data:
 
 - "type":     Job type (json-string; "stream" for image streaming
                                      "commit" for block commit)
+- "id":       Job identifier (json-string)
 - "device":   Device name (json-string)
 - "len":      Maximum progress value (json-int)
 - "offset":   Current progress value (json-int)
@@ -116,6 +117,7 @@ Data:
 
 - "type":     Job type (json-string; "stream" for image streaming
                                      "commit" for block commit)
+- "id":       Job identifier (json-string)
 - "device":   Device name (json-string)
 - "len":      Maximum progress value (json-int)
 - "offset":   Current progress value (json-int)
@@ -143,6 +145,7 @@ Emitted when a block job encounters an error.
 
 Data:
 
+- "id":     job identifier (json-string)
 - "device": device name (json-string)
 - "operation": I/O operation (json-string, "read" or "write")
 - "action": action that has been taken, it's one of the following (json-string):
@@ -167,6 +170,7 @@ Data:
 
 - "type":     Job type (json-string; "stream" for image streaming
                                      "commit" for block commit)
+- "id":       Job identifier (json-string)
 - "device":   Device name (json-string)
 - "len":      Maximum progress value (json-int)
 - "offset":   Current progress value (json-int)
diff --git a/qapi/block-core.json b/qapi/block-core.json
index f15e62f..b9544d1 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -713,6 +713,8 @@
 #
 # @type: the job type ('stream' for image streaming)
 #
+# @id: the job identifier
+#
 # @device: the block device name
 #
 # @len: the maximum progress value
@@ -734,7 +736,7 @@
 # Since: 1.1
 ##
 { 'struct': 'BlockJobInfo',
-  'data': {'type': 'str', 'device': 'str', 'len': 'int',
+  'data': {'type': 'str', 'id': 'str', 'device': 'str', 'len': 'int',
            'offset': 'int', 'busy': 'bool', 'paused': 'bool', 'speed': 'int',
            'io-status': 'BlockDeviceIoStatus', 'ready': 'bool'} }
 
@@ -2443,6 +2445,8 @@
 #
 # @type: job type
 #
+# @id: the job identifier
+#
 # @device: device name
 #
 # @len: maximum progress value
@@ -2461,6 +2465,7 @@
 ##
 { 'event': 'BLOCK_JOB_COMPLETED',
   'data': { 'type'  : 'BlockJobType',
+            'id'    : 'str',
             'device': 'str',
             'len'   : 'int',
             'offset': 'int',
@@ -2474,6 +2479,8 @@
 #
 # @type: job type
 #
+# @id: the job identifier
+#
 # @device: device name
 #
 # @len: maximum progress value
@@ -2487,6 +2494,7 @@
 ##
 { 'event': 'BLOCK_JOB_CANCELLED',
   'data': { 'type'  : 'BlockJobType',
+            'id'    : 'str',
             'device': 'str',
             'len'   : 'int',
             'offset': 'int',
@@ -2497,6 +2505,8 @@
 #
 # Emitted when a block job encounters an error
 #
+# @id: the job identifier
+#
 # @device: device name
 #
 # @operation: I/O operation
@@ -2506,7 +2516,8 @@
 # Since: 1.3
 ##
 { 'event': 'BLOCK_JOB_ERROR',
-  'data': { 'device'   : 'str',
+  'data': { 'id'       : 'str',
+            'device'   : 'str',
             'operation': 'IoOperationType',
             'action'   : 'BlockErrorAction' } }
 
@@ -2517,6 +2528,8 @@
 #
 # @type: job type
 #
+# @id: the job identifier
+#
 # @device: device name
 #
 # @len: maximum progress value
@@ -2533,6 +2546,7 @@
 ##
 { 'event': 'BLOCK_JOB_READY',
   'data': { 'type'  : 'BlockJobType',
+            'id'    : 'str',
             'device': 'str',
             'len'   : 'int',
             'offset': 'int',
diff --git a/tests/qemu-iotests/095 b/tests/qemu-iotests/095
index 030adb2..34617a7 100755
--- a/tests/qemu-iotests/095
+++ b/tests/qemu-iotests/095
@@ -71,7 +71,7 @@ h=$QEMU_HANDLE
 _send_qemu_cmd $h "{ 'execute': 'qmp_capabilities' }" "return"
 
 _send_qemu_cmd $h "{ 'execute': 'block-commit',
-                                 'arguments': { 'device': 'test',
+                                 'arguments': { 'job-id': 'testjob', 'device': 'test',
                                  'top': '"${TEST_IMG}.snp1"' } }" "BLOCK_JOB_COMPLETED"
 
 _cleanup_qemu
diff --git a/tests/qemu-iotests/095.out b/tests/qemu-iotests/095.out
index 73875ca..c146c72 100644
--- a/tests/qemu-iotests/095.out
+++ b/tests/qemu-iotests/095.out
@@ -12,7 +12,7 @@ virtual size: 5.0M (5242880 bytes)
 
 {"return": {}}
 {"return": {}}
-{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "BLOCK_JOB_COMPLETED", "data": {"device": "test", "len": 104857600, "offset": 104857600, "speed": 0, "type": "commit"}}
+{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "BLOCK_JOB_COMPLETED", "data": {"device": "test", "len": 104857600, "offset": 104857600, "speed": 0, "type": "commit", "id": "testjob"}}
 
 === Base image info after commit and resize ===
 image: TEST_DIR/t.IMGFMT.base
diff --git a/tests/qemu-iotests/124 b/tests/qemu-iotests/124
index de7cdbe..89d74cb 100644
--- a/tests/qemu-iotests/124
+++ b/tests/qemu-iotests/124
@@ -50,7 +50,7 @@ def transaction_bitmap_clear(node, name, **kwargs):
 
 def transaction_drive_backup(device, target, **kwargs):
     return transaction_action('drive-backup', device=device, target=target,
-                              **kwargs)
+                              job_id=device, **kwargs)
 
 
 class Bitmap:
@@ -479,6 +479,7 @@ class TestIncrementalBackup(TestIncrementalBackupBase):
         self.assertFalse(self.wait_qmp_backup(drive1['id']))
         error = self.vm.event_wait('BLOCK_JOB_ERROR')
         self.assert_qmp(error, 'data', {'device': drive1['id'],
+                                        'id': drive1['id'],
                                         'action': 'report',
                                         'operation': 'read'})
         self.assertFalse(self.vm.get_qmp_events(wait=False))
diff --git a/tests/qemu-iotests/141 b/tests/qemu-iotests/141
index b2617e5..85d9d9d 100755
--- a/tests/qemu-iotests/141
+++ b/tests/qemu-iotests/141
@@ -102,6 +102,7 @@ echo
 test_blockjob \
     "{'execute': 'drive-backup',
       'arguments': {'device': 'drv0',
+                    'job-id': 'backup1',
                     'target': '$TEST_DIR/o.$IMGFMT',
                     'format': '$IMGFMT',
                     'sync': 'none'}}" \
@@ -118,6 +119,7 @@ echo
 test_blockjob \
     "{'execute': 'drive-mirror',
       'arguments': {'device': 'drv0',
+                    'job-id': 'mirror1',
                     'target': '$TEST_DIR/o.$IMGFMT',
                     'format': '$IMGFMT',
                     'sync': 'none'}}" \
@@ -134,7 +136,7 @@ echo
 
 test_blockjob \
     "{'execute': 'block-commit',
-      'arguments': {'device': 'drv0'}}" \
+      'arguments': {'device': 'drv0', 'job-id': 'commit1'}}" \
     'BLOCK_JOB_READY' \
     'BLOCK_JOB_COMPLETED'
 
@@ -151,6 +153,7 @@ $QEMU_IO -c 'write 0 1M' "$TEST_DIR/m.$IMGFMT" | _filter_qemu_io
 test_blockjob \
     "{'execute': 'block-commit',
       'arguments': {'device': 'drv0',
+                    'job-id': 'commit2',
                     'top':    '$TEST_DIR/m.$IMGFMT',
                     'speed':  1}}" \
     'return' \
@@ -173,6 +176,7 @@ $QEMU_IO -c 'write 0 1M' "$TEST_DIR/b.$IMGFMT" | _filter_qemu_io
 test_blockjob \
     "{'execute': 'block-stream',
       'arguments': {'device': 'drv0',
+                    'job-id': 'stream1',
                     'speed': 1}}" \
     'return' \
     'BLOCK_JOB_CANCELLED'
diff --git a/tests/qemu-iotests/141.out b/tests/qemu-iotests/141.out
index adceac1..36c72bc 100644
--- a/tests/qemu-iotests/141.out
+++ b/tests/qemu-iotests/141.out
@@ -11,7 +11,7 @@ Formatting 'TEST_DIR/o.IMGFMT', fmt=IMGFMT size=1048576 backing_file=TEST_DIR/t.
 {"return": {}}
 {"error": {"class": "GenericError", "desc": "Node 'drv0' is busy: block device is in use by block job: backup"}}
 {"return": {}}
-{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "BLOCK_JOB_CANCELLED", "data": {"device": "drv0", "len": 1048576, "offset": 0, "speed": 0, "type": "backup"}}
+{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "BLOCK_JOB_CANCELLED", "data": {"device": "drv0", "len": 1048576, "offset": 0, "speed": 0, "type": "backup", "id": "backup1"}}
 {"return": {}}
 
 === Testing drive-mirror ===
@@ -19,20 +19,20 @@ Formatting 'TEST_DIR/o.IMGFMT', fmt=IMGFMT size=1048576 backing_file=TEST_DIR/t.
 {"return": {}}
 Formatting 'TEST_DIR/o.IMGFMT', fmt=IMGFMT size=1048576 backing_file=TEST_DIR/t.IMGFMT backing_fmt=IMGFMT
 {"return": {}}
-{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "BLOCK_JOB_READY", "data": {"device": "drv0", "len": 0, "offset": 0, "speed": 0, "type": "mirror"}}
+{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "BLOCK_JOB_READY", "data": {"device": "drv0", "len": 0, "offset": 0, "speed": 0, "type": "mirror", "id": "mirror1"}}
 {"error": {"class": "GenericError", "desc": "Node 'drv0' is busy: block device is in use by block job: mirror"}}
 {"return": {}}
-{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "BLOCK_JOB_COMPLETED", "data": {"device": "drv0", "len": 0, "offset": 0, "speed": 0, "type": "mirror"}}
+{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "BLOCK_JOB_COMPLETED", "data": {"device": "drv0", "len": 0, "offset": 0, "speed": 0, "type": "mirror", "id": "mirror1"}}
 {"return": {}}
 
 === Testing active block-commit ===
 
 {"return": {}}
 {"return": {}}
-{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "BLOCK_JOB_READY", "data": {"device": "drv0", "len": 0, "offset": 0, "speed": 0, "type": "commit"}}
+{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "BLOCK_JOB_READY", "data": {"device": "drv0", "len": 0, "offset": 0, "speed": 0, "type": "commit", "id": "commit1"}}
 {"error": {"class": "GenericError", "desc": "Node 'drv0' is busy: block device is in use by block job: commit"}}
 {"return": {}}
-{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "BLOCK_JOB_COMPLETED", "data": {"device": "drv0", "len": 0, "offset": 0, "speed": 0, "type": "commit"}}
+{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "BLOCK_JOB_COMPLETED", "data": {"device": "drv0", "len": 0, "offset": 0, "speed": 0, "type": "commit", "id": "commit1"}}
 {"return": {}}
 
 === Testing non-active block-commit ===
@@ -43,7 +43,7 @@ wrote 1048576/1048576 bytes at offset 0
 {"return": {}}
 {"error": {"class": "GenericError", "desc": "Node 'drv0' is busy: block device is in use by block job: commit"}}
 {"return": {}}
-{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "BLOCK_JOB_CANCELLED", "data": {"device": "drv0", "len": 1048576, "offset": 524288, "speed": 1, "type": "commit"}}
+{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "BLOCK_JOB_CANCELLED", "data": {"device": "drv0", "len": 1048576, "offset": 524288, "speed": 1, "type": "commit", "id": "commit2"}}
 {"return": {}}
 
 === Testing block-stream ===
@@ -54,6 +54,6 @@ wrote 1048576/1048576 bytes at offset 0
 {"return": {}}
 {"error": {"class": "GenericError", "desc": "Node 'drv0' is busy: block device is in use by block job: stream"}}
 {"return": {}}
-{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "BLOCK_JOB_CANCELLED", "data": {"device": "drv0", "len": 1048576, "offset": 524288, "speed": 1, "type": "stream"}}
+{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "BLOCK_JOB_CANCELLED", "data": {"device": "drv0", "len": 1048576, "offset": 524288, "speed": 1, "type": "stream", "id": "stream1"}}
 {"return": {}}
 *** done
diff --git a/tests/qemu-iotests/144 b/tests/qemu-iotests/144
index 00de3c3..09cebcc 100755
--- a/tests/qemu-iotests/144
+++ b/tests/qemu-iotests/144
@@ -85,6 +85,7 @@ echo
 # Block commit on active layer, push the new overlay into base
 _send_qemu_cmd $h "{ 'execute': 'block-commit',
                                 'arguments': {
+                                                 'job-id': 'commit1',
                                                  'device': 'virtio0'
                                               }
                     }" "READY"
diff --git a/tests/qemu-iotests/144.out b/tests/qemu-iotests/144.out
index 410d741..3208c1f 100644
--- a/tests/qemu-iotests/144.out
+++ b/tests/qemu-iotests/144.out
@@ -13,9 +13,9 @@ Formatting 'TEST_DIR/tmp.qcow2', fmt=qcow2 size=536870912 backing_file=TEST_DIR/
 === Performing block-commit on active layer ===
 
 {"return": {}}
-{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "BLOCK_JOB_READY", "data": {"device": "virtio0", "len": 0, "offset": 0, "speed": 0, "type": "commit"}}
+{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "BLOCK_JOB_READY", "data": {"device": "virtio0", "len": 0, "offset": 0, "speed": 0, "type": "commit", "id": "commit1"}}
 {"return": {}}
-{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "BLOCK_JOB_COMPLETED", "data": {"device": "virtio0", "len": 0, "offset": 0, "speed": 0, "type": "commit"}}
+{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "BLOCK_JOB_COMPLETED", "data": {"device": "virtio0", "len": 0, "offset": 0, "speed": 0, "type": "commit", "id": "commit1"}}
 
 === Performing Live Snapshot 2 ===
 
-- 
2.8.1

Re: [Qemu-devel] [PATCH 01/15] stream: Fix prototype of stream_start()

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 656 bytes --]

On 09.06.2016 10:20, Alberto Garcia wrote:
> 'stream-start' has a parameter called 'backing-file', which is the
> string to be written to bs->backing when the job finishes.
> 
> In the stream_start() implementation it is called 'backing_file_str',
> but it the prototype in the header file it is called 'base_id'.
> 
> This patch fixes it so the name is the same in both cases and is
> consistent with other cases (like commit_start()).
> 
> Signed-off-by: Alberto Garcia <berto@igalia.com>
> ---
>  include/block/block_int.h | 11 ++++++-----
>  1 file changed, 6 insertions(+), 5 deletions(-)

Reviewed-by: Max Reitz <mreitz@redhat.com>


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [PATCH 02/15] blockjob: Decouple the ID from the device name in the BlockJob struct

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 952 bytes --]

On 09.06.2016 10:20, Alberto Garcia wrote:
> Block jobs are identified by the name of the BlockBackend of the BDS
> where the job was started.
> 
> We want block jobs to have unique, arbitrary identifiers that are not
> tied to a block device, so this patch decouples the ID from the device
> name in the BlockJob structure.
> 
> The ID is generated automatically for the moment, in later patches
> we'll allow the user to set it.
> 
> Signed-off-by: Alberto Garcia <berto@igalia.com>
> ---
>  blockjob.c               | 15 +++++++++------
>  include/block/blockjob.h | 12 ++++++++----
>  include/qemu/id.h        |  1 +
>  util/id.c                |  1 +
>  4 files changed, 19 insertions(+), 10 deletions(-)

I suppose this patch tries to "silently" (i.e. not visibly) introduce
this new ID for now? If so, there is one instance of job->id left that
should probably be changed to job->device (in block_job_complete()).

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [PATCH 03/15] blockjob: Add block_job_get()

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 1215 bytes --]

On 09.06.2016 10:20, Alberto Garcia wrote:
> Currently the way to look for a specific block job is to iterate the
> list manually using block_job_next().
> 
> Since we want to be able to identify a job primarily by its ID it
> makes sense to have a function that does just that.
> 
> Signed-off-by: Alberto Garcia <berto@igalia.com>
> ---
>  blockjob.c               | 13 +++++++++++++
>  include/block/blockjob.h | 10 ++++++++++
>  2 files changed, 23 insertions(+)

Reviewed-by: Max Reitz <mreitz@redhat.com>

Just to throw out a suggestion, I'm not sure how useful it will be after
the rest of the series:

Would it make sense to prevent name clashes between block job IDs and
device IDs (i.e. BlockBackend names)? If we did that, this function
could be used to identify a block job both based on its name and its device.

Just a suggestion, though, I don't think it will be necessary. Legacy
applications will always create jobs with auto-generated IDs that cannot
clash with BB names anyway. If we require applications that do name
their block jobs manually to always use this ID when specifying a block
job, everything will be fine even if block job and BB names do clash.

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [PATCH 04/15] block: Simplify find_block_job() and make it accept a job ID

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 4115 bytes --]

On 09.06.2016 10:20, Alberto Garcia wrote:
> find_block_job() looks for a block backend with a specified name,
> checks whether it has a block job and acquires its AioContext. This
> patch uses block_job_next() and iterate directly over the block jobs.
> 
> In addition to that we want to identify jobs primarily by their ID, so
> this patch updates find_block_job() to allow IDs too. Only one of ID
> and device name can be specified when looking for a block job.
> 
> Signed-off-by: Alberto Garcia <berto@igalia.com>
> ---
>  blockdev.c | 66 +++++++++++++++++++++++++++++++++-----------------------------
>  1 file changed, 35 insertions(+), 31 deletions(-)
> 
> diff --git a/blockdev.c b/blockdev.c
> index 52ec4ae..bd0d5a1 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -3689,48 +3689,52 @@ void qmp_blockdev_mirror(const char *device, const char *target,
>      aio_context_release(aio_context);
>  }
>  
> -/* Get the block job for a given device name and acquire its AioContext */
> -static BlockJob *find_block_job(const char *device, AioContext **aio_context,
> -                                Error **errp)
> +/* Get a block job using its ID or device name and acquire its AioContext */

I think it would be good if this comment could mention that only either
of ID and device name may be specified. Not strictly necessary, though,
since that check is done right at the start of the function.

> +static BlockJob *find_block_job(const char *id, const char *device,
> +                                AioContext **aio_context, Error **errp)
>  {
> -    BlockBackend *blk;
> -    BlockDriverState *bs;
> +    BlockJob *job = NULL;
>  
>      *aio_context = NULL;
>  
> -    blk = blk_by_name(device);
> -    if (!blk) {
> -        goto notfound;
> +    if ((id && device) || (!id && !device)) {
> +        error_setg(errp, "Only one of ID or device name "

Maybe s/Only/Exactly/. Or it could just be an assertion.

> +                   "must be specified when looking for a block job");
> +        return NULL;
>      }
>  
> -    *aio_context = blk_get_aio_context(blk);
> -    aio_context_acquire(*aio_context);
> -
> -    if (!blk_is_available(blk)) {
> -        goto notfound;
> +    if (id) {
> +        job = block_job_get(id);
> +    } else {
> +        BlockJob *j;
> +        for (j = block_job_next(NULL); j; j = block_job_next(j)) {
> +            if (!strcmp(device, j->device)) {
> +                if (job) {
> +                    /* This scenario is currently not possible, but it
> +                     * could theoretically happen in the future. */
> +                    error_setg(errp, "More than one job on device '%s', "
> +                               "use the job ID instead", device);
> +                    return NULL;
> +                }
> +                job = j;
> +            }
> +        }
>      }
> -    bs = blk_bs(blk);
>  
> -    if (!bs->job) {
> -        goto notfound;
> +    if (job) {
> +        *aio_context = blk_get_aio_context(job->blk);
> +        aio_context_acquire(*aio_context);
> +    } else {
> +        error_setg(errp, "Block job '%s' not found", id ? id : device);

This changes the error class if device is set. Not sure if that is bad,
but keeping the old behavior should be simple (unless you're sure it's
fine).

Also, but this is a personal opinion, I'd make the error path more
explicit, i.e.:

if (!job) {
    /* ... */
    return NULL;
}

*aio_context = /* ... */
/* ...*/

>      }
>  
> -    return bs->job;
> -
> -notfound:
> -    error_set(errp, ERROR_CLASS_DEVICE_NOT_ACTIVE,
> -              "No active block job on device '%s'", device);
> -    if (*aio_context) {
> -        aio_context_release(*aio_context);
> -        *aio_context = NULL;
> -    }
> -    return NULL;
> +    return job;
>  }
>  

What's keeping me from giving an R-b is that I don't know whether
changing the error class will be fine. If you say it is, OK then. But
just keeping the old behavior should be simple enough, too.

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [PATCH 05/15] blockjob: Add 'job_id' parameter to block_job_create()

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 714 bytes --]

On 09.06.2016 10:20, Alberto Garcia wrote:
> Job IDs are generated automatically when a new job is created. This
> patch adds a new 'job_id' parameter to let the caller provide one
> instead. In this case the ID is verified to be unique and well-formed.
> 
> Signed-off-by: Alberto Garcia <berto@igalia.com>
> ---
>  block/backup.c            |  3 ++-
>  block/commit.c            |  2 +-
>  block/mirror.c            |  2 +-
>  block/stream.c            |  2 +-
>  blockjob.c                | 19 +++++++++++++++----
>  include/block/blockjob.h  |  8 +++++---
>  tests/test-blockjob-txn.c |  2 +-
>  7 files changed, 26 insertions(+), 12 deletions(-)

Reviewed-by: Max Reitz <mreitz@redhat.com>


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [PATCH 06/15] mirror: Add 'job-id' parameter to 'blockdev-mirror' and 'drive-mirror'

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 706 bytes --]

On 09.06.2016 10:20, Alberto Garcia wrote:
> This patch adds a new optional 'job-id' parameter to 'blockdev-mirror'
> and 'drive-mirror', allowing the user to specify the ID of the block
> job to be created.
> 
> The HMP 'drive_mirror' command remains unchanged.
> 
> Signed-off-by: Alberto Garcia <berto@igalia.com>
> ---
>  block/mirror.c            | 14 +++++++-------
>  blockdev.c                | 15 ++++++++-------
>  hmp.c                     |  2 +-
>  include/block/block_int.h |  6 ++++--
>  qapi/block-core.json      | 10 +++++++---
>  qmp-commands.hx           |  8 +++++---
>  6 files changed, 32 insertions(+), 23 deletions(-)

Reviewed-by: Max Reitz <mreitz@redhat.com>


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [PATCH 06/15] mirror: Add 'job-id' parameter to 'blockdev-mirror' and 'drive-mirror'

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 870 bytes --]

On 20.06.2016 19:33, Max Reitz wrote:
> On 09.06.2016 10:20, Alberto Garcia wrote:
>> This patch adds a new optional 'job-id' parameter to 'blockdev-mirror'
>> and 'drive-mirror', allowing the user to specify the ID of the block
>> job to be created.
>>
>> The HMP 'drive_mirror' command remains unchanged.
>>
>> Signed-off-by: Alberto Garcia <berto@igalia.com>
>> ---
>>  block/mirror.c            | 14 +++++++-------
>>  blockdev.c                | 15 ++++++++-------
>>  hmp.c                     |  2 +-
>>  include/block/block_int.h |  6 ++++--
>>  qapi/block-core.json      | 10 +++++++---
>>  qmp-commands.hx           |  8 +++++---
>>  6 files changed, 32 insertions(+), 23 deletions(-)
> 
> Reviewed-by: Max Reitz <mreitz@redhat.com>

Forgot to mention: The patch will need a rebase, but the necessary
changes are rather obvious.

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [PATCH 07/15] backup: Add 'job-id' parameter to 'blockdev-backup' and 'drive-backup'

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 730 bytes --]

On 09.06.2016 10:20, Alberto Garcia wrote:
> This patch adds a new optional 'job-id' parameter to 'blockdev-backup'
> and 'drive-backup', allowing the user to specify the ID of the block
> job to be created.
> 
> The HMP 'drive_backup' command remains unchanged.
> 
> Signed-off-by: Alberto Garcia <berto@igalia.com>
> ---
>  block/backup.c            |  8 ++++----
>  blockdev.c                | 43 ++++++++++++++++++++++++-------------------
>  hmp.c                     |  2 +-
>  include/block/block_int.h |  8 +++++---
>  qapi/block-core.json      | 10 +++++++---
>  qmp-commands.hx           |  8 +++++---
>  6 files changed, 46 insertions(+), 33 deletions(-)

Reviewed-by: Max Reitz <mreitz@redhat.com>


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [PATCH 08/15] stream: Add 'job-id' parameter to 'block-stream'

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 666 bytes --]

On 09.06.2016 10:20, Alberto Garcia wrote:
> This patch adds a new optional 'job-id' parameter to 'block-stream',
> allowing the user to specify the ID of the block job to be created.
> 
> The HMP 'block_stream' command remains unchanged.
> 
> Signed-off-by: Alberto Garcia <berto@igalia.com>
> ---
>  block/stream.c            | 12 ++++++------
>  blockdev.c                |  6 +++---
>  hmp.c                     |  2 +-
>  include/block/block_int.h | 10 ++++++----
>  qapi/block-core.json      |  7 +++++--
>  qmp-commands.hx           |  3 ++-
>  6 files changed, 23 insertions(+), 17 deletions(-)

Reviewed-by: Max Reitz <mreitz@redhat.com>


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [PATCH 09/15] stream: Add 'job-id' parameter to 'block-commit'

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 712 bytes --]

On 09.06.2016 10:20, Alberto Garcia wrote:
> This patch adds a new optional 'job-id' parameter to 'block-commit',
> allowing the user to specify the ID of the block job to be created.
> 
> Signed-off-by: Alberto Garcia <berto@igalia.com>
> ---
>  block/commit.c            |  7 ++++---
>  block/mirror.c            |  6 +++---
>  blockdev.c                |  9 +++++----
>  include/block/block_int.h | 16 ++++++++++------
>  qapi/block-core.json      |  4 +++-
>  qemu-img.c                |  2 +-
>  qmp-commands.hx           |  3 ++-
>  7 files changed, 28 insertions(+), 19 deletions(-)

With the commit message fixed to read s/^stream/commit/:

Reviewed-by: Max Reitz <mreitz@redhat.com>


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [PATCH 04/15] block: Simplify find_block_job() and make it accept a job ID

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 1711 bytes --]

On 20.06.2016 18:40, Max Reitz wrote:
> On 09.06.2016 10:20, Alberto Garcia wrote:
>> find_block_job() looks for a block backend with a specified name,
>> checks whether it has a block job and acquires its AioContext. This
>> patch uses block_job_next() and iterate directly over the block jobs.
>>
>> In addition to that we want to identify jobs primarily by their ID, so
>> this patch updates find_block_job() to allow IDs too. Only one of ID
>> and device name can be specified when looking for a block job.
>>
>> Signed-off-by: Alberto Garcia <berto@igalia.com>
>> ---
>>  blockdev.c | 66 +++++++++++++++++++++++++++++++++-----------------------------
>>  1 file changed, 35 insertions(+), 31 deletions(-)
>>
>> diff --git a/blockdev.c b/blockdev.c
>> index 52ec4ae..bd0d5a1 100644
>> --- a/blockdev.c
>> +++ b/blockdev.c
>> @@ -3689,48 +3689,52 @@ void qmp_blockdev_mirror(const char *device, const char *target,

[...]

>> +static BlockJob *find_block_job(const char *id, const char *device,
>> +                                AioContext **aio_context, Error **errp)
>>  {
>> -    BlockBackend *blk;
>> -    BlockDriverState *bs;
>> +    BlockJob *job = NULL;
>>  
>>      *aio_context = NULL;
>>  
>> -    blk = blk_by_name(device);
>> -    if (!blk) {
>> -        goto notfound;
>> +    if ((id && device) || (!id && !device)) {
>> +        error_setg(errp, "Only one of ID or device name "
> 
> Maybe s/Only/Exactly/. Or it could just be an assertion.

Message from reviewing-patch-10-future-me: No, it should not be an
assertion.

Max

>> +                   "must be specified when looking for a block job");
>> +        return NULL;
>>      }
>> 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [PATCH 10/15] blockjob: Add 'id' parameter to 'block-job-set-speed'

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 613 bytes --]

On 09.06.2016 10:20, Alberto Garcia wrote:
> This patch allows the 'block-job-set-speed' command to identify the
> job by either its ID or its device name. The latter becomes now
> optional since the ID alone is enough to identify the job.
> 
> The HMP 'block_job_set_speed' command remains unchanged.
> 
> Signed-off-by: Alberto Garcia <berto@igalia.com>
> ---
>  blockdev.c           | 7 +++++--
>  hmp.c                | 2 +-
>  qapi/block-core.json | 9 +++++++--
>  qmp-commands.hx      | 2 +-
>  4 files changed, 14 insertions(+), 6 deletions(-)

Reviewed-by: Max Reitz <mreitz@redhat.com>


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [PATCH 11/15] blockjob: Add 'id' parameter to 'block-job-cancel'

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 612 bytes --]

On 09.06.2016 10:20, Alberto Garcia wrote:
> This patch allows the 'block-job-cancel' command to identify the
> job by either its ID or its device name. The latter becomes now
> optional since the ID alone is enough to identify the job.
> 
> The HMP 'block_job_cancel' command remains unchanged.
> 
> Signed-off-by: Alberto Garcia <berto@igalia.com>
> ---
>  blockdev.c           |  7 +++++--
>  hmp.c                |  2 +-
>  qapi/block-core.json | 10 ++++++++--
>  qmp-commands.hx      |  2 +-
>  4 files changed, 15 insertions(+), 6 deletions(-)

Reviewed-by: Max Reitz <mreitz@redhat.com>


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [PATCH 12/15] blockjob: Add 'id' parameter to 'block-job-pause'

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 605 bytes --]

On 09.06.2016 10:20, Alberto Garcia wrote:
> This patch allows the 'block-job-pause' command to identify the
> job by either its ID or its device name. The latter becomes now
> optional since the ID alone is enough to identify the job.
> 
> The HMP 'block_job_pause' command remains unchanged.
> 
> Signed-off-by: Alberto Garcia <berto@igalia.com>
> ---
>  blockdev.c           | 7 +++++--
>  hmp.c                | 2 +-
>  qapi/block-core.json | 9 +++++++--
>  qmp-commands.hx      | 2 +-
>  4 files changed, 14 insertions(+), 6 deletions(-)

Reviewed-by: Max Reitz <mreitz@redhat.com>


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [PATCH 13/15] blockjob: Add 'id' parameter to 'block-job-resume'

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 605 bytes --]

On 09.06.2016 10:20, Alberto Garcia wrote:
> This patch allows the 'block-job-resume' command to identify the
> job by either its ID or its device name. The latter becomes now
> optional since the ID alone is enough to identify the job.
> 
> The HMP 'block_job_resume' command remains unchanged.
> 
> Signed-off-by: Alberto Garcia <berto@igalia.com>
> ---
>  blockdev.c           | 7 +++++--
>  hmp.c                | 2 +-
>  qapi/block-core.json | 7 ++++++-
>  qmp-commands.hx      | 2 +-
>  4 files changed, 13 insertions(+), 5 deletions(-)

Reviewed-by: Max Reitz <mreitz@redhat.com>


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [PATCH 14/15] blockjob: Add 'id' parameter to 'block-job-complete'

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 609 bytes --]

On 09.06.2016 10:20, Alberto Garcia wrote:
> This patch allows the 'block-job-complete' command to identify the
> job by either its ID or its device name. The latter becomes now
> optional since the ID alone is enough to identify the job.
> 
> The HMP 'block_job_complete' command remains unchanged.
> 
> Signed-off-by: Alberto Garcia <berto@igalia.com>
> ---
>  blockdev.c           | 7 +++++--
>  hmp.c                | 2 +-
>  qapi/block-core.json | 7 ++++++-
>  qmp-commands.hx      | 2 +-
>  4 files changed, 13 insertions(+), 5 deletions(-)

Reviewed-by: Max Reitz <mreitz@redhat.com>


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [PATCH 03/15] blockjob: Add block_job_get()

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1900 bytes --]

On 06/20/2016 10:24 AM, Max Reitz wrote:
> On 09.06.2016 10:20, Alberto Garcia wrote:
>> Currently the way to look for a specific block job is to iterate the
>> list manually using block_job_next().
>>
>> Since we want to be able to identify a job primarily by its ID it
>> makes sense to have a function that does just that.
>>
>> Signed-off-by: Alberto Garcia <berto@igalia.com>
>> ---
>>  blockjob.c               | 13 +++++++++++++
>>  include/block/blockjob.h | 10 ++++++++++
>>  2 files changed, 23 insertions(+)
> 
> Reviewed-by: Max Reitz <mreitz@redhat.com>
> 
> Just to throw out a suggestion, I'm not sure how useful it will be after
> the rest of the series:
> 
> Would it make sense to prevent name clashes between block job IDs and
> device IDs (i.e. BlockBackend names)? If we did that, this function
> could be used to identify a block job both based on its name and its device.

Adding the restriction now, and then relaxing it later, is easier than
keeping it relaxed and wishing we could tighten it later.

> 
> Just a suggestion, though, I don't think it will be necessary. Legacy
> applications will always create jobs with auto-generated IDs that cannot
> clash with BB names anyway. If we require applications that do name
> their block jobs manually to always use this ID when specifying a block
> job, everything will be fine even if block job and BB names do clash.

But I think you're right here - legacy users will be just fine (at most
one job per device, never using the id but also never clashing an id
with device names), and new users should just always assign a job id and
use that rather than relying on device names.  So I don't think we need
the extra restriction of merging device and job id namespaces into one.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH 04/15] block: Simplify find_block_job() and make it accept a job ID

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 2456 bytes --]

On 06/09/2016 02:20 AM, Alberto Garcia wrote:
> find_block_job() looks for a block backend with a specified name,
> checks whether it has a block job and acquires its AioContext. This
> patch uses block_job_next() and iterate directly over the block jobs.
> 
> In addition to that we want to identify jobs primarily by their ID, so
> this patch updates find_block_job() to allow IDs too. Only one of ID
> and device name can be specified when looking for a block job.
> 
> Signed-off-by: Alberto Garcia <berto@igalia.com>
> ---
>  blockdev.c | 66 +++++++++++++++++++++++++++++++++-----------------------------
>  1 file changed, 35 insertions(+), 31 deletions(-)
> 
> diff --git a/blockdev.c b/blockdev.c
> index 52ec4ae..bd0d5a1 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -3689,48 +3689,52 @@ void qmp_blockdev_mirror(const char *device, const char *target,
>      aio_context_release(aio_context);
>  }
>  
> -/* Get the block job for a given device name and acquire its AioContext */
> -static BlockJob *find_block_job(const char *device, AioContext **aio_context,
> -                                Error **errp)
> +/* Get a block job using its ID or device name and acquire its AioContext */
> +static BlockJob *find_block_job(const char *id, const char *device,
> +                                AioContext **aio_context, Error **errp)

Can this signature just be const char *id_or_device, rather than two
parameters,...

>  {
> -    BlockBackend *blk;
> -    BlockDriverState *bs;
> +    BlockJob *job = NULL;
>  
>      *aio_context = NULL;
>  
> -    blk = blk_by_name(device);
> -    if (!blk) {
> -        goto notfound;
> +    if ((id && device) || (!id && !device)) {
> +        error_setg(errp, "Only one of ID or device name "
> +                   "must be specified when looking for a block job");
> +        return NULL;
>      }

If you keep this check, you could simplify it to:

if (!id == !device) {

...at which point you don't need this check,...

>  
> -    *aio_context = blk_get_aio_context(blk);
> -    aio_context_acquire(*aio_context);
> -
> -    if (!blk_is_available(blk)) {
> -        goto notfound;
> +    if (id) {
> +        job = block_job_get(id);
> +    } else {

...and this would become:

job = block_job_get(id);
if (!job) {


-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH 06/15] mirror: Add 'job-id' parameter to 'blockdev-mirror' and 'drive-mirror'

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 493 bytes --]

On 06/09/2016 02:20 AM, Alberto Garcia wrote:
> This patch adds a new optional 'job-id' parameter to 'blockdev-mirror'
> and 'drive-mirror', allowing the user to specify the ID of the block
> job to be created.
> 
> The HMP 'drive_mirror' command remains unchanged.
> 
> Signed-off-by: Alberto Garcia <berto@igalia.com>
> ---

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH 03/15] blockjob: Add block_job_get()

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 2114 bytes --]

On 20.06.2016 20:48, Eric Blake wrote:
> On 06/20/2016 10:24 AM, Max Reitz wrote:
>> On 09.06.2016 10:20, Alberto Garcia wrote:
>>> Currently the way to look for a specific block job is to iterate the
>>> list manually using block_job_next().
>>>
>>> Since we want to be able to identify a job primarily by its ID it
>>> makes sense to have a function that does just that.
>>>
>>> Signed-off-by: Alberto Garcia <berto@igalia.com>
>>> ---
>>>  blockjob.c               | 13 +++++++++++++
>>>  include/block/blockjob.h | 10 ++++++++++
>>>  2 files changed, 23 insertions(+)
>>
>> Reviewed-by: Max Reitz <mreitz@redhat.com>
>>
>> Just to throw out a suggestion, I'm not sure how useful it will be after
>> the rest of the series:
>>
>> Would it make sense to prevent name clashes between block job IDs and
>> device IDs (i.e. BlockBackend names)? If we did that, this function
>> could be used to identify a block job both based on its name and its device.
> 
> Adding the restriction now, and then relaxing it later, is easier than
> keeping it relaxed and wishing we could tighten it later.
> 
>>
>> Just a suggestion, though, I don't think it will be necessary. Legacy
>> applications will always create jobs with auto-generated IDs that cannot
>> clash with BB names anyway. If we require applications that do name
>> their block jobs manually to always use this ID when specifying a block
>> job, everything will be fine even if block job and BB names do clash.
> 
> But I think you're right here - legacy users will be just fine (at most
> one job per device, never using the id but also never clashing an id
> with device names), and new users should just always assign a job id and
> use that rather than relying on device names.  So I don't think we need
> the extra restriction of merging device and job id namespaces into one.

Yes, also, after having reviewed (most of) the rest of the series, I've
seen that Berto has added the ID as a separate parameter of all block
job commands. Therefore, there really is no need to restrict the job ID
namespace.

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [PATCH 15/15] blockjob: Add 'id' field to 'BlockJobInfo' and all BLOCK_JOB_* events

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 1293 bytes --]

On 09.06.2016 10:20, Alberto Garcia wrote:
> Now that all jobs have an ID and all QMP commands that create and
> operate on block jobs can specify one we can finally expose the ID in
> all BLOCK_JOB_* events and the BlockJobInfo structure.
> 
> This modifies the output of several iotests, but now we have full
> control of the job IDs so this patch updates the affected ones.
> 
> Signed-off-by: Alberto Garcia <berto@igalia.com>
> ---
>  blockjob.c                 |  6 +++++-
>  docs/qmp-events.txt        |  4 ++++
>  qapi/block-core.json       | 18 ++++++++++++++++--
>  tests/qemu-iotests/095     |  2 +-
>  tests/qemu-iotests/095.out |  2 +-
>  tests/qemu-iotests/124     |  3 ++-
>  tests/qemu-iotests/141     |  6 +++++-
>  tests/qemu-iotests/141.out | 14 +++++++-------
>  tests/qemu-iotests/144     |  1 +
>  tests/qemu-iotests/144.out |  4 ++--
>  10 files changed, 44 insertions(+), 16 deletions(-)

Looks good, but in the meantime iotest 156 has been added which needs
the same treatment as the rest.

Also, I personally wouldn't mind making use of the IDs where trivial.
For example, you can just replace the "'device': 'virtio0'" by "'id':
'commit1'" in the block-job-complete invocation in 144. Optional, of
course, but would make sense.

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [PATCH 02/15] blockjob: Decouple the ID from the device name in the BlockJob struct

[Alberto Garcia]

On Mon 20 Jun 2016 06:17:30 PM CEST, Max Reitz wrote:

> I suppose this patch tries to "silently" (i.e. not visibly) introduce
> this new ID for now? If so, there is one instance of job->id left that
> should probably be changed to job->device (in block_job_complete()).

I decided to leave that one unchanged because it's an error message, it
doesn't have any other practical impact and it would need to be renamed
back to job->id anyway.

But now that you mentioned it I see that the error message actually says
"...the block job for device '%s'...", so I think I'd rather leave
job->id and change the error message instead.

Berto

Re: [Qemu-devel] [PATCH 03/15] blockjob: Add block_job_get()

[Alberto Garcia]

On Mon 20 Jun 2016 06:24:30 PM CEST, Max Reitz wrote:
>> Currently the way to look for a specific block job is to iterate the
>> list manually using block_job_next().
>> 
>> Since we want to be able to identify a job primarily by its ID it
>> makes sense to have a function that does just that.
>> 
>> Signed-off-by: Alberto Garcia <berto@igalia.com>
>> ---
>>  blockjob.c               | 13 +++++++++++++
>>  include/block/blockjob.h | 10 ++++++++++
>>  2 files changed, 23 insertions(+)
>
> Reviewed-by: Max Reitz <mreitz@redhat.com>
>
> Just to throw out a suggestion, I'm not sure how useful it will be
> after the rest of the series:
>
> Would it make sense to prevent name clashes between block job IDs and
> device IDs (i.e. BlockBackend names)? If we did that, this function
> could be used to identify a block job both based on its name and its
> device.

I considered that. Apart from the reasons that have been mentioned,
there's also the problem that the user doesn't have control over the
device name when creating jobs, so for example you could not start a new
job on a device with the same name as an existing job, or two jobs on
the same device.

These two cases are a bit far-fetched (the latter is anyway not
supported yet), but they hinted me that it's better to make the use of
the job ID explicit and leave the device name for older clients.

Berto

Re: [Qemu-devel] [PATCH 04/15] block: Simplify find_block_job() and make it accept a job ID

[Alberto Garcia]

On Mon 20 Jun 2016 08:53:08 PM CEST, Eric Blake wrote:
>> +static BlockJob *find_block_job(const char *id, const char *device,
>> +                                AioContext **aio_context, Error **errp)
>
> Can this signature just be const char *id_or_device, rather than two
> parameters,...

But what if there's a name clash?

Berto

Re: [Qemu-devel] [PATCH 04/15] block: Simplify find_block_job() and make it accept a job ID

[Alberto Garcia]

On Mon 20 Jun 2016 06:40:31 PM CEST, Max Reitz wrote:

>> +        error_setg(errp, "Block job '%s' not found", id ? id : device);
>
> This changes the error class if device is set. Not sure if that is
> bad, but keeping the old behavior should be simple (unless you're sure
> it's fine).

You're right, I'll fix it.

> Also, but this is a personal opinion, I'd make the error path more
> explicit, i.e.:
>
> if (!job) {
>     /* ... */
>     return NULL;
> }

Yeah, why not.

Berto

Re: [Qemu-devel] [PATCH 15/15] blockjob: Add 'id' field to 'BlockJobInfo' and all BLOCK_JOB_* events

[Alberto Garcia]

On Mon 20 Jun 2016 09:14:04 PM CEST, Max Reitz wrote:

> Also, I personally wouldn't mind making use of the IDs where trivial.
> For example, you can just replace the "'device': 'virtio0'" by "'id':
> 'commit1'" in the block-job-complete invocation in 144. Optional, of
> course, but would make sense.

My plan was to leave this patch with the minimum necessary changes and
then write additional tests for the job ID with all possible scenarios.

Berto

Re: [Qemu-devel] [PATCH 04/15] block: Simplify find_block_job() and make it accept a job ID

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1225 bytes --]

On 06/21/2016 06:27 AM, Alberto Garcia wrote:
> On Mon 20 Jun 2016 08:53:08 PM CEST, Eric Blake wrote:
>>> +static BlockJob *find_block_job(const char *id, const char *device,
>>> +                                AioContext **aio_context, Error **errp)
>>
>> Can this signature just be const char *id_or_device, rather than two
>> parameters,...
> 
> But what if there's a name clash?

All jobs have an id. And legacy users never set an id, so the id of
legacy jobs will always be generated (and you can tell by the #
character in the name that it was a generated id).  Basically, I'm
proposing a hierarchical lookup: If the id matches, then you use it.  If
the id doesn't match, then do a device lookup name.  New code that knows
to set the id should never pass anything but valid ids, so we only have
to worry about the case of new code trying to check the status of a job
that no longer exists and accidentally getting the status of an
unrelated job that happened to belong to a device with the same name -
but any new code shouldn't be that stupid as to use job ids that match
device ids.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH 04/15] block: Simplify find_block_job() and make it accept a job ID

[Alberto Garcia]

On Tue 21 Jun 2016 11:36:24 PM CEST, Eric Blake <eblake@redhat.com> wrote:
> On 06/21/2016 06:27 AM, Alberto Garcia wrote:
>> On Mon 20 Jun 2016 08:53:08 PM CEST, Eric Blake wrote:
>>>> +static BlockJob *find_block_job(const char *id, const char *device,
>>>> +                                AioContext **aio_context, Error **errp)
>>>
>>> Can this signature just be const char *id_or_device, rather than two
>>> parameters,...
>> 
>> But what if there's a name clash?
>
> All jobs have an id. And legacy users never set an id, so the id of
> legacy jobs will always be generated (and you can tell by the #
> character in the name that it was a generated id).  Basically, I'm
> proposing a hierarchical lookup: If the id matches, then you use it.  If
> the id doesn't match, then do a device lookup name.  New code that knows
> to set the id should never pass anything but valid ids, so we only have
> to worry about the case of new code trying to check the status of a job
> that no longer exists and accidentally getting the status of an
> unrelated job that happened to belong to a device with the same name -
> but any new code shouldn't be that stupid as to use job ids that match
> device ids.

Yeah, it *shouldn't*, but is it a good idea to leave this door open just
to have a slightly simpler internal API?

Berto