[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v4 03/11] block: Storage child access function
From: |
Max Reitz |
Subject: |
Re: [Qemu-devel] [PATCH v4 03/11] block: Storage child access function |
Date: |
Tue, 28 May 2019 20:09:09 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.6.1 |
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 <address@hidden>
[...]
>> --- 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;
>> }
>>
>
>
>
signature.asc
Description: OpenPGP digital signature