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]) {

This double negative feels hard to read for me.

> +        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);
> +    }

Maybe I'm missing something, but if a driver always returns positive 
permit, despite of what candidate is (or even it's relevant to bs), then 
doesn't it also affect other devices? because...

> +
> +    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) {

this iterates through all the known graph trees (device_list), instead 
of limiting to only the device that candidate belongs to.

Why not just check candidate's permission bitmap and go down from it? If 
an ancestor need to disable its descendants, it could simply set 
permission bits of its children and recurse down.

Fam