Libguestfs - Apr 2019 - Re: [PATCH nbdkit 2/2] filters: Be careful to set *err if nbdkit_add_extent or nbdkit_extents_new fail.

This fix isn't exhaustive but it fixes some obvious problems in the
filters.

Rich.

errno is currently set to a suitable value if nbdkit_extents_new
returns an error, but this was not documented so I added that
documentation.

Set errno to a suitable value if nbdkit_add_extent returns an error,
and also document that.

Thanks: Eric Blake for spotting the problem.
---
 docs/nbdkit-filter.pod | 3 ++-
 docs/nbdkit-plugin.pod | 2 +-
 server/extents.c       | 2 ++
 3 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/docs/nbdkit-filter.pod b/docs/nbdkit-filter.pod
index 9e51c68..6aeaa7b 100644
--- a/docs/nbdkit-filter.pod
+++ b/docs/nbdkit-filter.pod
@@ -571,7 +571,8 @@ the end, but for filters which adjust offsets, they should
pass in the
 adjusted offset.
 
 On error this function can return C<NULL>.  In this case it calls
-C<nbdkit_error> and/or C<nbdkit_set_error> as required.
+C<nbdkit_error> and/or C<nbdkit_set_error> as required. 
C<errno> will
+be set to a suitable value.
 
  void nbdkit_extents_free (struct nbdkit_extents *);
 
diff --git a/docs/nbdkit-plugin.pod b/docs/nbdkit-plugin.pod
index 272ec67..e9dc34f 100644
--- a/docs/nbdkit-plugin.pod
+++ b/docs/nbdkit-plugin.pod
@@ -812,7 +812,7 @@ guarantee that trimmed blocks read back as zeroes.
 
 C<nbdkit_extent_add> returns C<0> on success or C<-1> on
failure.  On
 failure C<nbdkit_error> and/or C<nbdkit_set_error> has already been
-called.
+called.  C<errno> will be set to a suitable value.
 
 =head1 THREADS
 
diff --git a/server/extents.c b/server/extents.c
index 105c4a7..d3d1a15 100644
--- a/server/extents.c
+++ b/server/extents.c
@@ -154,6 +154,7 @@ nbdkit_add_extent (struct nbdkit_extents *exts,
     nbdkit_error ("nbdkit_add_extent: "
                   "extents must be added in ascending order and "
                   "must be contiguous");
+    errno = ERANGE;
     return -1;
   }
   exts->next = offset + length;
@@ -186,6 +187,7 @@ nbdkit_add_extent (struct nbdkit_extents *exts,
       nbdkit_error ("nbdkit_add_extent: "
                     "first extent must not be > start (%" PRIu64
")",
                     exts->start);
+      errno = ERANGE;
       return -1;
     }
 
-- 
2.20.1

Thanks: Eric Blake for reporting the bug.
---
 filters/offset/offset.c       | 4 +++-
 filters/partition/partition.c | 4 +++-
 filters/truncate/truncate.c   | 8 +++++++-
 3 files changed, 13 insertions(+), 3 deletions(-)

diff --git a/filters/offset/offset.c b/filters/offset/offset.c
index 24ccb4c..633a1c7 100644
--- a/filters/offset/offset.c
+++ b/filters/offset/offset.c
@@ -156,8 +156,10 @@ offset_extents (struct nbdkit_next_ops *next_ops, void
*nxdata,
   for (i = 0; i < nbdkit_extents_count (extents2); ++i) {
     e = nbdkit_get_extent (extents2, i);
     e.offset -= offset;
-    if (nbdkit_add_extent (extents, e.offset, e.length, e.type) == -1)
+    if (nbdkit_add_extent (extents, e.offset, e.length, e.type) == -1) {
+      *err = errno;
       return -1;
+    }
   }
   return 0;
 }
