Re: [PATCH v6 04/33] block/export/fuse.c: allow writable exports to take RESIZE permission

[Hanna Reitz <hreitz@redhat.com>]

On 21.01.22 18:05, Emanuele Giuseppe Esposito wrote:
> Allow writable exports to get BLK_PERM_RESIZE permission
> from creation, in fuse_export_create().
> In this way, there is no need to give the permission in
> fuse_do_truncate(), which might be run in an iothread.
>
> Permissions should be set only in the main thread, so
> in any case if an iothread tries to set RESIZE, it will
> be blocked.
>
> Also assert in fuse_do_truncate that if we give the
> RESIZE permission we can then restore the original ones,
> since we don't check the return value of blk_set_perm.

We do, because we pass &error_abort for errp, so if an error were to 
occur, qemu would abort.

Not that I mind adding an assertion on the return value, just noting 
that we omitted that kind of intentionally.

Reviewed-by: Hanna Reitz <hreitz@redhat.com>

> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
> ---
>   block/export/fuse.c | 25 ++++++++++++++++++-------
>   1 file changed, 18 insertions(+), 7 deletions(-)
>
> diff --git a/block/export/fuse.c b/block/export/fuse.c
> index 823c126d23..3c177b9e67 100644
> --- a/block/export/fuse.c
> +++ b/block/export/fuse.c
> @@ -86,8 +86,8 @@ static int fuse_export_create(BlockExport *blk_exp,
>   
>       assert(blk_exp_args->type == BLOCK_EXPORT_TYPE_FUSE);
>   
> -    /* For growable exports, take the RESIZE permission */
> -    if (args->growable) {
> +    /* For growable and writable exports, take the RESIZE permission */
> +    if (args->growable || blk_exp_args->writable) {
>           uint64_t blk_perm, blk_shared_perm;
>   
>           blk_get_perm(exp->common.blk, &blk_perm, &blk_shared_perm);
> @@ -392,14 +392,23 @@ static int fuse_do_truncate(const FuseExport *exp, int64_t size,
>   {
>       uint64_t blk_perm, blk_shared_perm;
>       BdrvRequestFlags truncate_flags = 0;
> -    int ret;
> +    bool add_resize_perm;
> +    int ret, ret_check;
> +
> +    /* Growable and writable exports have a permanent RESIZE permission */
> +    add_resize_perm = !exp->growable && !exp->writable;
>   
>       if (req_zero_write) {
>           truncate_flags |= BDRV_REQ_ZERO_WRITE;
>       }
>   
> -    /* Growable exports have a permanent RESIZE permission */
> -    if (!exp->growable) {
> +    if (add_resize_perm) {
> +
> +        if (!qemu_in_main_thread()) {
> +            /* Changing permissions like below only works in the main thread */
> +            return -EPERM;
> +        }
> +
>           blk_get_perm(exp->common.blk, &blk_perm, &blk_shared_perm);
>   
>           ret = blk_set_perm(exp->common.blk, blk_perm | BLK_PERM_RESIZE,
> @@ -412,9 +421,11 @@ static int fuse_do_truncate(const FuseExport *exp, int64_t size,
>       ret = blk_truncate(exp->common.blk, size, true, prealloc,
>                          truncate_flags, NULL);
>   
> -    if (!exp->growable) {
> +    if (add_resize_perm) {
>           /* Must succeed, because we are only giving up the RESIZE permission */
> -        blk_set_perm(exp->common.blk, blk_perm, blk_shared_perm, &error_abort);
> +        ret_check = blk_set_perm(exp->common.blk, blk_perm,
> +                                 blk_shared_perm, &error_abort);
> +        assert(ret_check == 0);
>       }
>   
>       return ret;