[PATCH v2 0/3] block/export/fuse: Fix build failure on FreeBSD

[PATCH v2 0/3] block/export/fuse: Fix build failure on FreeBSD

[Philippe Mathieu-Daudé via]

Since v1:
- Extract fuse_allocate_*() to avoid mixing #ifdef'ry with if/else

Philippe Mathieu-Daudé (3):
  block/export/fuse: Extract fuse_fallocate_punch_hole()
  block/export/fuse: Extract fuse_fallocate_zero_range()
  block/export/fuse: Fix build failure on FreeBSD

 block/export/fuse.c | 109 +++++++++++++++++++++++++++-----------------
 1 file changed, 68 insertions(+), 41 deletions(-)

-- 
2.34.1

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

[Philippe Mathieu-Daudé via]

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);
-    }
-#endif /* CONFIG_FALLOCATE_ZERO_RANGE */
-    else if (!mode) {
+    } else if (!fuse_fallocate_zero_range(req, inode, blk_len, mode, offset, &length)) {
+        return;
+    } else if (!mode) {
         /* We can only fallocate at the EOF with a truncate */
         if (offset < blk_len) {
             fuse_reply_err(req, EOPNOTSUPP);
-- 
2.34.1

[PATCH v2 2/3] block/export/fuse: Extract fuse_fallocate_zero_range()

[Philippe Mathieu-Daudé via]

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 block/export/fuse.c | 48 +++++++++++++++++++++++++++++----------------
 1 file changed, 31 insertions(+), 17 deletions(-)

diff --git a/block/export/fuse.c b/block/export/fuse.c
index 31cb0503adc..3a158342c75 100644
--- a/block/export/fuse.c
+++ b/block/export/fuse.c
@@ -603,6 +603,35 @@ static void fuse_write(fuse_req_t req, fuse_ino_t inode, const char *buf,
     }
 }
 
+static bool fuse_fallocate_punch_hole(fuse_req_t req, fuse_ino_t inode,
+                                      int mode, int64_t blk_len,
+                                      off_t offset, off_t *length)
+{
+    FuseExport *exp = fuse_req_userdata(req);
+
+    if (mode & FALLOC_FL_KEEP_SIZE) {
+        *length = MIN(*length, blk_len - offset);
+    }
+
+    if (mode & FALLOC_FL_PUNCH_HOLE) {
+        int ret;
+
+        if (!(mode & FALLOC_FL_KEEP_SIZE)) {
+            fuse_reply_err(req, EINVAL);
+            return false;
+        }
+
+        do {
+            int size = MIN(*length, BDRV_REQUEST_MAX_BYTES);
+
+            ret = blk_pdiscard(exp->common.blk, offset, size);
+            offset += size;
+            *length -= size;
+        } while (ret == 0 && *length > 0);
+    }
+    return true;
+}
+
 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)
@@ -657,23 +686,8 @@ static void fuse_fallocate(fuse_req_t req, fuse_ino_t inode, int mode,
         return;
     }
 
