Re: [PATCH 10/21] block/copy-before-write: relax permission requirements when no parents

[Max Reitz]

On 17.05.21 08:44, Vladimir Sementsov-Ogievskiy wrote:
> We are going to publish copy-before-write filter. So, user should be
> able to create it with blockdev-add first, specifying both filtered and
> target children. And then do blockdev-reopen, to actually insert the
> filter where needed.
>
> Currently, filter unshares write permission unconditionally on source
> node. It's good, but it will not allow to do blockdev-add. So, let's
> relax restrictions when filter doesn't have any parent.
>
> Test output is modified, as now permission conflict happens only when
> job creates a blk parent for filter node.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>   block/copy-before-write.c  | 8 +++++---
>   tests/qemu-iotests/283.out | 2 +-
>   2 files changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/block/copy-before-write.c b/block/copy-before-write.c
> index 4776172f77..af2bb97a30 100644
> --- a/block/copy-before-write.c
> +++ b/block/copy-before-write.c
> @@ -142,10 +142,12 @@ static void cbw_child_perm(BlockDriverState *bs, 
> BdrvChild *c,
>           bdrv_default_perms(bs, c, role, reopen_queue,
>                              perm, shared, nperm, nshared);
>   
> -        if (perm & BLK_PERM_WRITE) {
> -            *nperm = *nperm | BLK_PERM_CONSISTENT_READ;
> +        if (!QLIST_EMPTY(&bs->parents)) {

I understand this works because with the transactional system, at the 
time the permissions are checked, the graph has already been changed, yes?

I was wondering still whether there was any way to express this through 
permissions alone.  I guess we could check
“perm & BLK_PERM_CONSISTENT_READ”, but that would actually be just a 
crutch to see whether there are any parents.  I suppose this really is 
about whether there are parents or not.  So:

Reviewed-by: Max Reitz <mreitz@redhat.com>

> +            if (perm & BLK_PERM_WRITE) {
> +                *nperm = *nperm | BLK_PERM_CONSISTENT_READ;
> +            }
> +            *nshared &= ~(BLK_PERM_WRITE | BLK_PERM_RESIZE);
>           }
> -        *nshared &= ~(BLK_PERM_WRITE | BLK_PERM_RESIZE);
>       }
>   }
>   
> diff --git a/tests/qemu-iotests/283.out b/tests/qemu-iotests/283.out
> index e08f807dab..d5350ce7a7 100644
> --- a/tests/qemu-iotests/283.out
> +++ b/tests/qemu-iotests/283.out
> @@ -5,7 +5,7 @@
>   {"execute": "blockdev-add", "arguments": {"driver": "blkdebug", "image": "base", "node-name": 
> "other", "take-child-perms": ["write"]}}
>   {"return": {}}
>   {"execute": "blockdev-backup", "arguments": {"device": "source", "sync": "full", 
> "target": "target"}}
> -{"error": {"class": "GenericError", "desc": "Cannot append copy-before-write 
> filter: Conflicts with use by source as 'image', which does not allow 'write' on base"}}
> +{"error": {"class": "GenericError", "desc": "Conflicts with use by source as 
> 'image', which does not allow 'write' on base"}}
>   
>   === copy-before-write filter should be gone after job-finalize ===
>