Libguestfs - Aug 2019 - [nbdkit PATCH 0/2] errno cleanup patches

I ran into these while trying to prepare patches to add
NBD_CMD_FLAG_FAST_ZERO, which will expose a new NBD_ENOTSUP wire
value.

Eric Blake (2):
  plugins: Don't lose original error when emulating FUA
  plugins: Permit ENOTSUP as synonym for EOPNOTSUPP

 docs/nbdkit-filter.pod  | 11 ++++++-----
 docs/nbdkit-plugin.pod  | 12 +++++++-----
 plugins/file/file.c     | 16 +++++++++++-----
 plugins/perl/perl.c     |  2 +-
 plugins/python/python.c |  2 +-
 plugins/ruby/ruby.c     |  2 +-
 server/plugins.c        | 11 ++++-------
 7 files changed, 31 insertions(+), 25 deletions(-)

-- 
2.20.1

Calling plugin_flush(b, conn, 0, err) instead of the longer
p->plugin.flush(connection_get_handle(conn, 0) has an unfortunate side
effect: if .can_fua is emulate, and the client requested FUA, there is
a possibility that a failed attempt to flush has set neither the
threadlocal error nor errno to a sane value (the most obvious case is
when the plugin lacks .flush, and directly sets *err to EINVAL).
However, when the caller detects that flush failed, it blindly assigns
*err = get_error(p), which will default to EIO if nothing else can be
located in threadlocal or errno.

In practice, this corner case requires a custom plugin to hit: the sh
plugin seemed like the most obvious candidate, but it provides a
.flush callback that reliably sets errno on failure (so the subsequent
call to get_error(p) happens to reset *err to the value it already
had).  Other bindings like Python haven't even been converted to
version 2 API yet, which means .can_fua can only be set indirectly
based on .flush existing.

But it's easy enough to prevent: after calling any other plugin_*
function fails, only change *err if it is not already set; in turn
this means we don't have to mess with errno after plugin_pwrite fails
during .zero fallback.

Fixes: 20db811e
Signed-off-by: Eric Blake <eblake@redhat.com>
---
 server/plugins.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/server/plugins.c b/server/plugins.c
index be952504..88a41044 100644
--- a/server/plugins.c
+++ b/server/plugins.c
@@ -599,7 +599,7 @@ plugin_pwrite (struct backend *b, struct connection *conn,
   }
   if (r != -1 && need_flush)
     r = plugin_flush (b, conn, 0, err);
-  if (r == -1)
+  if (r == -1 && !*err)
     *err = get_error (p);
   return r;
 }
@@ -633,7 +633,7 @@ plugin_trim (struct backend *b, struct connection *conn,
   }
   if (r != -1 && need_flush)
     r = plugin_flush (b, conn, 0, err);
-  if (r == -1)
+  if (r == -1 && !*err)
     *err = get_error (p);
   return r;
 }
@@ -700,13 +700,10 @@ plugin_zero (struct backend *b, struct connection *conn,
     count -= limit;
   }

-  *err = errno;
-  errno = *err;
-
  done:
   if (r != -1 && need_flush)
     r = plugin_flush (b, conn, 0, err);
-  if (r == -1)
+  if (r == -1 && !*err)
     *err = get_error (p);
   return r;
 }
-- 
2.20.1

POSIX allows but does not require ENOTSUP and EOPNOTSUPP to map to the
same errno value.  This patch has no impact on Linux, but does affect
other systems: we want to be permissive in what we accept (either
spelling, since the two are commonly confused), while strict in what
we generate (we documented EOPNOTSUPP as the trigger to fall back to
.write, so stick to that internally where it makes sense).  An
upcoming NBD protocol extension is proposing the exposure of an
NBD_ENOTSUP error, and so we'll need to make sure that both errno
values (when they are distinct) feed into that single wire value.

Signed-off-by: Eric Blake <eblake@redhat.com>
---
 docs/nbdkit-filter.pod  | 11 ++++++-----
 docs/nbdkit-plugin.pod  | 12 +++++++-----
 plugins/file/file.c     | 16 +++++++++++-----
 plugins/perl/perl.c     |  2 +-
 plugins/python/python.c |  2 +-
 plugins/ruby/ruby.c     |  2 +-
 server/plugins.c        |  2 +-
 7 files changed, 28 insertions(+), 19 deletions(-)

