[Qemu-devel] [PATCH v4 00/11] Allow creating block jobs with a user-defined ID

[Qemu-devel] [PATCH v4 00/11] Allow creating block jobs with a user-defined ID

[Alberto Garcia]

Hi all,

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

The problem with this is that you cannot have block jobs on nodes
where there is no such name.

This series turns the 'id' field of the BlockJob structure into an
actual ID, it guarantees that is unique and always present and allows
the user to set it.

Berto

v4:
- Rebase on top of the current master
- Patch 4: Disallow null IDs in find_block_job().
- Patches 6-9: Clarify that if @job-id is missing the device name
  will be used as job ID.
- Patch 11: Capitalize the first word in the description of the
  'device' parameter.

v3: https://lists.gnu.org/archive/html/qemu-devel/2016-07/msg00121.html
- Instead of having separate 'id' and 'device' fields, use only the
  former. If the ID is not specified by the user it defaults to the
  device name to ensure backward compatibility.
- Patch 2:
  - Don't create a new field, simply update the description and the
    error messages. 
- Patch 4:
  - Now find_block_job() is simply a wrapper around block_job_get()
    that acquires the AioContext.
- Patch 5:
  - Allow setting the job ID but fall back to the device name if the
    user doesn't provide one. Fix the tests where the default ID was
    empty.
- Patches 10-14 [v2]:
  - These are no longer necessary, there's no need to add a new 'id'
    parameter to the 'block-job-*' QMP commands.
- Patch 10 [v3]:
  - Make img_commit() set a job ID
- Patch 11 [v3]:
  - Update the documentation of the QMP API to explain that the
   'device' field is now an actual ID.

v2: https://lists.gnu.org/archive/html/qemu-devel/2016-06/msg06397.html
- Rebased on top of the current master
- Patch 2
  - Now that job->id no longer holds the device name, replace
    "The active block job for device '%s' cannot be completed"
    with "The active block job '%s' cannot be completed"
    in mirror_complete() and block_job_complete().
- Patch 4
  - Keep using ERROR_CLASS_DEVICE_NOT_ACTIVE in find_block_job() and
    change error path. [Max]
- Patch 6
  - Fix merge conflicts after 274fcce.
- Patch 9
  - Fix subject in commit message [Max]
- Patch 15
  - Update iotests 109 and 156 [Max]

v1: https://lists.gnu.org/archive/html/qemu-devel/2016-06/msg02438.html
- Initial release

git backport-diff against v3:

Key:
[----] : patches are identical
[####] : number of functional differences between upstream/downstream patch
[down] : patch is downstream-only
The flags [FC] indicate (F)unctional and (C)ontextual differences, respectively

001/11:[----] [--] 'stream: Fix prototype of stream_start()'
002/11:[----] [--] 'blockjob: Update description of the 'id' field'
003/11:[----] [--] 'blockjob: Add block_job_get()'
004/11:[0009] [FC] 'block: Use block_job_get() in find_block_job()'
005/11:[----] [--] 'blockjob: Add 'job_id' parameter to block_job_create()'
006/11:[0012] [FC] 'mirror: Add 'job-id' parameter to 'blockdev-mirror' and 'drive-mirror''
007/11:[0012] [FC] 'backup: Add 'job-id' parameter to 'blockdev-backup' and 'drive-backup''
008/11:[0008] [FC] 'stream: Add 'job-id' parameter to 'block-stream''
009/11:[0012] [FC] 'commit: Add 'job-id' parameter to 'block-commit''
010/11:[----] [--] 'qemu-img: Set the ID of the block job in img_commit()'
011/11:[0020] [FC] 'blockjob: Update description of the 'device' field in the QMP API'

Alberto Garcia (11):
  stream: Fix prototype of stream_start()
  blockjob: Update description of the 'id' field
  blockjob: Add block_job_get()
  block: Use block_job_get() in find_block_job()
  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'
  commit: Add 'job-id' parameter to 'block-commit'
  qemu-img: Set the ID of the block job in img_commit()
  blockjob: Update description of the 'device' field in the QMP API

 block/backup.c            |   9 ++--
 block/commit.c            |   7 +--
 block/mirror.c            |  22 +++++----
 block/stream.c            |  12 ++---
 blockdev.c                | 113 ++++++++++++++++++++++------------------------
 blockjob.c                |  39 ++++++++++++++--
 docs/qmp-events.txt       |  12 +++--
 hmp.c                     |   6 +--
 include/block/block_int.h |  47 +++++++++++--------
 include/block/blockjob.h  |  23 +++++++---
 include/qapi/qmp/qerror.h |   3 --
 qapi/block-core.json      |  72 +++++++++++++++++++++--------
 qemu-img.c                |   2 +-
 qmp-commands.hx           |  28 ++++++++----
 tests/test-blockjob-txn.c |   7 ++-
 15 files changed, 249 insertions(+), 153 deletions(-)

-- 
2.8.1

[Qemu-devel] [PATCH v4 01/11] 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>
Reviewed-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Kevin Wolf <kwolf@redhat.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 2057156..549b1ed 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -625,8 +625,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.
@@ -637,11 +637,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 v4 02/11] blockjob: Update description of the 'id' field

