Re: [Qemu-devel] [PATCH 03/19] block: Respect backing bs in bdrv_refresh_filename

[Max Reitz <mreitz@redhat.com>]

[-- Attachment #1.1: Type: text/plain, Size: 3269 bytes --]

On 02.05.2016 17:36, Kevin Wolf wrote:
> Am 26.04.2016 um 23:32 hat Max Reitz geschrieben:
>> Basically, bdrv_refresh_filename() should respect all children of a
>> BlockDriverState. However, generally those children are driver-specific,
>> so this function cannot handle the general case. On the other hand,
>> there are only few drivers which use other children than @file and
>> @backing (that being vmdk, quorum, and blkverify).
>>
>> Most block drivers only use @file and/or @backing (if they use any
>> children at all). Both can be implemented directly in
>> bdrv_refresh_filename.
>>
>> The user overriding the file's filename is already handled, however, the
>> user overriding the backing file is not. If this is done, opening the
>> BDS with the plain filename of its file will not be correct, so we may
>> not set bs->exact_filename in that case.
>>
>> iotest 051 contains test cases for overwriting the backing file, and so
>> its output changes with this patch applied (which I consider a good
>> thing).
>>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> ---
>>  block.c                       | 14 +++++++++++++-
>>  tests/qemu-iotests/051.out    |  8 ++++----
>>  tests/qemu-iotests/051.pc.out |  8 ++++----
>>  3 files changed, 21 insertions(+), 9 deletions(-)
>>
>> diff --git a/block.c b/block.c
>> index e178488..aff9ea3 100644
>> --- a/block.c
>> +++ b/block.c
>> @@ -3925,6 +3925,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 */
>> @@ -3936,13 +3937,24 @@ 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))
>> +        {
>>              qdict_put_obj(opts, "driver",
>>                            QOBJECT(qstring_from_str(drv->format_name)));
>> +
>>              QINCREF(bs->file->bs->full_open_options);
>>              qdict_put_obj(opts, "file",
>>                            QOBJECT(bs->file->bs->full_open_options));
>>  
>> +            if (bs->backing) {
>> +                QINCREF(bs->backing->bs->full_open_options);
>> +                qdict_put_obj(opts, "backing",
>> +                              QOBJECT(bs->backing->bs->full_open_options));
>> +            } else if (bs->backing_overridden && !bs->backing) {
>> +                qdict_put_obj(opts, "backing", QOBJECT(qstring_new()));
>> +            }
> 
> I see that the surrounding code does the same, but what's wrong with
> qdict_put()?

That it's a macro. clang_complete does not tell me about macros.

Will fix and try to keep in mind in the future, thanks.

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]