Re: [Qemu-devel] [RFC V3 6/7] block: Create authorizations mechanism for external snapshots.

[Fam Zheng <famz@redhat.com>]

On 2013年12月03日 21:26, Benoît Canet wrote:
> ---
>   block.c                   | 64 +++++++++++++++++++++++++++++++++++++++++------
>   block/blkverify.c         |  2 +-
>   include/block/block.h     | 16 +++++++++---
>   include/block/block_int.h |  9 ++++---
>   4 files changed, 75 insertions(+), 16 deletions(-)
>
> diff --git a/block.c b/block.c
> index 8016ff2..0569cb2 100644
> --- a/block.c
> +++ b/block.c
> @@ -4945,21 +4945,69 @@ int bdrv_amend_options(BlockDriverState *bs, QEMUOptionParameter *options)
>       return bs->drv->bdrv_amend_options(bs, options);
>   }
>
> -ExtSnapshotPerm bdrv_check_ext_snapshot(BlockDriverState *bs)
> +/* will be used to recurse on single child block filter until first format
> + * (single child block filter will store their child in bs->file)
> + */
> +ExtSnapshotPerm bdrv_generic_check_ext_snapshot(BlockDriverState *bs,
> +                                                BlockDriverState *candidate)
>   {
> -    if (bs->drv->bdrv_check_ext_snapshot) {
> -        return bs->drv->bdrv_check_ext_snapshot(bs);
> +    if (!bs->drv) {
> +        return EXT_SNAPSHOT_FORBIDDEN;
>       }
>
> -    if (bs->file && bs->file->drv && bs->file->drv->bdrv_check_ext_snapshot) {
> -        return bs->file->drv->bdrv_check_ext_snapshot(bs);
> +    if (!bs->drv->authorizations[BS_CANT_SNAPSHOT]) {
> +        if (bs == candidate) {
> +             return EXT_SNAPSHOT_ALLOWED;
> +        } else {
> +             return EXT_SNAPSHOT_FORBIDDEN;
> +        }
>       }
>
> -    /* external snapshots are allowed by default */
> -    return EXT_SNAPSHOT_ALLOWED;
> +    if (!bs->drv->authorizations[BS_FILTER_PASS_DOWN]) {
> +        return EXT_SNAPSHOT_FORBIDDEN;
> +    }
> +
> +    if (!bs->file) {
> +        return EXT_SNAPSHOT_FORBIDDEN;
> +    }
> +
> +    return bdrv_recurse_check_ext_snapshot(bs->file, candidate);
>   }
>
> -ExtSnapshotPerm bdrv_check_ext_snapshot_forbidden(BlockDriverState *bs)
> +ExtSnapshotPerm bdrv_recurse_check_ext_snapshot(BlockDriverState *bs,
> +                                                BlockDriverState *candidate)
>   {
> +    if (bs->drv && bs->drv->bdrv_check_ext_snapshot) {
> +        return bs->drv->bdrv_check_ext_snapshot(bs, candidate);
> +    }
> +
> +    return bdrv_generic_check_ext_snapshot(bs, candidate);
> +}
> +
> +/* This function check if the candidate bs has snapshots authorized by going
> + * down the forest of bs, skipping filters and stopping on the the first bses
> + * authorizing snapshots
> + */
> +ExtSnapshotPerm bdrv_check_ext_snapshot(BlockDriverState *candidate)
> +{
> +    BlockDriverState *bs;
> +
> +    /* walk down the bs forest recursively */
> +    QTAILQ_FOREACH(bs, &bdrv_states, device_list) {
> +        ExtSnapshotPerm perm;
> +
> +        if (!bs->file) {
> +            continue;
> +        }
> +
> +        perm = bdrv_recurse_check_ext_snapshot(bs->file, candidate);
> +
> +        /* allowed in the right subtree -> stop here */
> +        if (perm == EXT_SNAPSHOT_ALLOWED) {
> +            return EXT_SNAPSHOT_ALLOWED;
> +        }
> +    }
> +
> +    /* external snapshots are forbidden by default */
>       return EXT_SNAPSHOT_FORBIDDEN;
>   }
> diff --git a/block/blkverify.c b/block/blkverify.c
> index e755e4e..b93017c 100644
> --- a/block/blkverify.c
> +++ b/block/blkverify.c
> @@ -313,7 +313,7 @@ static BlockDriver bdrv_blkverify = {
>       .bdrv_aio_writev        = blkverify_aio_writev,
>       .bdrv_aio_flush         = blkverify_aio_flush,
>
> -    .bdrv_check_ext_snapshot = bdrv_check_ext_snapshot_forbidden,
> +    .authorizations         = { true, false },
>   };
>
>   static void bdrv_blkverify_init(void)
> diff --git a/include/block/block.h b/include/block/block.h
> index 26c48e7..73c59fe 100644
> --- a/include/block/block.h
> +++ b/include/block/block.h
> @@ -280,16 +280,24 @@ int bdrv_amend_options(BlockDriverState *bs_new, QEMUOptionParameter *options);
>   /* external snapshots */
>
>   typedef enum {
> -    EXT_SNAPSHOT_ALLOWED,
>       EXT_SNAPSHOT_FORBIDDEN,
> +    EXT_SNAPSHOT_ALLOWED,
>   } ExtSnapshotPerm;
>
> +typedef enum {
> +    BS_CANT_SNAPSHOT,
> +    BS_FILTER_PASS_DOWN,
> +    BS_AUTHORIZATION_COUNT,
> +} BsAuthorization;
> +
>   /* return EXT_SNAPSHOT_ALLOWED if external snapshot is allowed
>    * return EXT_SNAPSHOT_FORBIDDEN if external snapshot is forbidden
>    */
> -ExtSnapshotPerm bdrv_check_ext_snapshot(BlockDriverState *bs);
> -/* helper used to forbid external snapshots like in blkverify */
> -ExtSnapshotPerm bdrv_check_ext_snapshot_forbidden(BlockDriverState *bs);
> +ExtSnapshotPerm bdrv_generic_check_ext_snapshot(BlockDriverState *bs,
> +                                                BlockDriverState *candidate);
> +ExtSnapshotPerm bdrv_recurse_check_ext_snapshot(BlockDriverState *bs,
> +                                                BlockDriverState *candidate);
> +ExtSnapshotPerm bdrv_check_ext_snapshot(BlockDriverState *candidate);
>
>   /* async block I/O */
>   typedef void BlockDriverDirtyHandler(BlockDriverState *bs, int64_t sector,
> diff --git a/include/block/block_int.h b/include/block/block_int.h
> index 9e789d2..d9704f2 100644
> --- a/include/block/block_int.h
> +++ b/include/block/block_int.h
> @@ -69,10 +69,13 @@ struct BlockDriver {
>       const char *format_name;
>       int instance_size;
>
> -    /* if not defined external snapshots are allowed
> -     * future block filters will query their children to build the response
> +    /* this table of boolean contains authorizations for the block operations */
> +    bool authorizations[BS_AUTHORIZATION_COUNT];

OK, I see some overlap of work here with my image fleecing patch series:

http://lists.gnu.org/archive/html/qemu-devel/2013-11/msg03694.html

The disadvantage of using a bool array is that it can't keep track of 
multiple points in code that want to forbid the same operation. So I 
think an array of int is better here.

And what do you think of my posted interface, does it works for you?

Fam

> +    /* future complex block filters will implement the following to query their
> +     * children to check if snapshoting is allowed on a bs of the graph
>        */
> -    ExtSnapshotPerm (*bdrv_check_ext_snapshot)(BlockDriverState *bs);
> +    ExtSnapshotPerm (*bdrv_check_ext_snapshot)(BlockDriverState *bs,
> +                                               BlockDriverState *candidate);
>
>       int (*bdrv_probe)(const uint8_t *buf, int buf_size, const char *filename);
>       int (*bdrv_probe_device)(const char *filename);
>