Re: [PATCH v2 34/36] block: refactor bdrv_child_set_perm_safe() transaction action

[Kevin Wolf]

Am 27.11.2020 um 15:45 hat Vladimir Sementsov-Ogievskiy geschrieben:
> Old interfaces dropped, nobody directly calls
> bdrv_child_set_perm_abort() and bdrv_child_set_perm_commit(), so we can
> use personal state structure for the action and stop exploiting
> BdrvChild structure. Also, drop "_safe" suffix which is redundant now.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

> diff --git a/block.c b/block.c
> index 3093d20db8..1fde22e4f4 100644
> --- a/block.c
> +++ b/block.c
> @@ -2070,59 +2070,40 @@ static GSList *bdrv_topological_dfs(GSList *list, 
> GHashTable *found,
>      return g_slist_prepend(list, bs);
>  }
>  
> -static void bdrv_child_set_perm_commit(void *opaque)
> -{
> -    BdrvChild *c = opaque;
> -
> -    c->has_backup_perm = false;
> -}
> +typedef struct BdrvChildSetPermState {
> +    BdrvChild *child;
> +    uint64_t old_perm;
> +    uint64_t old_shared_perm;
> +} BdrvChildSetPermState;
>  
>  static void bdrv_child_set_perm_abort(void *opaque)
>  {
> -    BdrvChild *c = opaque;
> -    /*
> -     * We may have child->has_backup_perm unset at this point, as in case of
> -     * _check_ stage of permission update failure we may _check_ not the 
> whole
> -     * subtree.  Still, _abort_ is called on the whole subtree anyway.
> -     */
> -    if (c->has_backup_perm) {
> -        c->perm = c->backup_perm;
> -        c->shared_perm = c->backup_shared_perm;
> -        c->has_backup_perm = false;
> -    }
> +    BdrvChildSetPermState *s = opaque;
> +
> +    s->child->perm = s->old_perm;
> +    s->child->shared_perm = s->old_shared_perm;
>  }

Ah, so this patch actually implements what I had asked for somewhere at
the start of the series.

Don't bother changing it earlier then. As long as it's in the same
series, this is fine.

Kevin