Re: [Qemu-devel] [PATCH 5/9] block: Allow child references on reopen

[Max Reitz]

On 2018-08-29 16:56, Alberto Garcia wrote:
> On Wed 29 Aug 2018 02:59:01 PM CEST, Max Reitz wrote:
>>>> Specifically, that means when you don't specify @backing, the default
>>>> chain will be opened and may replace the current one.
>>>
>>> At the moment "backing" is the only child option that can be omitted,
>>> all others must be specified. So if you leave "backing" out on reopen()
>>> we have the following possibilities:
>>>
>>>   a) We open the default backing file from disk. I don't think we want
>>>      to open a new image on reopen(). Plus, what happens if the current
>>>      backing image is the default one? Do we need to detect that? And
>>>      what if it's the default image but has non-default options? The
>>>      whole thing looks like a can of worms.
>>
>> True.
>>
>>>   b) We leave the current backing file untouched.
>>
>> Hm.  That doesn't make sense for blockdev-add, so you could argue that
>> this behavior works as a default for reopen (because it cannot be
>> "confused" with the blockdev-add default).
>>
>>>   c) We forbid it, and the user has to pass an explicit child, or NULL.
>>
>> That sounds good.
>>
>>> I chose (b) in this series. I suppose (c) could also be an option. But
>>> (a) doesn't looke like a good idea.
>>
>> I agree with the last sentiment, but I'd prefer making it an error
>> instead of choosing (b).  Yes, you could argue that choosing (b) as a
>> default makes sense in a way, but in another, it just doesn't make
>> sense.  (Because while (b) makes no sense for blockdev-add, (a) would
>> make sense for both blockdev-add and reopen.  Sure, it's horrible to
>> support, but from a user's POV, it makes sense.  So it's just
>> confusing not to make that the default for reopen as well).
>>
>> But the most important point is that it's trivial for the user to keep
>> the current backing file.  They just need to give the current
>> backing's node-name.
> 
> Or NULL if there's no backing file, right?

Ah, right, that case...  Yes.  Or we could make an exception (if
@backing wasn't specified, and there is no default backing file, then we
can suppress the error and handle it like backing=null).

Right now, I'm not sure whether making an exception is a great idea, but
it's stupid always having to pass backing=null for all qcow2 nodes that
do not have a backing file...

> The problem I see with this is that it would make the options quite
> verbose. "backing" is an attribute of all BDSs, also protocol ones. I
> suppose we can make "backing" optional in those, or is there any case
> where it makes sense?

Right.  Making it optional when there is no default backing file maybe
is less bad than always requiring it.

Max

signature.asc
Description: OpenPGP digital signature