[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-block] [PATCH v2 02/15] blockjob: Decouple the ID from the dev

From: Alberto Garcia
Subject: Re: [Qemu-block] [PATCH v2 02/15] blockjob: Decouple the ID from the device name in the BlockJob struct
Date: Thu, 23 Jun 2016 14:47:06 +0200
User-agent: Notmuch/0.18.2 (http://notmuchmail.org) Emacs/24.4.1 (i586-pc-linux-gnu)

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.


reply via email to

[Prev in Thread] Current Thread [Next in Thread]