Re: [Qemu-devel] [PATCH v7 for-2.12 05/25] block: Respect backing bs in bdrv_refresh_filename

[Max Reitz]

On 2017-12-05 14:27, Alberto Garcia wrote:
> On Mon 20 Nov 2017 09:09:44 PM CET, Max Reitz wrote:
>> @@ -5016,6 +5016,7 @@ void bdrv_refresh_filename(BlockDriverState *bs)
>>  
>>          opts = qdict_new();
>>          has_open_options = append_open_options(opts, bs);
>> +        has_open_options |= bs->backing_overridden;
>>  
>>          /* If no specific options have been given for this BDS, the 
>> filename of
>>           * the underlying file should suffice for this one as well */
>> @@ -5027,11 +5028,20 @@ void bdrv_refresh_filename(BlockDriverState *bs)
>>           * file BDS. The full options QDict of that file BDS should somehow
>>           * contain a representation of the filename, therefore the following
>>           * suffices without querying the (exact_)filename of this BDS. */
>> -        if (bs->file->bs->full_open_options) {
>> +        if (bs->file->bs->full_open_options &&
>> +            (!bs->backing || bs->backing->bs->full_open_options))
>> +        {
> 
> Does this mean that both file. and backing. have to be overriden?

The bs->file->bs->full_open_options is not a check whether anything has
been overridden but just whether we have a QDict of runtime options for
file (and we should always have one).  After this series, that check
therefore disappears.

> Shouldn't that be a || instead of a && ??

The block under this if condition automatically constructs
bs->full_open_options -- but it can only do so if all of the relevant
children's options are known (because their options need to be put into
bs->full_open_options).

Previously, that was only the file child.  This patch adds support for
the backing child: Whenever there is a backing child node, we need to
include its options under the "backing" key.

So that's where the condition comes from: For all children that this
node has, we need their full_open_options or we cannot construct this
node's full_open_options.

We do have bs->file (because that is the condition for the block this
condition is in), so we need bs->file->bs->full_open_options.  And if we
have bs->backing, we also need bs->backing->bs->full_open_options.

>>              qdict_put_str(opts, "driver", drv->format_name);
>>              QINCREF(bs->file->bs->full_open_options);
>>              qdict_put(opts, "file", bs->file->bs->full_open_options);
>>  
>> +            if (bs->backing) {
>> +                QINCREF(bs->backing->bs->full_open_options);
>> +                qdict_put(opts, "backing", 
>> bs->backing->bs->full_open_options);
>> +            } else if (bs->backing_overridden && !bs->backing) {
>> +                qdict_put(opts, "backing", qstring_new());
>> +            }
> 
> You don't need the !bs->backing in the second if, it's implied from the
> previous one.

Maybe that was intentional (to be more explicit, and because after this
series only this branch remains), maybe it wasn't.  I don't know
anymore, so I'll just drop it (even though I'll have to re-add it later,
because as I said, only this else if branch will remain).

Max

signature.asc
Description: OpenPGP digital signature