On 20.05.19 12:41, Vladimir Sementsov-Ogievskiy wrote:
> 10.04.2019 23:20, Max Reitz wrote:
>> For completeness' sake, add a function for accessing a node's storage
>> child, too.  For filters, this is their filtered child; for non-filters,
>> this is bs->file.
>>
>> Some places are deliberately left unconverted:
>> - BDS opening/closing functions where bs->file is handled specially
>>    (which is basically wrong, but at least simplifies probing)
>> - bdrv_co_block_status_from_file(), because its name implies that it
>>    points to ->file
>> - bdrv_snapshot_goto() in one places unrefs bs->file.  Such a
>>    modification is not covered by this patch and is therefore just
>>    safeguarded by an additional assert(), but otherwise kept as-is.
>>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>

[...]

>> --- a/block/snapshot.c
>> +++ b/block/snapshot.c
> 
> [..]
> 
>> @@ -184,6 +186,7 @@ int bdrv_snapshot_goto(BlockDriverState *bs,
>>                          Error **errp)
>>   {
>>       BlockDriver *drv = bs->drv;
>> +    BlockDriverState *storage_bs;
>>       int ret, open_ret;
>>   
>>       if (!drv) {
>> @@ -204,39 +207,40 @@ int bdrv_snapshot_goto(BlockDriverState *bs,
>>           return ret;
>>       }
>>   
>> -    if (bs->file) {
>> -        BlockDriverState *file;
>> +    storage_bs = bdrv_storage_bs(bs);
>> +    if (storage_bs) {
>>           QDict *options = qdict_clone_shallow(bs->options);
>>           QDict *file_options;
>>           Error *local_err = NULL;
>>   
>> -        file = bs->file->bs;
>>           /* Prevent it from getting deleted when detached from bs */
>> -        bdrv_ref(file);
>> +        bdrv_ref(storage_bs);
>>   
>>           qdict_extract_subqdict(options, &file_options, "file.");
>>           qobject_unref(file_options);
>> -        qdict_put_str(options, "file", bdrv_get_node_name(file));
>> +        qdict_put_str(options, "file", bdrv_get_node_name(storage_bs));
>>   
>>           if (drv->bdrv_close) {
>>               drv->bdrv_close(bs);
>>           }
>> +
>> +        assert(bs->file->bs == storage_bs);
> 
> Hmm, but what save us from this assertion fail for backing-filters? Before your
> patch it was unreachable for them. Or what I miss?

Ha, good point.  I simply missed this point.  Yes, I need to check
whether storage_bs is bs->file or bs->backing and then take the
corresponding sub-QDict from bs->options.

Max

>>           bdrv_unref_child(bs, bs->file);
>>           bs->file = NULL;
>>   
>> -        ret = bdrv_snapshot_goto(file, snapshot_id, errp);
>> +        ret = bdrv_snapshot_goto(storage_bs, snapshot_id, errp);
>>           open_ret = drv->bdrv_open(bs, options, bs->open_flags, &local_err);
>>           qobject_unref(options);
>>           if (open_ret < 0) {
>> -            bdrv_unref(file);
>> +            bdrv_unref(storage_bs);
>>               bs->drv = NULL;
>>               /* A bdrv_snapshot_goto() error takes precedence */
>>               error_propagate(errp, local_err);
>>               return ret < 0 ? ret : open_ret;
>>           }
>>   
>> -        assert(bs->file->bs == file);
>> -        bdrv_unref(file);
>> +        assert(bs->file->bs == storage_bs);
>> +        bdrv_unref(storage_bs);
>>           return ret;
>>       }
>>   
> 
> 
>