Re: [PATCH v5 08/45] block/snapshot: stress that we fallback to primary child

[Hanna Reitz <hreitz@redhat.com>]

On 30.03.22 23:28, Vladimir Sementsov-Ogievskiy wrote:
> Actually what we chose is a primary child. Let's stress it in the code.
>
> We are going to drop indirect pointer logic here in future. Actually
> this commit simplifies the future work: we drop use of indirection in
> the assertion now.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@openvz.org>
> ---
>   block/snapshot.c | 30 ++++++++++--------------------
>   1 file changed, 10 insertions(+), 20 deletions(-)
>
> diff --git a/block/snapshot.c b/block/snapshot.c
> index d6f53c3065..f4ec4f9ef3 100644
> --- a/block/snapshot.c
> +++ b/block/snapshot.c
> @@ -161,21 +161,14 @@ bool bdrv_snapshot_find_by_id_and_name(BlockDriverState *bs,
>   static BdrvChild **bdrv_snapshot_fallback_ptr(BlockDriverState *bs)
>   {
>       BdrvChild **fallback;
> -    BdrvChild *child;
> +    BdrvChild *child = bdrv_primary_child(bs);
>   
> -    /*
> -     * The only BdrvChild pointers that are safe to modify (and which
> -     * we can thus return a reference to) are bs->file and
> -     * bs->backing.
> -     */
> -    fallback = &bs->file;
> -    if (!*fallback && bs->drv && bs->drv->is_filter) {
> -        fallback = &bs->backing;
> -    }
> -
> -    if (!*fallback) {
> +    /* We allow fallback only to primary child */
> +    if (!child) {
>           return NULL;
>       }
> +    fallback = (child == bs->file ? &bs->file : &bs->backing);
> +    assert(*fallback == child);
>   
>       /*
>        * Check that there are no other children that would need to be
> @@ -309,15 +302,12 @@ int bdrv_snapshot_goto(BlockDriverState *bs,
>           }
>   
>           /*
> -         * fallback_ptr is &bs->file or &bs->backing.  *fallback_ptr
> -         * was closed above and set to NULL, but the .bdrv_open() call
> -         * has opened it again, because we set the respective option
> -         * (with the qdict_put_str() call above).
> -         * Assert that .bdrv_open() has attached some child on
> -         * *fallback_ptr, and that it has attached the one we wanted
> -         * it to (i.e., fallback_bs).
> +         * fallback was a primary child. It was closed above and set to NULL,
> +         * but the .bdrv_open() call has opened it again, because we set the
> +         * respective option (with the qdict_put_str() call above).
> +         * Assert that .bdrv_open() has attached some BDS as primary child.

s/some/the right/?

Reviewed-by: Hanna Reitz <hreitz@redhat.com>

>            */
> -        assert(*fallback_ptr && fallback_bs == (*fallback_ptr)->bs);
> +        assert(bdrv_primary_bs(bs) == fallback_bs);
>           bdrv_unref(fallback_bs);
>           return ret;
>       }