Re: [PATCH v5 14/45] block/snapshot: drop indirection around bdrv_snapshot_fallback_ptr

[Hanna Reitz]

On 30.03.22 23:28, Vladimir Sementsov-Ogievskiy wrote:
> Now the indirection is not actually used, we can safely reduce it to
> simple pointer.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@openvz.org>
> ---
>   block/snapshot.c | 39 +++++++++++++++++----------------------
>   1 file changed, 17 insertions(+), 22 deletions(-)

Looks good, just wondering whether we should drop some of the "_ptr" 
suffixes now.

> diff --git a/block/snapshot.c b/block/snapshot.c
> index 02a880911f..4eb9258de6 100644
> --- a/block/snapshot.c
> +++ b/block/snapshot.c
> @@ -151,34 +151,29 @@ bool bdrv_snapshot_find_by_id_and_name(BlockDriverState 
> *bs,
>   }
>   
>   /**
> - * Return a pointer to the child BDS pointer to which we can fall
> + * Return a pointer to child of given BDS to which we can fall
>    * back if the given BDS does not support snapshots.
>    * Return NULL if there is no BDS to (safely) fall back to.
> - *
> - * We need to return an indirect pointer because bdrv_snapshot_goto()
> - * has to modify the BdrvChild pointer.
>    */
> -static BdrvChild **bdrv_snapshot_fallback_ptr(BlockDriverState *bs)
> +static BdrvChild *bdrv_snapshot_fallback_ptr(BlockDriverState *bs)

The _ptr part was meant to point out that it returns an indirect 
pointer; maybe we should name it bdrv_snapshot_fallback_child() now?

>   {
> -    BdrvChild **fallback;
> -    BdrvChild *child = bdrv_primary_child(bs);
> +    BdrvChild *fallback = bdrv_primary_child(bs);
> +    BdrvChild *child;
>   
>       /* We allow fallback only to primary child */
> -    if (!child) {
> +    if (!fallback) {
>           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
>        * snapshotted.  If there are, it is not safe to fall back to
> -     * *fallback.
> +     * fallback.
>        */
>       QLIST_FOREACH(child, &bs->children, next) {
>           if (child->role & (BDRV_CHILD_DATA | BDRV_CHILD_METADATA |
>                              BDRV_CHILD_FILTERED) &&
> -            child != *fallback)
> +            child != fallback)
>           {
>               return NULL;
>           }
> @@ -189,8 +184,8 @@ static BdrvChild 
> **bdrv_snapshot_fallback_ptr(BlockDriverState *bs)
>   
>   static BlockDriverState *bdrv_snapshot_fallback(BlockDriverState *bs)
>   {
> -    BdrvChild **child_ptr = bdrv_snapshot_fallback_ptr(bs);

Just "child" is enough (and better) now, I think.

> -    return child_ptr ? (*child_ptr)->bs : NULL;
> +    BdrvChild *child_ptr = bdrv_snapshot_fallback_ptr(bs);
> +    return child_ptr ? child_ptr->bs : NULL;
>   }
>   
>   int bdrv_can_snapshot(BlockDriverState *bs)