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

Commit Message

Alberto Garcia June 9, 2016, 8:20 a.m. UTC

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 <[e-mail]>
---
 blockjob.c               | 15 +++++++++------
 include/block/blockjob.h | 12 ++++++++----
 include/qemu/id.h        |  1 +
 util/id.c                |  1 +
 4 files changed, 19 insertions(+), 10 deletions(-)

Comments

Max Reitz June 20, 2016, 4:17 p.m. UTC | #1

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 <[e-mail]>
> ---
>  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

Alberto Garcia June 21, 2016, 11:42 a.m. UTC | #2

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

Patch

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",
 };
 
 /*