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

[Fam Zheng <famz@redhat.com>]

On 2013年12月04日 13:20, Benoît Canet wrote:
> Le Wednesday 04 Dec 2013 à 11:47:22 (+0800), Fam Zheng a écrit :
>> 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.
>
> The recursion termination success is candidate == bs.
> This make sure that the scan of the other tree of the forest will not return any
> spurious success.
>

But the "candidate == bs" check is in bdrv_generic_check_ext_snapshot, 
which gets short-circuited by driver implementation if the driver 
implements it, in bdrv_recurse_check_ext_snapshot.

So if I have an "always yes" drv->bdrv_check_ext_snapshot and it happens 
to be the first one in bdrv_states, I will allow all snapshot operations.

Fam