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

[Alberto Garcia <berto@igalia.com>]

On Wed 22 Jun 2016 02:42:24 PM CEST, Kevin Wolf wrote:
>> 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?

That was my first approach when I started to write the series, but I
discarded it for several reasons:

- More confusing API: we'd have many instances of fields and parameters
  named 'device' holding something that is not a device name or anything
  similar.

  We'd have e.g. block-commit(id, device), and the created job would
  have a 'device' that is not the user-specified 'device' but the
  user-specified 'id', which has no relation to devices.

- More risk of collisions: with the current system if no ID is specified
  a new one is be generated. That's guaranteed to be valid and unique.
  If we default to the device name we have no such guarantee.

- Potential compatibility break: there's a field that used to hold the
  name of the device but it no longer does.

I don't think any of them is a showstopper, and as you said if a client
is recent enough to understand job IDs it should also expect the device
field to hold something different from a device name. But I'm not a big
fan of this kind of scenarios where the semantics of a returned value
depend on what we can assume that the caller is able to handle.

I thought adding a new 'ID' field was simpler. The device name is still
a device name (where it makes sense). The default ID is guaranteed to be
valid and guaranteed not to clash with user-defined IDs. The API is (in
my opinion) more clear.

The only problems that I can think of:

- BlockJobInfo and the events expose the 'device' field which is
  superfluous.
- block-job-{pause,resume,...} can take an ID or a device name.

All that said, I'm open to change the implementation, but I'd like to
make sure that the problems of keeping the ID and device name merged are
considered.

Berto