[Alberto Garcia]

The 'id' field of the BlockJob structure will be able to hold any ID,
not only a device name. This patch updates the description of that
field and the error messages where it is being used.

Soon we'll add the ability to set an arbitrary ID when creating a
block job.

Signed-off-by: Alberto Garcia <berto@igalia.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
---
 block/mirror.c            | 3 ++-
 blockjob.c                | 3 ++-
 include/block/blockjob.h  | 5 +----
 include/qapi/qmp/qerror.h | 3 ---
 4 files changed, 5 insertions(+), 9 deletions(-)

diff --git a/block/mirror.c b/block/mirror.c
index 8d96049..6e3dbd2 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -761,7 +761,8 @@ static void mirror_complete(BlockJob *job, Error **errp)
     target = blk_bs(s->target);
 
     if (!s->synced) {
-        error_setg(errp, QERR_BLOCK_JOB_NOT_READY, job->id);
+        error_setg(errp, "The active block job '%s' cannot be completed",
+                   job->id);
         return;
     }
 
diff --git a/blockjob.c b/blockjob.c
index 205da9d..ce0e27c 100644
--- a/blockjob.c
+++ b/blockjob.c
@@ -290,7 +290,8 @@ void block_job_set_speed(BlockJob *job, int64_t speed, Error **errp)
 void block_job_complete(BlockJob *job, Error **errp)
 {
     if (job->pause_count || job->cancelled || !job->driver->complete) {
-        error_setg(errp, QERR_BLOCK_JOB_NOT_READY, job->id);
+        error_setg(errp, "The active block job '%s' cannot be completed",
+                   job->id);
         return;
     }
 
diff --git a/include/block/blockjob.h b/include/block/blockjob.h
index 7dc720c..5181514 100644
--- a/include/block/blockjob.h
+++ b/include/block/blockjob.h
@@ -106,10 +106,7 @@ 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;
 
diff --git a/include/qapi/qmp/qerror.h b/include/qapi/qmp/qerror.h
index d08652a..6586c9f 100644
--- a/include/qapi/qmp/qerror.h
+++ b/include/qapi/qmp/qerror.h
@@ -19,9 +19,6 @@
 #define QERR_BASE_NOT_FOUND \
     "Base '%s' not found"
 
-#define QERR_BLOCK_JOB_NOT_READY \
-    "The active block job for device '%s' cannot be completed"
-
 #define QERR_BUS_NO_HOTPLUG \
     "Bus '%s' does not support hotplugging"
 
-- 
2.8.1

[Qemu-devel] [PATCH v4 03/11] 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>
Reviewed-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
---
 blockjob.c               | 13 +++++++++++++
 include/block/blockjob.h | 10 ++++++++++
 2 files changed, 23 insertions(+)

diff --git a/blockjob.c b/blockjob.c
index ce0e27c..ca2291b 100644
--- a/blockjob.c
+++ b/blockjob.c
@@ -60,6 +60,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;
+}
+
 /* Normally the job runs in its BlockBackend's AioContext.  The exception is
  * block_job_defer_to_main_loop() where it runs in the QEMU main loop.  Code
  * that supports both cases uses this helper function.
diff --git a/include/block/blockjob.h b/include/block/blockjob.h
index 5181514..0fe1540 100644
--- a/include/block/blockjob.h
+++ b/include/block/blockjob.h
@@ -211,6 +211,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 v4 04/11] block: Use block_job_get() in find_block_job()

[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.

We want to identify jobs by their ID and not by the block backend
they're attached to, so this patch ignores the backends altogether and
gets the job directly. Apart from making the code simpler, this will
allow us to find block jobs once they start having user-specified IDs.

To ensure backward compatibility we keep ERROR_CLASS_DEVICE_NOT_ACTIVE
as the error class if the job doesn't exist. In subsequent patches
we'll also need to keep the device name as the default job ID if the
user doesn't specify a different one.

Signed-off-by: Alberto Garcia <berto@igalia.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
---
 blockdev.c | 40 +++++++++++++---------------------------
 1 file changed, 13 insertions(+), 27 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index 3a104a0..f19f3b7 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -3704,42 +3704,28 @@ 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,
+/* Get a block job using its ID and acquire its AioContext */
+static BlockJob *find_block_job(const char *id, AioContext **aio_context,
                                 Error **errp)
 {
-    BlockBackend *blk;
-    BlockDriverState *bs;
+    BlockJob *job;
+
+    assert(id != NULL);
 
     *aio_context = NULL;
 
-    blk = blk_by_name(device);
-    if (!blk) {
-        goto notfound;
+    job = block_job_get(id);
+
+    if (!job) {
+        error_set(errp, ERROR_CLASS_DEVICE_NOT_ACTIVE,
+                  "Block job '%s' not found", id);
+        return NULL;
     }
 
-    *aio_context = blk_get_aio_context(blk);
+    *aio_context = blk_get_aio_context(job->blk);
     aio_context_acquire(*aio_context);
 
-    if (!blk_is_available(blk)) {
-        goto notfound;
-    }
-    bs = blk_bs(blk);
-
-    if (!bs->job) {
-        goto notfound;
-    }
-
-    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)
-- 
2.8.1

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

[Alberto Garcia]

When a new job is created, the job ID is taken from the device name of
the BDS. This patch adds a new 'job_id' parameter to let the caller
provide one instead.

This patch also verifies that the ID is always unique and well-formed.
This causes problems in a couple of places where no ID is being set,
because the BDS does not have a device name.

In the case of test_block_job_start() (from test-blockjob-txn.c) we
can simply use this new 'job_id' parameter to set the missing ID.

In the case of img_commit() (from qemu-img.c) we still don't have the
API to make commit_active_start() set the job ID, so we solve it by
setting a default value. We'll get rid of this as soon as we extend
the API.

Signed-off-by: Alberto Garcia <berto@igalia.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
---
 block/backup.c            |  3 ++-
 block/commit.c            |  2 +-
 block/mirror.c            |  2 +-
 block/stream.c            |  2 +-
 blockjob.c                | 29 +++++++++++++++++++++++++----
 include/block/blockjob.h  |  8 +++++---
 tests/test-blockjob-txn.c |  7 +++++--
 7 files changed, 40 insertions(+), 13 deletions(-)

diff --git a/block/backup.c b/block/backup.c
index f87f8d5..19c587c 100644
--- a/block/backup.c
+++ b/block/backup.c
@@ -541,7 +541,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 6e3dbd2..08d88ca 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -873,7 +873,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 ca2291b..511c0db 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"
@@ -116,9 +117,9 @@ static void block_job_detach_aio_context(void *opaque)
     block_job_unref(job);
 }
 
