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

[Max Reitz]

On 2018-08-29 14:29, Alberto Garcia wrote:
> On Wed 29 Aug 2018 01:26:46 PM CEST, Max Reitz wrote:
>> On 2018-08-26 16:09, Alberto Garcia wrote:
>>> In the previous patches we removed all child references from
>>> bs->{options,explicit_options} because keeping them is useless and
>>> wrong.
>>>
>>> Because of this, any attempt to reopen a BlockDriverState using a
>>> child reference as one of its options would result in a failure,
>>> because bdrv_reopen_prepare() would detect that there's a new option
>>> (the child reference) that wasn't present in bs->options.
>>>
>>> But passing child references on reopen can be useful. It's a way to
>>> specify a BDS's child without having to pass recursively all of the
>>> child's options, and if the reference points to a different BDS then
>>> this can allow us to replace the child.
>>>
>>> However, replacing the child is something that needs to be implemented
>>> case by case and only when it makes sense. For now, this patch allows
>>> passing a child reference as long as it points to the current child of
>>> the BlockDriverState.
>>
>> "For now"?  If you intend it to be case-by-case, won't it be handled by
>> the driver's bdrv_reopen_prepare() implementation?
> 
> Yes, it will (except the "backing" reference which is handled by the
> main bdrv_reopen_prepare() function).
> 
> What I meant with "For now" is that this patch doesn't add support for
> any other case.

OK.

>>> @@ -3241,6 +3241,25 @@ int bdrv_reopen_prepare(BDRVReopenState 
>>> *reopen_state, BlockReopenQueue *queue,
>>>              QObject *new = entry->value;
>>>              QObject *old = qdict_get(reopen_state->bs->options, 
>>> entry->key);
>>>  
>>> +            /* We allow child references 'child_name'='value'
>>> +             * if 'child_name' is an existing child of the BDS
>>> +             * and 'value' is the child's node name (a string). */
>>> +            if (qobject_type(new) == QTYPE_QSTRING) {
>>> +                BdrvChild *child;
>>> +                QLIST_FOREACH(child, &reopen_state->bs->children, next) {
>>> +                    if (!strcmp(child->name, entry->key)) {
>>> +                        break;
>>> +                    }
>>> +                }
>>> +
>>> +                if (child) {
>>> +                    const char *str = qobject_get_try_str(new);
>>> +                    if (!strcmp(child->bs->node_name, str)) {
>>> +                        continue; /* Found child with this name, skip 
>>> option */
>>> +                    }
>>> +                }
>>> +            }
>>> +
>>
>> Looks good, but I think the comment looks a bit convoluted.  Maybe add
>> a note like "(so the child has to stay the same)"?
> 
> Ok, I can think of making it shorter.

I didn't really mean "convoluted" in the sense of "it's too much".  It's
mostly just hard to grasp because it tries to be as exact as possible.
Adding a short "colloquial" summary would help.

>> Third...  Hm.  This is just a general question.  So as far as I
>> understand we decided that when you don't specify an option for
>> reopen, it means that the default is used, and that we don't want to
>> retain the old value.
> 
> Correct.
> 
>> 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.

So choosing (b) yields no real value over (c), and it may be confusing.
Which is why I prefer (c).

(Whereas (a) would provide real value, because the user has no simple
way of opening the default backing chain.  But I agree that we don't
need it (now), and that going for it would spill the can of worms.)

> It it inconsistent with the way blockdev-reopen is supposed to work?
> Maybe it is, but I think we have to make an exception in this case.

Making it an error would definitely be safe, however.

>> So why isn't it mandatory to re-specify all children for now?
> 
> It is
Is it?  Why does

$ qemu-io -c 'reopen -o lazy-refcounts=on' foo.qcow2

work, then?  Shouldn't I have to specify @file?

Max

signature.asc
Description: OpenPGP digital signature