diff --git a/filters/partition/partition.c b/filters/partition/partition.c
index a89dbec..a635df8 100644
--- a/filters/partition/partition.c
+++ b/filters/partition/partition.c
@@ -246,8 +246,10 @@ partition_extents (struct nbdkit_next_ops *next_ops, void
*nxdata,
   for (i = 0; i < nbdkit_extents_count (extents2); ++i) {
     e = nbdkit_get_extent (extents2, i);
     e.offset -= h->offset;
-    if (nbdkit_add_extent (extents, e.offset, e.length, e.type) == -1)
+    if (nbdkit_add_extent (extents, e.offset, e.length, e.type) == -1) {
+      *err = errno;
       return -1;
+    }
   }
   return 0;
 }
diff --git a/filters/truncate/truncate.c b/filters/truncate/truncate.c
index 076ae22..0c5dedb 100644
--- a/filters/truncate/truncate.c
+++ b/filters/truncate/truncate.c
@@ -319,6 +319,10 @@ truncate_extents (struct nbdkit_next_ops *next_ops, void
*nxdata,
    * returned data to the original array.
    */
   extents2 = nbdkit_extents_new (offset, real_size_copy);
+  if (extents2 == NULL) {
+    *err = errno;
+    return -1;
+  }
   if (offset + count <= real_size_copy)
     n = count;
   else
@@ -329,8 +333,10 @@ truncate_extents (struct nbdkit_next_ops *next_ops, void
*nxdata,
   for (i = 0; i < nbdkit_extents_count (extents2); ++i) {
     struct nbdkit_extent e = nbdkit_get_extent (extents2, i);
 
-    if (nbdkit_add_extent (extents, e.offset, e.length, e.type) == -1)
+    if (nbdkit_add_extent (extents, e.offset, e.length, e.type) == -1) {
+      *err = errno;
       return -1;
+    }
   }
 
   return 0;
-- 
2.20.1

On 4/24/19 6:04 AM, Richard W.M. Jones wrote:
> errno is currently set to a suitable value if nbdkit_extents_new
> returns an error, but this was not documented so I added that
> documentation.
> 
> Set errno to a suitable value if nbdkit_add_extent returns an error,
> and also document that.
> 
> Thanks: Eric Blake for spotting the problem.
> ---
>  docs/nbdkit-filter.pod | 3 ++-
>  docs/nbdkit-plugin.pod | 2 +-
>  server/extents.c       | 2 ++
>  3 files changed, 5 insertions(+), 2 deletions(-)
> +++ b/server/extents.c
> @@ -154,6 +154,7 @@ nbdkit_add_extent (struct nbdkit_extents *exts,
>      nbdkit_error ("nbdkit_add_extent: "
>                    "extents must be added in ascending order and
"
>                    "must be contiguous");
> +    errno = ERANGE;
>      return -1;
ERANGE can't be sent over the wire to the client (we end up sending
NBD_EINVAL instead), but it is the best choice for server-side logging
when diagnosing the problem.  ACK.

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

On 4/24/19 6:04 AM, Richard W.M. Jones wrote:
> Thanks: Eric Blake for reporting the bug.
> ---
>  filters/offset/offset.c       | 4 +++-
>  filters/partition/partition.c | 4 +++-
>  filters/truncate/truncate.c   | 8 +++++++-
>  3 files changed, 13 insertions(+), 3 deletions(-)
> 
> diff --git a/filters/offset/offset.c b/filters/offset/offset.c
> index 24ccb4c..633a1c7 100644
> --- a/filters/offset/offset.c
> +++ b/filters/offset/offset.c
> @@ -156,8 +156,10 @@ offset_extents (struct nbdkit_next_ops *next_ops, void
*nxdata,
>    for (i = 0; i < nbdkit_extents_count (extents2); ++i) {
>      e = nbdkit_get_extent (extents2, i);
>      e.offset -= offset;
> -    if (nbdkit_add_extent (extents, e.offset, e.length, e.type) == -1)
> +    if (nbdkit_add_extent (extents, e.offset, e.length, e.type) == -1) {
> +      *err = errno;
>        return -1;
> +    }
>    }
I'll see if I can spot other places that also need the cleanup, but this
patch is incrementally better than what we have, so ACK.

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