-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;
@@ -129,6 +130,26 @@ void *block_job_create(const BlockJobDriver *driver, BlockDriverState *bs,
         return NULL;
     }
 
+    if (job_id == NULL) {
+        job_id = bdrv_get_device_name(bs);
+        /* Assign a default ID if the BDS does not have a device
+         * name. We'll get rid of this soon when we finish extending
+         * the API of all commands that create block jobs. */
+        if (job_id[0] == '\0') {
+            job_id = "default_job";
+        }
+    }
+
+    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);
 
@@ -139,7 +160,7 @@ 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->id            = g_strdup(job_id);
     job->blk           = blk;
     job->cb            = cb;
     job->opaque        = opaque;
diff --git a/include/block/blockjob.h b/include/block/blockjob.h
index 0fe1540..7f6d924 100644
--- a/include/block/blockjob.h
+++ b/include/block/blockjob.h
@@ -222,6 +222,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.
@@ -238,9 +240,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 d3030f1..50e232a 100644
--- a/tests/test-blockjob-txn.c
+++ b/tests/test-blockjob-txn.c
@@ -91,11 +91,14 @@ static BlockJob *test_block_job_start(unsigned int iterations,
     BlockDriverState *bs;
     TestBlockJob *s;
     TestBlockJobCBData *data;
+    static unsigned counter;
+    char job_id[24];
 
     data = g_new0(TestBlockJobCBData, 1);
     bs = bdrv_new();
-    s = block_job_create(&test_block_job_driver, bs, 0, test_block_job_cb,
-                         data, &error_abort);
+    snprintf(job_id, sizeof(job_id), "job%u", counter++);
+    s = block_job_create(job_id, &test_block_job_driver, bs, 0,
+                         test_block_job_cb, data, &error_abort);
     s->iterations = iterations;
     s->use_timer = use_timer;
     s->rc = rc;
-- 
2.8.1

[Qemu-devel] [PATCH v4 06/11] 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>
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
---
 block/mirror.c            | 15 ++++++++-------
 blockdev.c                | 15 ++++++++-------
 hmp.c                     |  2 +-
 include/block/block_int.h |  6 ++++--
 qapi/block-core.json      | 12 +++++++++---
 qmp-commands.hx           | 10 +++++++---
 6 files changed, 37 insertions(+), 23 deletions(-)

diff --git a/block/mirror.c b/block/mirror.c
index 08d88ca..702a686 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -843,8 +843,8 @@ static const BlockJobDriver commit_active_job_driver = {
     .attached_aio_context   = mirror_attached_aio_context,
 };
 
-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,
                              BlockMirrorBackingMode backing_mode,
@@ -873,7 +873,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;
     }
@@ -906,8 +906,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, BlockMirrorBackingMode backing_mode,
                   BlockdevOnError on_source_error,
