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

[Max Reitz]

On 23.06.2016 14:47, Alberto Garcia wrote:
> On Wed 22 Jun 2016 05:49:28 PM CEST, Kevin Wolf wrote:
>>> 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.
> 
> I think this part is negligible, but ok.
> 
>> 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.
> 
> Fair enough. Unless Max, Eric or someone else has something else to add
> I'll give it a try and see how it looks.

Sorry for the late response, but then again I don't actually have an
opinion either way.

The thing I feel most strongly about is the issue of storing a general
ID in a field named "device". However, as Kevin hinted at this becoming
irrelevant with John's work on decoupling block jobs from the block layer.

(And it probably will, because we don't want to call e.g.
block-job-pause that way then anymore, so John's probably going to add
newly named commands anyway, even if they do the same as the old ones. I
don't know.)

But this also means that I don't understand the first point of the
second part of what you quoted from Kevin here. I think he's referring
to what qemu returns e.g. in query-block-jobs. But why should
query-block-jobs return non-*block*-jobs? I think it should always omit
all background jobs that are not related to the block layer. Or at least
that would make sense.

The second point of the second part doesn't really strike with me
either. If a client uses the device name to identify a block job, they
should be used to a version of qemu that isn't capable of having more
than one job per device anyway, so they shouldn't launch more than a
single job per device to begin with.


So I don't think any of the pros and cons is a very strong point. I
personally feel like having a single ID field is more natural and making
the device name its default value is very intuitive, but I take issue
with its naming ("device"). So I don't care either way.

(Maybe, aside from John's work, we could get the naming issue out of the
way by introducing something like QMP aliases...?)

Max

signature.asc
Description: OpenPGP digital signature