-    if (mode & FALLOC_FL_KEEP_SIZE) {
-        length = MIN(length, blk_len - offset);
-    }
-
-    if (mode & FALLOC_FL_PUNCH_HOLE) {
-        if (!(mode & FALLOC_FL_KEEP_SIZE)) {
-            fuse_reply_err(req, EINVAL);
-            return;
-        }
-
-        do {
-            int size = MIN(length, BDRV_REQUEST_MAX_BYTES);
-
-            ret = blk_pdiscard(exp->common.blk, offset, size);
-            offset += size;
-            length -= size;
-        } while (ret == 0 && length > 0);
+    if (!fuse_fallocate_punch_hole(req, inode, mode, blk_len, offset, &length)) {
+        return;
     } else if (!fuse_fallocate_zero_range(req, inode, blk_len, mode, offset, &length)) {
         return;
     } else if (!mode) {
-- 
2.34.1

[PATCH v2 3/3] block/export/fuse: Fix build failure on FreeBSD

[Philippe Mathieu-Daudé via]

When building on FreeBSD we get:

  [816/6851] Compiling C object libblockdev.fa.p/block_export_fuse.c.o
  ../block/export/fuse.c:612:16: error: use of undeclared identifier 'FALLOC_FL_KEEP_SIZE'
      if (mode & FALLOC_FL_KEEP_SIZE) {
                 ^
  ../block/export/fuse.c:616:16: error: use of undeclared identifier 'FALLOC_FL_PUNCH_HOLE'
      if (mode & FALLOC_FL_PUNCH_HOLE) {
                 ^
  ../block/export/fuse.c:619:22: error: use of undeclared identifier 'FALLOC_FL_KEEP_SIZE'
          if (!(mode & FALLOC_FL_KEEP_SIZE)) {
                       ^
  3 errors generated.
  FAILED: libblockdev.fa.p/block_export_fuse.c.o

Meson indeed reported FALLOC_FL_PUNCH_HOLE is not available:

  C compiler for the host machine: cc (clang 10.0.1 "FreeBSD clang version 10.0.1")
  Checking for function "fallocate" : NO
  Checking for function "posix_fallocate" : YES
  Header <linux/falloc.h> has symbol "FALLOC_FL_PUNCH_HOLE" : NO
  Header <linux/falloc.h> has symbol "FALLOC_FL_ZERO_RANGE" : NO
  ...

Similarly to commit 304332039 ("block/export/fuse.c: fix musl build"),
guard the code requiring FALLOC_FL_KEEP_SIZE / FALLOC_FL_PUNCH_HOLE
definitions under CONFIG_FALLOCATE_PUNCH_HOLE #ifdef'ry.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 block/export/fuse.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/block/export/fuse.c b/block/export/fuse.c
index 3a158342c75..4c5d662be87 100644
--- a/block/export/fuse.c
+++ b/block/export/fuse.c
@@ -607,6 +607,7 @@ static bool fuse_fallocate_punch_hole(fuse_req_t req, fuse_ino_t inode,
                                       int mode, int64_t blk_len,
                                       off_t offset, off_t *length)
 {
+#ifdef CONFIG_FALLOCATE_PUNCH_HOLE
     FuseExport *exp = fuse_req_userdata(req);
 
     if (mode & FALLOC_FL_KEEP_SIZE) {
@@ -629,6 +630,7 @@ static bool fuse_fallocate_punch_hole(fuse_req_t req, fuse_ino_t inode,
             *length -= size;
         } while (ret == 0 && *length > 0);
     }
+#endif /* CONFIG_FALLOCATE_PUNCH_HOLE */
     return true;
 }
 
-- 
2.34.1

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

[Thomas Huth]

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?

  Thomas


> -    }
> -#endif /* CONFIG_FALLOCATE_ZERO_RANGE */
> -    else if (!mode) {
> +    } else if (!fuse_fallocate_zero_range(req, inode, blk_len, mode, offset, &length)) {
> +        return;
> +    } else if (!mode) {
>           /* We can only fallocate at the EOF with a truncate */
>           if (offset < blk_len) {
>               fuse_reply_err(req, EOPNOTSUPP);

Re: [PATCH v2 2/3] block/export/fuse: Extract fuse_fallocate_zero_range()

[Thomas Huth]

On 24/01/2022 23.03, Philippe Mathieu-Daudé via wrote:
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>   block/export/fuse.c | 48 +++++++++++++++++++++++++++++----------------
>   1 file changed, 31 insertions(+), 17 deletions(-)
> 
> diff --git a/block/export/fuse.c b/block/export/fuse.c
> index 31cb0503adc..3a158342c75 100644
> --- a/block/export/fuse.c
> +++ b/block/export/fuse.c
> @@ -603,6 +603,35 @@ static void fuse_write(fuse_req_t req, fuse_ino_t inode, const char *buf,
>       }
>   }
>   
> +static bool fuse_fallocate_punch_hole(fuse_req_t req, fuse_ino_t inode,
> +                                      int mode, int64_t blk_len,
> +                                      off_t offset, off_t *length)
> +{
> +    FuseExport *exp = fuse_req_userdata(req);
> +
> +    if (mode & FALLOC_FL_KEEP_SIZE) {
> +        *length = MIN(*length, blk_len - offset);
> +    }
> +
> +    if (mode & FALLOC_FL_PUNCH_HOLE) {
> +        int ret;
> +
> +        if (!(mode & FALLOC_FL_KEEP_SIZE)) {
> +            fuse_reply_err(req, EINVAL);
> +            return false;
> +        }
> +
> +        do {
> +            int size = MIN(*length, BDRV_REQUEST_MAX_BYTES);
> +
> +            ret = blk_pdiscard(exp->common.blk, offset, size);
> +            offset += size;
> +            *length -= size;
> +        } while (ret == 0 && *length > 0);
> +    }
> +    return true;
> +}
> +
>   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)
> @@ -657,23 +686,8 @@ static void fuse_fallocate(fuse_req_t req, fuse_ino_t inode, int mode,
>           return;
>       }
>   
> -    if (mode & FALLOC_FL_KEEP_SIZE) {
> -        length = MIN(length, blk_len - offset);
> -    }
> -
> -    if (mode & FALLOC_FL_PUNCH_HOLE) {
> -        if (!(mode & FALLOC_FL_KEEP_SIZE)) {
> -            fuse_reply_err(req, EINVAL);
> -            return;
> -        }
> -
> -        do {
> -            int size = MIN(length, BDRV_REQUEST_MAX_BYTES);
> -
> -            ret = blk_pdiscard(exp->common.blk, offset, size);
> -            offset += size;
> -            length -= size;
> -        } while (ret == 0 && length > 0);
> +    if (!fuse_fallocate_punch_hole(req, inode, mode, blk_len, offset, &length)) {
> +        return;

Same issue as with the previous patch? If the do-while loop has been 
executed, the else branches below should not be called...

  Thomas

>       } else if (!fuse_fallocate_zero_range(req, inode, blk_len, mode, offset, &length)) {
>           return;
>       } else if (!mode) {

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

[Philippe Mathieu-Daudé via]

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.