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

[Kevin Wolf]

Am 22.06.2016 um 16:42 hat Alberto Garcia geschrieben:
> 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.

Yes, the 'device' field ends up having the wrong name then. This is
ugly, but I think having code to deal with two ways of addressing jobs,
in two different namespaces, is a cost too high to be justifiable just
for making things look nicer.

I seem to remember that John is going to introduce a new set of job
management functions anyway while he generalises block jobs to just
background jobs, so in that case the ugliness would be confined to a
deprecated legacy interface. I think that's tolerable.

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

The expectation is that if a client passes an ID for one job, it will
pass IDs for all jobs. As long as you never pass an ID, the interface
looks exactly the same as it used to. If you pass it always, you're
obviously responsible for choosing unique IDs.

The only vaguely interesting case is if you mix both styles and then
create a new job with an explicit ID that you already used for a block
device. In this case you get an error, and I'd argue you deserve it.

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

Old clients never set an explicit ID, so what they get in this field is
always the default, which is a device name.

I don't see the compatibility problem?

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

It's really just another case of making something configurable and
letting it default to the previously hardcoded value. Pretty much a
normal thing to do.

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

Yes. There are two parts that I don't like about this.

The first one is that we need additional code to keep track of the
device name and to look it up.

The other, more important one is that it couples block jobs more tightly
with a BDS:

* What do you with a background job that doesn't have a device
  name because it doesn't work on a BDS? Will 'device' become optional
  everywhere? But how is this less problematic for compatibility than
  returning non-device-name IDs? (To be clear, I don't think either is a
  real problem, but you can hardly dismiss one and accept the other.)

* And what do you do once we allow more than one job per device? Then
  the device name isn't suitable for addressing the job any more. And
  letting the client use the device name after it started the first job,
  but not any more after it started the second one, feels wrong.

Kevin