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 <mre...@redhat.com>
[..]
> --- 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
