Re: [Qemu-devel] [RFC PATCH 06/41] block: Involve block drivers in permission granting

[Fam Zheng <famz@redhat.com>]

On Mon, 02/13 18:22, Kevin Wolf wrote:
> +int bdrv_child_try_set_perm(BdrvChild *c, uint64_t perm, uint64_t shared,
> +                            Error **errp)
> +{
> +    int ret;
> +
> +    ret = bdrv_child_check_perm(c, perm, shared, errp);
> +    if (ret < 0) {
> +        return ret;
> +    }
> +
> +    bdrv_child_set_perm(c, perm, shared);

This has an issue of TOCTOU, which means image locking cannot fit in easily.
Maybe squash them into one callback (.bdrv_try_set_perm) that can return error?

> +
>      return 0;
>  }
>  
> @@ -1322,6 +1445,7 @@ static void bdrv_replace_child(BdrvChild *child, BlockDriverState *new_bs)
>              child->role->drained_end(child);
>          }
>          QLIST_REMOVE(child, next_parent);
> +        bdrv_update_perm(old_bs);
>      }
>  
>      child->bs = new_bs;
> @@ -1331,6 +1455,7 @@ static void bdrv_replace_child(BdrvChild *child, BlockDriverState *new_bs)
>          if (new_bs->quiesce_counter && child->role->drained_begin) {
>              child->role->drained_begin(child);
>          }
> +        bdrv_update_perm(new_bs);
>      }
>  }
>  
> diff --git a/include/block/block_int.h b/include/block/block_int.h
> index f36b064..8578e17 100644
> --- a/include/block/block_int.h
> +++ b/include/block/block_int.h
> @@ -320,6 +320,45 @@ struct BlockDriver {
>      void (*bdrv_del_child)(BlockDriverState *parent, BdrvChild *child,
>                             Error **errp);
>  
> +    /**
> +     * Checks whether the requested set of cumulative permissions in @perm
> +     * can be granted for accessing @bs and whether no other users are using
> +     * permissions other than those given in @shared (both arguments take
> +     * BLK_PERM_* bitmasks).
> +     *
> +     * If both conditions are met, 0 is returned. Otherwise, -errno is returned
> +     * and errp is set to an error describing the conflict.
> +     */
> +    int (*bdrv_check_perm)(BlockDriverState *bs, uint64_t perm,
> +                           uint64_t shared, Error **errp);
> +
> +    /**
> +     * Called to inform the driver that the set of cumulative set of used
> +     * permissions for @bs has changed to @perm, and the set of sharable
> +     * permission to @shared. The driver can use this to propagate changes to
> +     * its children (i.e. request permissions only if a parent actually needs
> +     * them).
> +     *
> +     * If permissions are added to @perm or dropped from @shared, callers must
> +     * use bdrv_check_perm() first to ensure that this operation is valid.
> +     * Dropping from @perm or adding to @shared is always allowed without a
> +     * previous check.
> +     */
> +    void (*bdrv_set_perm)(BlockDriverState *bs, uint64_t perm, uint64_t shared);
> +
> +    /**
> +     * Returns in @nperm and @nshared the permissions that the driver for @bs
> +     * needs on its child @c, based on the cumulative permissions requested by
> +     * the parents in @parent_perm and @parent_shared.
> +     *
> +     * If @c is NULL, return the permissions for attaching a new child for the
> +     * given @role.
> +     */
> +     void (*bdrv_child_perm)(BlockDriverState* bs, BdrvChild *c,
> +                             const BdrvChildRole *role,
> +                             uint64_t parent_perm, uint64_t parent_shared,
> +                             uint64_t *nperm, uint64_t *nshared);
> +
>      QLIST_ENTRY(BlockDriver) list;
>  };
>  
> @@ -832,6 +871,13 @@ BdrvChild *bdrv_root_attach_child(BlockDriverState *child_bs,
>                                    void *opaque, Error **errp);
>  void bdrv_root_unref_child(BdrvChild *child);
>  
> +int bdrv_child_check_perm(BdrvChild *c, uint64_t perm, uint64_t shared,
> +                          Error **errp);
> +void bdrv_child_set_perm(BdrvChild *c, uint64_t perm, uint64_t shared);
> +int bdrv_child_try_set_perm(BdrvChild *c, uint64_t perm, uint64_t shared,
> +                            Error **errp);
> +
> +
>  const char *bdrv_get_parent_name(const BlockDriverState *bs);
>  void blk_dev_change_media_cb(BlockBackend *blk, bool load);
>  bool blk_dev_has_removable_media(BlockBackend *blk);
> -- 
> 1.8.3.1
>