Libguestfs - Jul 2018 - Re: [PATCH v2] file: Normalize errno value for ENODEV

Fix issues Eric found in the original patch:
https://www.redhat.com/archives/libguestfs/2018-July/msg00072.html

- When handling ENODEV, the caller is expecting EOPNOTSUPP to trigger
  fallback.
- ENODEV should be ignored in file_trim.

Tested only on Fedora 28 and RHEL 7.5.
---
 plugins/file/file.c | 33 ++++++++++++++++++++++++---------
 1 file changed, 24 insertions(+), 9 deletions(-)

diff --git a/plugins/file/file.c b/plugins/file/file.c
index a7c07fb..a8a6253 100644
--- a/plugins/file/file.c
+++ b/plugins/file/file.c
@@ -50,6 +50,21 @@
 
 static char *filename = NULL;
 
+#if defined(FALLOC_FL_PUNCH_HOLE) || defined(FALLOC_FL_ZERO_RANGE)
+static int
+do_fallocate(int fd, int mode, off_t offset, off_t len)
+{
+  int r = -1;
+  r = fallocate (fd, mode, offset, len);
+  /* kernel 3.10 fails with ENODEV for block device. Kernel >= 4.9 fails
+     with EOPNOTSUPP in this case. Normalize errno to simplify callers. */
+  if (r == -1 && errno == ENODEV) {
+    errno = EOPNOTSUPP;
+  }
+  return r;
+}
+#endif
+
 static void
 file_unload (void)
 {
@@ -241,9 +256,9 @@ file_zero (void *handle, uint32_t count, uint64_t offset,
int may_trim)
 
 #ifdef FALLOC_FL_PUNCH_HOLE
   if (may_trim) {
-    r = fallocate (h->fd, FALLOC_FL_PUNCH_HOLE | FALLOC_FL_KEEP_SIZE,
-		   offset, count);
-    if (r == -1 && errno != EOPNOTSUPP && errno != ENODEV) {
+    r = do_fallocate (h->fd, FALLOC_FL_PUNCH_HOLE | FALLOC_FL_KEEP_SIZE,
+                      offset, count);
+    if (r == -1 && errno != EOPNOTSUPP) {
       nbdkit_error ("zero: %m");
     }
     /* PUNCH_HOLE is older; if it is not supported, it is likely that
@@ -253,8 +268,8 @@ file_zero (void *handle, uint32_t count, uint64_t offset,
int may_trim)
 #endif
 
 #ifdef FALLOC_FL_ZERO_RANGE
-  r = fallocate (h->fd, FALLOC_FL_ZERO_RANGE, offset, count);
-  if (r == -1 && errno != EOPNOTSUPP && errno != ENODEV) {
+  r = do_fallocate (h->fd, FALLOC_FL_ZERO_RANGE, offset, count);
+  if (r == -1 && errno != EOPNOTSUPP) {
     nbdkit_error ("zero: %m");
   }
 #else
@@ -288,11 +303,11 @@ file_trim (void *handle, uint32_t count, uint64_t offset)
   struct handle *h = handle;
 
   /* Trim is advisory; we don't care if it fails for anything other
-   * than EIO, EPERM, or ENODEV (kernel 3.10) */
-  r = fallocate (h->fd, FALLOC_FL_PUNCH_HOLE | FALLOC_FL_KEEP_SIZE,
-                 offset, count);
+   * than EIO or EPERM. */
+  r = do_fallocate (h->fd, FALLOC_FL_PUNCH_HOLE | FALLOC_FL_KEEP_SIZE,
+                    offset, count);
   if (r < 0) {
-    if (errno != EPERM && errno != EIO && errno != ENODEV) {
+    if (errno != EPERM && errno != EIO) {
       nbdkit_debug ("ignoring failed fallocate during trim: %m");
       r = 0;
     }
-- 
2.17.1

On Mon, Jul 30, 2018 at 8:11 PM Nir Soffer <nirsof@gmail.com> wrote:
> Fix issues Eric found in the original patch:
> https://www.redhat.com/archives/libguestfs/2018-July/msg00072.html
>
> - When handling ENODEV, the caller is expecting EOPNOTSUPP to trigger
>   fallback.
> - ENODEV should be ignored in file_trim.
>
> Tested only on Fedora 28 and RHEL 7.5.
> ---
>
I forgot to mention that v2 only fixes a typo on v1, that was here:
https://www.redhat.com/archives/libguestfs/2018-July/msg00077.html

>  plugins/file/file.c | 33 ++++++++++++++++++++++++---------
>  1 file changed, 24 insertions(+), 9 deletions(-)
>
> diff --git a/plugins/file/file.c b/plugins/file/file.c
> index a7c07fb..a8a6253 100644
> --- a/plugins/file/file.c
> +++ b/plugins/file/file.c
> @@ -50,6 +50,21 @@
>
>  static char *filename = NULL;
>
> +#if defined(FALLOC_FL_PUNCH_HOLE) || defined(FALLOC_FL_ZERO_RANGE)
> +static int
> +do_fallocate(int fd, int mode, off_t offset, off_t len)
> +{
> +  int r = -1;
> +  r = fallocate (fd, mode, offset, len);
> +  /* kernel 3.10 fails with ENODEV for block device. Kernel >= 4.9
fails
> +     with EOPNOTSUPP in this case. Normalize errno to simplify callers. */
> +  if (r == -1 && errno == ENODEV) {
> +    errno = EOPNOTSUPP;
> +  }
> +  return r;
> +}
> +#endif
> +
>  static void
>  file_unload (void)
>  {
> @@ -241,9 +256,9 @@ file_zero (void *handle, uint32_t count, uint64_t
> offset, int may_trim)
>
>  #ifdef FALLOC_FL_PUNCH_HOLE
>    if (may_trim) {
> -    r = fallocate (h->fd, FALLOC_FL_PUNCH_HOLE | FALLOC_FL_KEEP_SIZE,
> -                  offset, count);
> -    if (r == -1 && errno != EOPNOTSUPP && errno != ENODEV)
{
> +    r = do_fallocate (h->fd, FALLOC_FL_PUNCH_HOLE |
FALLOC_FL_KEEP_SIZE,
> +                      offset, count);
> +    if (r == -1 && errno != EOPNOTSUPP) {
>        nbdkit_error ("zero: %m");
>      }
>      /* PUNCH_HOLE is older; if it is not supported, it is likely that
> @@ -253,8 +268,8 @@ file_zero (void *handle, uint32_t count, uint64_t
> offset, int may_trim)
>  #endif
>
>  #ifdef FALLOC_FL_ZERO_RANGE
> -  r = fallocate (h->fd, FALLOC_FL_ZERO_RANGE, offset, count);
> -  if (r == -1 && errno != EOPNOTSUPP && errno != ENODEV) {
> +  r = do_fallocate (h->fd, FALLOC_FL_ZERO_RANGE, offset, count);
> +  if (r == -1 && errno != EOPNOTSUPP) {
>      nbdkit_error ("zero: %m");
>    }
>  #else
> @@ -288,11 +303,11 @@ file_trim (void *handle, uint32_t count, uint64_t
> offset)
>    struct handle *h = handle;
>
>    /* Trim is advisory; we don't care if it fails for anything other
> -   * than EIO, EPERM, or ENODEV (kernel 3.10) */
> -  r = fallocate (h->fd, FALLOC_FL_PUNCH_HOLE | FALLOC_FL_KEEP_SIZE,
> -                 offset, count);
> +   * than EIO or EPERM. */
> +  r = do_fallocate (h->fd, FALLOC_FL_PUNCH_HOLE | FALLOC_FL_KEEP_SIZE,
> +                    offset, count);
>    if (r < 0) {
> -    if (errno != EPERM && errno != EIO && errno != ENODEV)
{
> +    if (errno != EPERM && errno != EIO) {
>        nbdkit_debug ("ignoring failed fallocate during trim:
%m");
>        r = 0;
>      }
> --
> 2.17.1
>
>

On 07/30/2018 12:02 PM, Nir Soffer wrote:
> Fix issues Eric found in the original patch:
> https://www.redhat.com/archives/libguestfs/2018-July/msg00072.html
> 
> - When handling ENODEV, the caller is expecting EOPNOTSUPP to trigger
>    fallback.
> - ENODEV should be ignored in file_trim.
> 
> Tested only on Fedora 28 and RHEL 7.5.
> ---
>   plugins/file/file.c | 33 ++++++++++++++++++++++++---------
>   1 file changed, 24 insertions(+), 9 deletions(-)
> 
> +#if defined(FALLOC_FL_PUNCH_HOLE) || defined(FALLOC_FL_ZERO_RANGE)
> +static int
> +do_fallocate(int fd, int mode, off_t offset, off_t len)
> +{
> +  int r = -1;
> +  r = fallocate (fd, mode, offset, len);
Dead assignment to r in the declaration. Could merge these two lines 
into one.  Not necessarily worth a respin just for that.
> +  /* kernel 3.10 fails with ENODEV for block device. Kernel >= 4.9
fails
> +     with EOPNOTSUPP in this case. Normalize errno to simplify callers. */
Comment is slightly misleading - new enough kernels coupled with decent 
enough block device drivers actually succeed rather than failing. But 
I'm fine with checking in the comment as worded.

ACK

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

On Mon, Jul 30, 2018 at 9:23 PM Eric Blake <eblake@redhat.com> wrote:
> On 07/30/2018 12:02 PM, Nir Soffer wrote:
> > Fix issues Eric found in the original patch:
> > https://www.redhat.com/archives/libguestfs/2018-July/msg00072.html
> >
> > - When handling ENODEV, the caller is expecting EOPNOTSUPP to trigger
> >    fallback.
> > - ENODEV should be ignored in file_trim.
> >
> > Tested only on Fedora 28 and RHEL 7.5.
> > ---
> >   plugins/file/file.c | 33 ++++++++++++++++++++++++---------
> >   1 file changed, 24 insertions(+), 9 deletions(-)
> >
>
> > +#if defined(FALLOC_FL_PUNCH_HOLE) || defined(FALLOC_FL_ZERO_RANGE)
> > +static int
> > +do_fallocate(int fd, int mode, off_t offset, off_t len)
> > +{
> > +  int r = -1;
> > +  r = fallocate (fd, mode, offset, len);
>
> Dead assignment to r in the declaration. Could merge these two lines
> into one.  Not necessarily worth a respin just for that.
>
I tried to keep the style used in this file, but here it is indeed never
needed.

>
> > +  /* kernel 3.10 fails with ENODEV for block device. Kernel >= 4.9
fails
> > +     with EOPNOTSUPP in this case. Normalize errno to simplify
callers.
> */
>
> Comment is slightly misleading - new enough kernels coupled with decent
> enough block device drivers actually succeed rather than failing. But
> I'm fine with checking in the comment as worded.
>
The comment is only about the error flow. Moving it into the block would
avoid the confusion.

>
> ACK
>
> --
> Eric Blake, Principal Software Engineer
> Red Hat, Inc.           +1-919-301-3266 <(919)%20301-3266>
> Virtualization:  qemu.org | libvirt.org
>