Re: [Qemu-devel] [PATCH v5 12/22] block: Allow recursive "file"s

[Max Reitz <mreitz@redhat.com>]

On 13.12.2013 21:19, Kevin Wolf wrote:
> Am 13.12.2013 um 18:10 hat Max Reitz geschrieben:
>> It should be possible to use a format as a driver for a file which in
>> turn requires another file, i.e., nesting file formats.
>>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
> Hm, does this do what I think it does?
>
> $ ./qemu-img convert -O qcow2 /home/kwolf/images/hd.img /tmp/hd.qcow2
> $ ./qemu-img convert -f raw -O qcow2 /tmp/hd.qcow2 /tmp/hd.qcow2.qcow2
> $ x86_64-softmmu/qemu-system-x86_64 -drive driver=qcow2,file.driver=qcow2,file.file.driver=file,file.file.filename=/tmp/hd.qcow2.qcow2
>
> I can't decide whether this is awesomeness or insanity, but in any case
> it works with this patch. :-)
>
> Worth a qemu-iotests case, I think.
>
>>   block.c | 17 ++++++++++++-----
>>   1 file changed, 12 insertions(+), 5 deletions(-)
>>
>> diff --git a/block.c b/block.c
>> index 9659eb5..9222669 100644
>> --- a/block.c
>> +++ b/block.c
>> @@ -948,14 +948,19 @@ int bdrv_file_open(BlockDriverState **pbs, const char *filename,
>>           goto fail;
>>       }
>>   
>> -    ret = bdrv_open_common(bs, NULL, options, flags, drv, &local_err);
>> +    if (!drv->bdrv_file_open) {
>> +        ret = bdrv_open(bs, filename, options, flags, drv, &local_err);
>> +        options = NULL;
>> +    } else {
>> +        ret = bdrv_open_common(bs, NULL, options, flags, drv, &local_err);
>> +    }
>>       if (ret < 0) {
>>           error_propagate(errp, local_err);
>>           goto fail;
>>       }
>>   
>>       /* Check if any unknown options were used */
>> -    if (qdict_size(options) != 0) {
>> +    if (options && (qdict_size(options) != 0)) {
>>           const QDictEntry *entry = qdict_first(options);
>>           error_setg(errp, "Block protocol '%s' doesn't support the option '%s'",
>>                      drv->format_name, entry->key);
>> @@ -970,10 +975,12 @@ int bdrv_file_open(BlockDriverState **pbs, const char *filename,
>>   
>>   fail:
>>       QDECREF(options);
>> -    if (!bs->drv) {
>> -        QDECREF(bs->options);
>> +    if (bs) {
>> +        if (!bs->drv) {
>> +            QDECREF(bs->options);
>> +        }
>> +        bdrv_unref(bs);
>>       }
>> -    bdrv_unref(bs);
>>       return ret;
>>   }
> Not sure why this hunk is needed, but anyway:

Ah, I think I know. It is there because of patch 9. As stated in the 
cover letter, additionally to the double QDECREF on options, I noticed a 
possible NULL dereference of bs on error in the "if (reference)" path 
and fixed it in this version. In v3, it was addressed by this hunk, 
which somehow got into patch 12 rather than 9. I'll remove it, now that 
it's unnecessary.

Max