@@ -925,7 +925,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, backing_mode,
                      on_source_error, on_target_error, unmap, cb, opaque, errp,
                      &mirror_job_driver, is_none_mode, base);
@@ -973,7 +973,8 @@ void commit_active_start(BlockDriverState *bs, BlockDriverState *base,
         }
     }
 
-    mirror_start_job(bs, base, NULL, speed, 0, 0, MIRROR_LEAVE_BACKING_CHAIN,
+    mirror_start_job(NULL, bs, base, NULL, speed, 0, 0,
+                     MIRROR_LEAVE_BACKING_CHAIN,
                      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 f19f3b7..7693b07 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -3422,7 +3422,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,
@@ -3482,15 +3482,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, backing_mode,
                  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,
@@ -3634,7 +3634,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, backing_mode,
                            has_speed, speed,
                            has_granularity, granularity,
@@ -3649,7 +3649,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,
@@ -3690,7 +3691,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, backing_mode,
                            has_speed, speed,
                            has_granularity, granularity,
diff --git a/hmp.c b/hmp.c
index 925601a..3dd9943 100644
--- a/hmp.c
+++ b/hmp.c
@@ -1097,7 +1097,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 549b1ed..a572941 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -680,6 +680,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 use the
+ * device name of @bs.
  * @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
@@ -701,8 +703,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, BlockMirrorBackingMode backing_mode,
                   BlockdevOnError on_source_error,
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 98a20d2..76ded56 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -1108,6 +1108,9 @@
 #
 # Start mirroring a block device's writes to a new destination.
 #
+# @job-id: #optional identifier for the newly-created block job. If
+#          omitted, the device name will be used. (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 +1163,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 +1246,9 @@
 #
 # Start mirroring a block device's writes to a new destination.
 #
+# @job-id: #optional identifier for the newly-created block job. If
+#          omitted, the device name will be used. (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 +1285,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 6937e83..6a2ec8f 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -1656,8 +1656,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?",
@@ -1677,6 +1677,8 @@ of the source.
 
 Arguments:
 
+- "job-id": Identifier for the newly-created block job. If omitted,
+            the device name will be used. (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)
@@ -1720,7 +1722,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,
@@ -1735,6 +1737,8 @@ specifies the target of mirror operation.
 
 Arguments:
 
+- "job-id": Identifier for the newly-created block job. If omitted,
+            the device name will be used. (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 v4 07/11] 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>
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
---
 block/backup.c            |  8 ++++----
 blockdev.c                | 43 ++++++++++++++++++++++++-------------------
 hmp.c                     |  2 +-
 include/block/block_int.h |  8 +++++---
 qapi/block-core.json      | 12 +++++++++---
 qmp-commands.hx           | 10 +++++++---
 6 files changed, 50 insertions(+), 33 deletions(-)

diff --git a/block/backup.c b/block/backup.c
index 19c587c..dce6614 100644
--- a/block/backup.c
+++ b/block/backup.c
@@ -474,9 +474,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,
@@ -541,7 +541,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 7693b07..9cd5ae8 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1836,9 +1836,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,
@@ -1876,7 +1876,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,
@@ -1921,8 +1922,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,
@@ -1968,8 +1969,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,
@@ -3182,9 +3183,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,
@@ -3303,7 +3304,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);
@@ -3316,7 +3317,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,
@@ -3326,7 +3328,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,
@@ -3339,8 +3342,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,
@@ -3395,7 +3398,7 @@ void do_blockdev_backup(const char *device, const char *target,
             goto out;
         }
     }
