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

[Emanuele Giuseppe Esposito]

On 25/01/2022 17:51, Hanna Reitz wrote:
> 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.
> 

I will then just remove the last line in the commit message ("since ..
blk_set_perm.").

Thank you,
Emanuele

> 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;
>