Re: [Qemu-devel] [PATCH v4 03/11] block: Storage child access function

[Vladimir Sementsov-Ogievskiy]

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/io.c
> +++ b/block/io.c

[..]

> @@ -2559,7 +2554,7 @@ int coroutine_fn bdrv_co_flush(BlockDriverState *bs)
>       }
>   
>       /* Write back cached data to the OS even with cache=unsafe */
> -    BLKDBG_EVENT(bs->file, BLKDBG_FLUSH_TO_OS);
> +    BLKDBG_EVENT(bdrv_storage_child(bs), BLKDBG_FLUSH_TO_OS);

Hmm, preexistent, but strange that we call EVENT for bs->file before action on 
bs...

>       if (bs->drv->bdrv_co_flush_to_os) {
>           ret = bs->drv->bdrv_co_flush_to_os(bs);
>           if (ret < 0) {
> @@ -2577,7 +2572,7 @@ int coroutine_fn bdrv_co_flush(BlockDriverState *bs)
>           goto flush_parent;
>       }
>   
> -    BLKDBG_EVENT(bs->file, BLKDBG_FLUSH_TO_DISK);
> +    BLKDBG_EVENT(bdrv_storage_child(bs), BLKDBG_FLUSH_TO_DISK);
>       if (!bs->drv) {
>           /* bs->drv->bdrv_co_flush() might have ejected the BDS
>            * (even in case of apparent success) */
> @@ -2622,7 +2617,8 @@ int coroutine_fn bdrv_co_flush(BlockDriverState *bs)
>        * in the case of cache=unsafe, so there are no useless flushes.
>        */
>   flush_parent:
> -    ret = bs->file ? bdrv_co_flush(bs->file->bs) : 0;
> +    storage_bs = bdrv_storage_bs(bs);
> +    ret = storage_bs ? bdrv_co_flush(storage_bs) : 0;
>   out:
>       /* Notify any pending flushes that we have completed */
>       if (ret == 0) {

[..]

> --- 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?

>           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;
>       }
>   



-- 
Best regards,
Vladimir