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

[John Snow <jsnow@redhat.com>]

On 06/30/2016 09:03 AM, Alberto Garcia wrote:
> On Wed 29 Jun 2016 07:20:55 PM CEST, Max Reitz 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.
> 
> I actually forgot to Cc him, I'm doing it now.
> 
> The idea is that I don't want to add anything now that is going to cause
> headaches later. Adding a new 'id' field to block jobs and keeping
> 'device' feels more natural to me, but reusing the 'device' field and
> allowing any ID set by the user requires less changes both to the code
> and the API.
> 
> Berto
> 

Reviewing everything now, sorry for being MIA, and thank you for keeping
me in the loop.

--js