-    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);
@@ -3404,7 +3407,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,
@@ -3413,7 +3417,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 3dd9943..0ca480a 100644
--- a/hmp.c
+++ b/hmp.c
@@ -1127,7 +1127,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 a572941..2e78804 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -715,6 +715,8 @@ void mirror_start(const char *job_id, BlockDriverState *bs,
 
 /*
  * backup_start:
+ * @job_id: The id of the newly-created job, or %NULL to use the
+ * device name of @bs.
  * @bs: Block device to operate on.
  * @target: Block device to write to.
  * @speed: The maximum speed, in bytes per second, or 0 for unlimited.
@@ -729,9 +731,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 76ded56..af1923d 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -866,6 +866,9 @@
 ##
 # @DriveBackup
 #
+# @job-id: #optional identifier for the newly-created block job. If
+#          omitted, the device name will be used. (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 +906,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 +915,9 @@
 ##
 # @BlockdevBackup
 #
+# @job-id: #optional identifier for the newly-created block job. If
+#          omitted, the device name will be used. (Since 2.7)
+#
 # @device: the name of the device which should be copied.
 #
 # @target: the name of the backup target device.
@@ -938,7 +944,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 6a2ec8f..a032089 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -1212,8 +1212,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,
     },
 
@@ -1229,6 +1229,8 @@ block-job-cancel command.
 
 Arguments:
 
+- "job-id": Identifier for the newly-created block job. If omitted,
+            the device name will be used. (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
@@ -1266,7 +1268,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,
     },
@@ -1280,6 +1282,8 @@ as backup target.
 
 Arguments:
 
+- "job-id": Identifier for the newly-created block job. If omitted,
+            the device name will be used. (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 v4 08/11] 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>
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
---
 block/stream.c            | 12 ++++++------
 blockdev.c                |  6 +++---
 hmp.c                     |  2 +-
 include/block/block_int.h | 10 ++++++----
 qapi/block-core.json      |  8 ++++++--
 qmp-commands.hx           |  4 +++-
 6 files changed, 25 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 9cd5ae8..6382ea6 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -3005,7 +3005,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,
@@ -3064,8 +3064,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 0ca480a..3733863 100644
--- a/hmp.c
+++ b/hmp.c
@@ -1485,7 +1485,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 2e78804..eaec978 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -622,6 +622,8 @@ int is_windows_drive(const char *filename);
 
 /**
  * stream_start:
+ * @job_id: The id of the newly-created job, or %NULL to use the
+ * device name of @bs.
  * @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.
@@ -640,10 +642,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 af1923d..dce1cc6 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -1425,6 +1425,9 @@
 # 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. If
+#          omitted, the device name will be used. (Since 2.7)
+#
 # @device: the device name
 #
 # @base:   #optional the common backing file name
@@ -1456,8 +1459,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 a032089..d61ea20 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -1106,7 +1106,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,
     },
 
@@ -1118,6 +1118,8 @@ Copy data from a backing file into a block device.
 
 Arguments:
 
+- "job-id": Identifier for the newly-created block job. If omitted,
+            the device name will be used. (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 v4 09/11] commit: 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>
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
---
 block/commit.c            |  7 ++++---
 block/mirror.c            |  6 +++---
 blockdev.c                |  9 +++++----
 include/block/block_int.h | 16 ++++++++++------
 qapi/block-core.json      |  5 ++++-
 qemu-img.c                |  2 +-
 qmp-commands.hx           |  4 +++-
 7 files changed, 30 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 702a686..705fbc0 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -931,8 +931,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)
@@ -973,7 +973,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,
                      MIRROR_LEAVE_BACKING_CHAIN,
                      on_error, on_error, false, cb, opaque, &local_err,
                      &commit_active_job_driver, false, base);
diff --git a/blockdev.c b/blockdev.c
index 6382ea6..9c679e3 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -3077,7 +3077,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,
@@ -3168,10 +3168,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 eaec978..a9e64c6 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -649,6 +649,8 @@ void stream_start(const char *job_id, BlockDriverState *bs,
 
 /**
  * commit_start:
+ * @job_id: The id of the newly-created job, or %NULL to use the
+ * device name of @bs.
  * @bs: Active block device.
  * @top: Top block device to be committed.
  * @base: Block device that will be written into, and become the new top.
@@ -660,12 +662,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 use the
+ * device name of @bs.
  * @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.
@@ -675,8 +679,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 dce1cc6..572f195 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -1010,6 +1010,9 @@
 # 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. If
+#          omitted, the device name will be used. (Since 2.7)
+#
 # @device:  the name of the device
 #
 # @base:   #optional The file name of the backing image to write data into.
@@ -1061,7 +1064,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 3322a1e..a78f598 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -921,7 +921,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 d61ea20..c46c65c 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -1151,7 +1151,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,
     },
 
@@ -1164,6 +1164,8 @@ data between 'top' and 'base' into 'base'.
 
 Arguments:
 
+- "job-id": Identifier for the newly-created block job. If omitted,
+            the device name will be used. (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 v4 10/11] qemu-img: Set the ID of the block job in img_commit()

[Alberto Garcia]

img_commit() creates a block job without an ID. This is no longer
allowed now that we require it to be unique and well-formed. We were
solving this by having a fallback in block_job_create(), but now that
we extended the API of commit_active_start() we can finally set an
explicit ID and revert that change.

Signed-off-by: Alberto Garcia <berto@igalia.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
---
 blockjob.c | 6 ------
 qemu-img.c | 2 +-
 2 files changed, 1 insertion(+), 7 deletions(-)

diff --git a/blockjob.c b/blockjob.c
index 511c0db..3b9cec7 100644
--- a/blockjob.c
+++ b/blockjob.c
@@ -132,12 +132,6 @@ void *block_job_create(const char *job_id, const BlockJobDriver *driver,
 
     if (job_id == NULL) {
         job_id = bdrv_get_device_name(bs);
-        /* Assign a default ID if the BDS does not have a device
-         * name. We'll get rid of this soon when we finish extending
-         * the API of all commands that create block jobs. */
-        if (job_id[0] == '\0') {
-            job_id = "default_job";
-        }
     }
 
     if (!id_wellformed(job_id)) {
diff --git a/qemu-img.c b/qemu-img.c
index a78f598..521724c 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -921,7 +921,7 @@ static int img_commit(int argc, char **argv)
         .bs   = bs,
     };
 