diff --git a/docs/nbdkit-filter.pod b/docs/nbdkit-filter.pod
index 02f15c13..6e2bea61 100644
--- a/docs/nbdkit-filter.pod
+++ b/docs/nbdkit-filter.pod
@@ -524,15 +524,16 @@ C<next_ops-E<gt>zero> if
C<next_ops-E<gt>can_fua> returned a positive
 value.

 Note that unlike the plugin C<.zero> which is permitted to fail with
-C<EOPNOTSUPP> to force a fallback to C<.pwrite>, the function
-C<next_ops-E<gt>zero> will never fail with C<err> set to
C<EOPNOTSUPP>
-because the fallback has already taken place.
+C<ENOTSUP> or C<EOPNOTSUPP> to force a fallback to
C<.pwrite>, the
+function C<next_ops-E<gt>zero> will never fail with C<err>
set to
+C<ENOTSUP> or C<EOPNOTSUPP> because the fallback has already taken
+place.

 If there is an error, C<.zero> should call C<nbdkit_error> with an
 error message B<and> return -1 with C<err> set to the positive
errno
 value to return to the client.  The filter should never fail with
-C<EOPNOTSUPP> (while plugins have automatic fallback to C<.pwrite>,
-filters do not).
+C<ENOTSUP> or C<EOPNOTSUPP> (while plugins have automatic fallback
to
+C<.pwrite>, filters do not).

 =head2 C<.extents>

diff --git a/docs/nbdkit-plugin.pod b/docs/nbdkit-plugin.pod
index 03269e88..423cccdb 100644
--- a/docs/nbdkit-plugin.pod
+++ b/docs/nbdkit-plugin.pod
@@ -612,15 +612,16 @@ plugin wants the C<.zero> callback to be utilized. 
Support for
 writing zeroes is still advertised to the client (unless the nbdkit
 filter nozero is also used), so returning false merely serves as a way
 to avoid complicating the C<.zero> callback to have to fail with
-C<EOPNOTSUPP> on the connections where it will never be more efficient
-than using C<.pwrite> up front.
+C<ENOTSUP> or C<EOPNOTSUPP> on the connections where it will never
be
+more efficient than using C<.pwrite> up front.

 If there is an error, C<.can_zero> should call C<nbdkit_error> with
an
 error message and return C<-1>.

 This callback is not required.  If omitted, then nbdkit always tries
 C<.zero> first if it is present, and gracefully falls back to
-C<.pwrite> if C<.zero> was absent or failed with
C<EOPNOTSUPP>.
+C<.pwrite> if C<.zero> was absent or failed with C<ENOTSUP>
or
+C<EOPNOTSUPP>.

 =head2 C<.can_extents>

@@ -809,8 +810,9 @@ C<.can_fua>.
 If C<NBDKIT_FLAG_MAY_TRIM> is requested, the operation can punch a
 hole instead of writing actual zero bytes, but only if subsequent
 reads from the hole read as zeroes.  If this callback is omitted, or
-if it fails with C<EOPNOTSUPP> (whether by C<nbdkit_set_error> or
-C<errno>), then C<.pwrite> will be used instead.
+if it fails with C<ENOTSUP> or C<EOPNOTSUPP> (whether by
+C<nbdkit_set_error> or C<errno>), then C<.pwrite> will be
used
+instead.

 The callback must write the whole C<count> bytes if it can.  The NBD
 protocol doesn't allow partial writes (instead, these would be
diff --git a/plugins/file/file.c b/plugins/file/file.c
index 9df5001d..a9d6491a 100644
--- a/plugins/file/file.c
+++ b/plugins/file/file.c
@@ -73,6 +73,12 @@ static pthread_mutex_t lseek_lock =
PTHREAD_MUTEX_INITIALIZER;
 /* to enable: -D file.zero=1 */
 int file_debug_zero;

+static bool
+file_is_enotsup (int err)
+{
+  return err == ENOTSUP || err == EOPNOTSUPP;
+}
+
 static void
 file_unload (void)
 {
@@ -399,7 +405,7 @@ file_zero (void *handle, uint32_t count, uint64_t offset,
uint32_t flags)
       goto out;
     }

-    if (errno != EOPNOTSUPP) {
+    if (!file_is_enotsup (errno)) {
       nbdkit_error ("zero: %m");
       return -1;
     }
@@ -418,7 +424,7 @@ file_zero (void *handle, uint32_t count, uint64_t offset,
uint32_t flags)
       goto out;
     }

-    if (errno != EOPNOTSUPP) {
+    if (!file_is_enotsup (errno)) {
       nbdkit_error ("zero: %m");
       return -1;
     }
@@ -443,14 +449,14 @@ file_zero (void *handle, uint32_t count, uint64_t offset,
uint32_t flags)
         goto out;
       }

