Re: [PATCH v2 1/3] block/export/fuse: Extract fuse_fallocate_punch_hole()

[Philippe Mathieu-Daudé]

On 1/25/22 12:10, Thomas Huth wrote:
> On 24/01/2022 23.03, Philippe Mathieu-Daudé via wrote:
>> Extract fuse_fallocate_punch_hole() to avoid #ifdef'ry
>> mixed within if/else statement.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>> ---
>>   block/export/fuse.c | 59 +++++++++++++++++++++++++++------------------
>>   1 file changed, 35 insertions(+), 24 deletions(-)
>>
>> diff --git a/block/export/fuse.c b/block/export/fuse.c
>> index 6710d8aed86..31cb0503adc 100644
>> --- a/block/export/fuse.c
>> +++ b/block/export/fuse.c
>> @@ -603,6 +603,38 @@ static void fuse_write(fuse_req_t req, fuse_ino_t
>> inode, const char *buf,
>>       }
>>   }
>>   +static bool fuse_fallocate_zero_range(fuse_req_t req, fuse_ino_t
>> inode,
>> +                                      int mode, int64_t blk_len,
>> +                                      off_t offset, off_t *length)
>> +{
>> +#ifdef CONFIG_FALLOCATE_ZERO_RANGE
>> +    FuseExport *exp = fuse_req_userdata(req);
>> +
>> +    if (mode & FALLOC_FL_ZERO_RANGE) {
>> +        int ret;
>> +
>> +       if (!(mode & FALLOC_FL_KEEP_SIZE) && offset + *length >
>> blk_len) {
>> +            /* No need for zeroes, we are going to write them
>> ourselves */
>> +            ret = fuse_do_truncate(exp, offset + *length, false,
>> +                                   PREALLOC_MODE_OFF);
>> +            if (ret < 0) {
>> +                fuse_reply_err(req, -ret);
>> +                return false;
>> +            }
>> +        }
>> +
>> +        do {
>> +            int size = MIN(*length, BDRV_REQUEST_MAX_BYTES);
>> +
>> +            ret = blk_pwrite_zeroes(exp->common.blk, offset, size, 0);
>> +            offset += size;
>> +            *length -= size;
>> +        } while (ret == 0 && *length > 0);
>> +    }
>> +#endif /* CONFIG_FALLOCATE_ZERO_RANGE */
>> +    return true;
>> +}
>> +
>>   /**
>>    * Let clients perform various fallocate() operations.
>>    */
>> @@ -642,30 +674,9 @@ static void fuse_fallocate(fuse_req_t req,
>> fuse_ino_t inode, int mode,
>>               offset += size;
>>               length -= size;
>>           } while (ret == 0 && length > 0);
>> -    }
>> -#ifdef CONFIG_FALLOCATE_ZERO_RANGE
>> -    else if (mode & FALLOC_FL_ZERO_RANGE) {
>> -        if (!(mode & FALLOC_FL_KEEP_SIZE) && offset + length >
>> blk_len) {
>> -            /* No need for zeroes, we are going to write them
>> ourselves */
>> -            ret = fuse_do_truncate(exp, offset + length, false,
>> -                                   PREALLOC_MODE_OFF);
>> -            if (ret < 0) {
>> -                fuse_reply_err(req, -ret);
>> -                return;
>> -            }
>> -        }
>> -
>> -        do {
>> -            int size = MIN(length, BDRV_REQUEST_MAX_BYTES);
>> -
>> -            ret = blk_pwrite_zeroes(exp->common.blk,
>> -                                    offset, size, 0);
>> -            offset += size;
>> -            length -= size;
>> -        } while (ret == 0 && length > 0);
> 
> I might not have enough coffee today yet, but I think your patch is
> wrong: If the code executed this do-while loop/if-statement-branch, the
> other else-statements below were never called. Now with your patch, if
> the do-while loop in fuse_fallocate_zero_range() is called, the function
> will return with "true" at the end, causing the other else-statements
> below to be called, so that ret finally gets set to -EOPNOTSUPP. Or did
> I miss something?

You are right, this patch is crap, sorry.