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

[Kevin Wolf]

Am 22.06.2016 um 14:24 hat Alberto Garcia geschrieben:
> Block jobs are identified by the name of the BlockBackend of the BDS
> where the job was started.

Let me rephrase that: Block jobs are identified by an ID which defaults
to the name of the BlockBackend where the job was started and can't
currently be set by the user.

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

This approach requires us to keep track of both a device name and an ID
in order to maintain compatibility, and we always need to check both
when a job is referenced. This model is already a pain with node-name
and BB name, I wouldn't want to introduce more instances.

Why don't we go the easy route and make the ID configurable, but still
default to the device name?

> Signed-off-by: Alberto Garcia <address@hidden>

> --- a/include/block/blockjob.h
> +++ b/include/block/blockjob.h
>      /**
> +     * 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;

This in particular feels like a step backwards. This addition is
tightening the coupling between jobs and devices instead of removing it.

> @@ -464,7 +468,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;
> @@ -486,7 +490,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,
> @@ -496,7 +500,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,
> @@ -510,7 +514,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);
> @@ -538,7 +542,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);

These monitor related functions are the only users of the device name.
Let's simply redefine the respective QMP fields to refer to the job ID
rather than the device (because identifying the job is really what they
are used for; once we allow more than one block job per device, putting
the device there will be completely useless).

All of these redefinitions are backwards compatible. Only clients which
are new enough to set an individual ID can get something that isn't a
device name here. And they will be prepared for it.

Kevin