-      if (errno != EOPNOTSUPP) {
+      if (!file_is_enotsup (errno)) {
         nbdkit_error ("zero: %m");
         return -1;
       }

       h->can_fallocate = false;
     } else {
-      if (errno != EOPNOTSUPP) {
+      if (!file_is_enotsup (errno)) {
         nbdkit_error ("zero: %m");
         return -1;
       }
@@ -513,7 +519,7 @@ file_trim (void *handle, uint32_t count, uint64_t offset,
uint32_t flags)
         return -1;
       }

-      if (errno == EOPNOTSUPP)
+      if (file_is_enotsup (EOPNOTSUPP))
         h->can_punch_hole = false;

       nbdkit_debug ("ignoring failed fallocate during trim: %m");
diff --git a/plugins/perl/perl.c b/plugins/perl/perl.c
index e8395dd2..59b26788 100644
--- a/plugins/perl/perl.c
+++ b/plugins/perl/perl.c
@@ -505,7 +505,7 @@ perl_zero (void *handle, uint32_t count, uint64_t offset,
int may_trim)
     FREETMPS;
     LEAVE;

-    if (last_error == EOPNOTSUPP) {
+    if (last_error == EOPNOTSUPP || last_error == ENOTSUP) {
       /* When user requests this particular error, we want to
          gracefully fall back, and to accomodate both a normal return
          and an exception. */
diff --git a/plugins/python/python.c b/plugins/python/python.c
index 71a982a3..20232f4f 100644
--- a/plugins/python/python.c
+++ b/plugins/python/python.c
@@ -603,7 +603,7 @@ py_zero (void *handle, uint32_t count, uint64_t offset, int
may_trim)
     r = PyObject_CallObject (fn, args);
     Py_DECREF (fn);
     Py_DECREF (args);
-    if (last_error == EOPNOTSUPP) {
+    if (last_error == EOPNOTSUPP || last_error == ENOTSUP) {
       /* When user requests this particular error, we want to
          gracefully fall back, and to accomodate both a normal return
          and an exception. */
diff --git a/plugins/ruby/ruby.c b/plugins/ruby/ruby.c
index b31f6a4e..ff7932c4 100644
--- a/plugins/ruby/ruby.c
+++ b/plugins/ruby/ruby.c
@@ -420,7 +420,7 @@ plugin_rb_zero (void *handle, uint32_t count, uint64_t
offset, int may_trim)
   argv[3] = may_trim ? Qtrue : Qfalse;
   last_error = 0;
   (void) funcall2 (Qnil, rb_intern ("zero"), 4, argv,
&exception_happened);
-  if (last_error == EOPNOTSUPP ||
+  if (last_error == EOPNOTSUPP || last_error == ENOTSUP ||
       exception_happened == EXCEPTION_NO_METHOD_ERROR) {
     nbdkit_debug ("zero falling back to pwrite");
     nbdkit_set_error (EOPNOTSUPP);
diff --git a/server/plugins.c b/server/plugins.c
index 88a41044..1497c8b1 100644
--- a/server/plugins.c
+++ b/server/plugins.c
@@ -679,7 +679,7 @@ plugin_zero (struct backend *b, struct connection *conn,
       emulate = true;
     if (r == -1)
       *err = emulate ? EOPNOTSUPP : get_error (p);
-    if (r == 0 || *err != EOPNOTSUPP)
+    if (r == 0 || (*err != EOPNOTSUPP && *err != ENOTSUP))
       goto done;
   }

-- 
2.20.1

On Mon, Aug 12, 2019 at 10:10:07PM -0500, Eric Blake
wrote:
> index 9df5001d..a9d6491a 100644
> --- a/plugins/file/file.c
> +++ b/plugins/file/file.c
> @@ -73,6 +73,12 @@ static pthread_mutex_t lseek_lock =
PTHREAD_MUTEX_INITIALIZER;
>  /* to enable: -D file.zero=1 */
>  int file_debug_zero;
> 
> +static bool
> +file_is_enotsup (int err)
> +{
> +  return err == ENOTSUP || err == EOPNOTSUPP;
> +}
Series is fine, but I'd probably have called this function just
"is_enotsup".  We have usually reserved the <plugin>_ prefix for
the callback functions themselves, not random helper functions
in the plugin.

ACK

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-df lists disk usage of guests without needing to install any
software inside the virtual machine.  Supports Linux and Windows.
http://people.redhat.com/~rjones/virt-df/