-    commit_active_start(NULL, bs, base_bs, 0, BLOCKDEV_ON_ERROR_REPORT,
+    commit_active_start("commit", bs, base_bs, 0, BLOCKDEV_ON_ERROR_REPORT,
                         common_block_job_cb, &cbi, &local_err);
     if (local_err) {
         goto done;
-- 
2.8.1

[Qemu-devel] [PATCH v4 11/11] blockjob: Update description of the 'device' field in the QMP API

[Alberto Garcia]

The 'device' field in all BLOCK_JOB_* events and 'block-job-*' command
is no longer the device name, but the ID of the job. This patch
updates the documentation to clarify that.

Signed-off-by: Alberto Garcia <berto@igalia.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
---
 docs/qmp-events.txt  | 12 ++++++++----
 qapi/block-core.json | 35 +++++++++++++++++++++++++----------
 2 files changed, 33 insertions(+), 14 deletions(-)

diff --git a/docs/qmp-events.txt b/docs/qmp-events.txt
index fa7574d..7967ec4 100644
--- a/docs/qmp-events.txt
+++ b/docs/qmp-events.txt
@@ -92,7 +92,8 @@ Data:
 
 - "type":     Job type (json-string; "stream" for image streaming
                                      "commit" for block commit)
-- "device":   Device name (json-string)
+- "device":   Job identifier. Originally the device name but other
+              values are allowed since QEMU 2.7 (json-string)
 - "len":      Maximum progress value (json-int)
 - "offset":   Current progress value (json-int)
               On success this is equal to len.
@@ -116,7 +117,8 @@ Data:
 
 - "type":     Job type (json-string; "stream" for image streaming
                                      "commit" for block commit)
-- "device":   Device name (json-string)
+- "device":   Job identifier. Originally the device name but other
+              values are allowed since QEMU 2.7 (json-string)
 - "len":      Maximum progress value (json-int)
 - "offset":   Current progress value (json-int)
               On success this is equal to len.
@@ -143,7 +145,8 @@ Emitted when a block job encounters an error.
 
 Data:
 
-- "device": device name (json-string)
+- "device": Job identifier. Originally the device name but other
+            values are allowed since QEMU 2.7 (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):
     "ignore": error has been ignored, the job may fail later
@@ -167,7 +170,8 @@ Data:
 
 - "type":     Job type (json-string; "stream" for image streaming
                                      "commit" for block commit)
-- "device":   Device name (json-string)
+- "device":   Job identifier. Originally the device name but other
+              values are allowed since QEMU 2.7 (json-string)
 - "len":      Maximum progress value (json-int)
 - "offset":   Current progress value (json-int)
               On success this is equal to len.
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 572f195..780c4a5 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -713,7 +713,8 @@
 #
 # @type: the job type ('stream' for image streaming)
 #
-# @device: the block device name
+# @device: The job identifier. Originally the device name but other
+#          values are allowed since QEMU 2.7
 #
 # @len: the maximum progress value
 #
@@ -1475,7 +1476,9 @@
 #
 # Throttling can be disabled by setting the speed to 0.
 #
-# @device: the device name
+# @device: The job identifier. This used to be a device name (hence
+#          the name of the parameter), but since QEMU 2.7 it can have
+#          other values.
 #
 # @speed:  the maximum speed, in bytes per second, or 0 for unlimited.
 #          Defaults to 0.
@@ -1506,7 +1509,9 @@
 # operation can be started at a later time to finish copying all data from the
 # backing file.
 #
-# @device: the device name
+# @device: The job identifier. This used to be a device name (hence
+#          the name of the parameter), but since QEMU 2.7 it can have
+#          other values.
 #
 # @force: #optional whether to allow cancellation of a paused job (default
 #         false).  Since 1.3.
@@ -1532,7 +1537,9 @@
 # the operation is actually paused.  Cancelling a paused job automatically
 # resumes it.
 #
-# @device: the device name
+# @device: The job identifier. This used to be a device name (hence
+#          the name of the parameter), but since QEMU 2.7 it can have
+#          other values.
 #
 # Returns: Nothing on success
 #          If no background operation is active on this device, DeviceNotActive
@@ -1552,7 +1559,9 @@
 #
 # This command also clears the error status of the job.
 #
-# @device: the device name
+# @device: The job identifier. This used to be a device name (hence
+#          the name of the parameter), but since QEMU 2.7 it can have
+#          other values.
 #
 # Returns: Nothing on success
 #          If no background operation is active on this device, DeviceNotActive
@@ -1578,7 +1587,9 @@
 #
 # A cancelled or paused job cannot be completed.
 #
-# @device: the device name
+# @device: The job identifier. This used to be a device name (hence
+#          the name of the parameter), but since QEMU 2.7 it can have
+#          other values.
 #
 # Returns: Nothing on success
 #          If no background operation is active on this device, DeviceNotActive
@@ -2423,7 +2434,8 @@
 #
 # @type: job type
 #
-# @device: device name
+# @device: The job identifier. Originally the device name but other
+#          values are allowed since QEMU 2.7
 #
 # @len: maximum progress value
 #
@@ -2454,7 +2466,8 @@
 #
 # @type: job type
 #
-# @device: device name
+# @device: The job identifier. Originally the device name but other
+#          values are allowed since QEMU 2.7
 #
 # @len: maximum progress value
 #
@@ -2477,7 +2490,8 @@
 #
 # Emitted when a block job encounters an error
 #
-# @device: device name
+# @device: The job identifier. Originally the device name but other
+#          values are allowed since QEMU 2.7
 #
 # @operation: I/O operation
 #
@@ -2497,7 +2511,8 @@
 #
 # @type: job type
 #
-# @device: device name
+# @device: The job identifier. Originally the device name but other
+#          values are allowed since QEMU 2.7
 #
 # @len: maximum progress value
 #
-- 
2.8.1

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

[Max Reitz]

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

On 05.07.2016 16:28, 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: Kevin Wolf <kwolf@redhat.com>
> ---
>  block/mirror.c            | 15 ++++++++-------
>  blockdev.c                | 15 ++++++++-------
>  hmp.c                     |  2 +-
>  include/block/block_int.h |  6 ++++--
>  qapi/block-core.json      | 12 +++++++++---
>  qmp-commands.hx           | 10 +++++++---
>  6 files changed, 37 insertions(+), 23 deletions(-)

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



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

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

[Max Reitz]

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

On 05.07.2016 16:28, 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>
> Reviewed-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block/backup.c            |  8 ++++----
>  blockdev.c                | 43 ++++++++++++++++++++++++-------------------
>  hmp.c                     |  2 +-
>  include/block/block_int.h |  8 +++++---
>  qapi/block-core.json      | 12 +++++++++---
>  qmp-commands.hx           | 10 +++++++---
>  6 files changed, 50 insertions(+), 33 deletions(-)

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


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

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

[Max Reitz]

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

On 05.07.2016 16:28, 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>
> Reviewed-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block/stream.c            | 12 ++++++------
>  blockdev.c                |  6 +++---
>  hmp.c                     |  2 +-
>  include/block/block_int.h | 10 ++++++----
>  qapi/block-core.json      |  8 ++++++--
>  qmp-commands.hx           |  4 +++-
>  6 files changed, 25 insertions(+), 17 deletions(-)

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


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

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

[Max Reitz]

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

On 05.07.2016 16:29, 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>
> Reviewed-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block/commit.c            |  7 ++++---
>  block/mirror.c            |  6 +++---
>  blockdev.c                |  9 +++++----
>  include/block/block_int.h | 16 ++++++++++------
>  qapi/block-core.json      |  5 ++++-
>  qemu-img.c                |  2 +-
>  qmp-commands.hx           |  4 +++-
>  7 files changed, 30 insertions(+), 19 deletions(-)

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


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

Re: [Qemu-devel] [PATCH v4 10/11] qemu-img: Set the ID of the block job in img_commit()

[Max Reitz]

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

On 05.07.2016 16:29, Alberto Garcia wrote:
> img_commit() creates a block job without an ID. This is no longer
> allowed now that we require it to be unique and well-formed. We were
> solving this by having a fallback in block_job_create(), but now that
> we extended the API of commit_active_start() we can finally set an
> explicit ID and revert that change.
> 
> Signed-off-by: Alberto Garcia <berto@igalia.com>
> Reviewed-by: Max Reitz <mreitz@redhat.com>
> Reviewed-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  blockjob.c | 6 ------
>  qemu-img.c | 2 +-
>  2 files changed, 1 insertion(+), 7 deletions(-)
> 
> diff --git a/blockjob.c b/blockjob.c
> index 511c0db..3b9cec7 100644
> --- a/blockjob.c
> +++ b/blockjob.c
> @@ -132,12 +132,6 @@ void *block_job_create(const char *job_id, const BlockJobDriver *driver,
>  
>      if (job_id == NULL) {
>          job_id = bdrv_get_device_name(bs);
> -        /* Assign a default ID if the BDS does not have a device
> -         * name. We'll get rid of this soon when we finish extending
> -         * the API of all commands that create block jobs. */
> -        if (job_id[0] == '\0') {
> -            job_id = "default_job";
> -        }

I stand by my R-b, but as a remark to what you said for v3: I can't
imagine how this function can be called in a way that job_id will be
empty here (after this patch), and this is why I proposed an
assert(job_id[0] != '\0'). It'd probably be a mistake on our part if
such a case could happen (which is why an assertion would be fine).

However, gracefully returning an error is of course fine, too. The issue
I take with this is that the error we'd be returning is "Invalid job ID
''", which isn't very helpful (it should be "No job ID specified, and no
default available").

In any case, since I can't imagine the job_id being empty here and since
we'll handle that case gracefully in case it should occur, I won't
object (and my R-b stands).

Max

>      }
>  
>      if (!id_wellformed(job_id)) {
> diff --git a/qemu-img.c b/qemu-img.c
> index a78f598..521724c 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -921,7 +921,7 @@ static int img_commit(int argc, char **argv)
>          .bs   = bs,
>      };
>  
> -    commit_active_start(NULL, bs, base_bs, 0, BLOCKDEV_ON_ERROR_REPORT,
> +    commit_active_start("commit", bs, base_bs, 0, BLOCKDEV_ON_ERROR_REPORT,
>                          common_block_job_cb, &cbi, &local_err);
>      if (local_err) {
>          goto done;
> 



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

Re: [Qemu-devel] [PATCH v4 10/11] qemu-img: Set the ID of the block job in img_commit()

[Alberto Garcia]

On Tue 05 Jul 2016 05:56:09 PM CEST, Max Reitz <mreitz@redhat.com> wrote:
>> @@ -132,12 +132,6 @@ void *block_job_create(const char *job_id, const BlockJobDriver *driver,
>>  
>>      if (job_id == NULL) {
>>          job_id = bdrv_get_device_name(bs);
>> -        /* Assign a default ID if the BDS does not have a device
>> -         * name. We'll get rid of this soon when we finish extending
>> -         * the API of all commands that create block jobs. */
>> -        if (job_id[0] == '\0') {
>> -            job_id = "default_job";
>> -        }
>
> I stand by my R-b, but as a remark to what you said for v3: I can't
> imagine how this function can be called in a way that job_id will be
> empty here (after this patch), and this is why I proposed an
> assert(job_id[0] != '\0'). It'd probably be a mistake on our part if
> such a case could happen (which is why an assertion would be fine).

At the moment you cannot do it, but if we allow creating block jobs on
other nodes (e.g. intermediate block streaming) the user must provide a
job ID, else there'll be no default.

> However, gracefully returning an error is of course fine, too. The
> issue I take with this is that the error we'd be returning is "Invalid
> job ID ''", which isn't very helpful (it should be "No job ID
> specified, and no default available").

We can add a special check for the empty string and use that error
message that you propose.

Berto

Re: [Qemu-devel] [PATCH v4 00/11] Allow creating block jobs with a user-defined ID

[Kevin Wolf]

Am 05.07.2016 um 16:28 hat Alberto Garcia geschrieben:
> Hi all,
> 
> block jobs are currently identified by the name of the block backend
> of the BDS where the job was started.
> 
> The problem with this is that you cannot have block jobs on nodes
> where there is no such name.
> 
> This series turns the 'id' field of the BlockJob structure into an
> actual ID, it guarantees that is unique and always present and allows
> the user to set it.

Thanks, applied to the block branch.

Kevin

Re: [Qemu-devel] [PATCH v4 00/11] Allow creating block jobs with a user-defined ID

[Alberto Garcia]

On Thu 07 Jul 2016 02:48:17 PM CEST, Kevin Wolf wrote:
>> Hi all,
>> 
>> block jobs are currently identified by the name of the block backend
>> of the BDS where the job was started.
>> 
>> The problem with this is that you cannot have block jobs on nodes
>> where there is no such name.
>> 
>> This series turns the 'id' field of the BlockJob structure into an
>> actual ID, it guarantees that is unique and always present and allows
>> the user to set it.
>
> Thanks, applied to the block branch.

Thanks Kevin,

I would still like to have an ACK from John Snow, not necessarily a
complete review of the series but I definitely want to avoid any
fundamental conflict with the "Jobs 2.0 API" that is in the works.

Berto

Re: [Qemu-devel] [PATCH v4 00/11] Allow creating block jobs with a user-defined ID

[John Snow]

On 07/07/2016 11:12 AM, Alberto Garcia wrote:
> On Thu 07 Jul 2016 02:48:17 PM CEST, Kevin Wolf wrote:
>>> Hi all,
>>>
>>> block jobs are currently identified by the name of the block backend
>>> of the BDS where the job was started.
>>>
>>> The problem with this is that you cannot have block jobs on nodes
>>> where there is no such name.
>>>
>>> This series turns the 'id' field of the BlockJob structure into an
>>> actual ID, it guarantees that is unique and always present and allows
>>> the user to set it.
>>
>> Thanks, applied to the block branch.
> 
> Thanks Kevin,
> 
> I would still like to have an ACK from John Snow, not necessarily a
> complete review of the series but I definitely want to avoid any
> fundamental conflict with the "Jobs 2.0 API" that is in the works.
> 
> Berto
> 

It's fine, I'm lagging behind with my upstream stuff. You're doing the
work and I'm not right now, so I lose my chance to complain :)

I appreciate the kindness, though.

ACK.