Libguestfs - Feb 2018 - [nbdkit PATCH 0/2] Consistent plugin return value handling

While working on improving the backend interface to allow filters
to handle errors, I noticed that I've introduced some minor
incompatibilities for filters that don't quite obey the documentation
which states that a callback should return only 0/-1.  Prior to
my additions, we treated all plugin returns other than -1 as success
(sort of makes sense for a positive return, particularly if a
plugin can guarantee no short read/write and so just returns the
syscall value; a bit weirder if a plugin returns something other
than -1 on failure, where a typical case might be a plugin that
tried to return negative errno).  However, I've had two commits
that were inconsistent, where a single plugin callback's
undocumented return value outside of 0/-1 was sometimes treated
as success and sometimes as failure.

We have two options: we can tighten the nbdkit code to match the
existing documentation (previously unenforced) to require that a
plugin MUST return 0 (or positive) on success (and treat
everything else as failures), which may subtly break some
existing plugins that weren't expecting this.  Or we can fix the
nbdkit code to consistently always treat -1 as the only failure
value, and continue to allow -2 or positive values as
(undocumented) success.

This series goes with the latter option, but because I also see
the possibility of going with the former option, I'm not pushing
these without review.

Eric Blake (2):
  plugins: Consistent error handling in zero
  plugins: Consistent error handling on FUA

 src/plugins.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

-- 
2.14.3

We document that a plugin callback must return -1 on failure, and
all other places in the code treat any other value as success (the
ideal plugin returns only 0 or -1, but if some existing plugin
returns any other value, changing our reaction now might break
binary back-compatibility).  However, we missed a case: if
plugin_zero() falls back to pwrite, we were treating all negative
values as failure, even when -2 returned from directly from
pwrite is success.

Fixes: 19184d3eb6356ae3b14da0fbaa9c9bdc7743a448
Signed-off-by: Eric Blake <eblake@redhat.com>
---
 src/plugins.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/plugins.c b/src/plugins.c
index dba3e24..699e9c5 100644
--- a/src/plugins.c
+++ b/src/plugins.c
@@ -491,7 +491,7 @@ plugin_zero (struct backend *b, struct connection *conn,
   while (count) {
     result = p->plugin.pwrite (connection_get_handle (conn, 0),
                                buf, limit, offset);
-    if (result < 0)
+    if (result == -1)
       break;
     count -= limit;
     if (count < limit)
-- 
2.14.3

We document that a plugin callback should return 0 on success, but
other places in the code treat all values other than -1 as success
(perhaps we should treat all negative values as errors, instead of
exactly -1, but that may break binary back-compatibility).  However,
when reworking where FUA fallback occurs, we introduced a subtle
change: if a plugin returns a positive value on success, and the
client requested FUA, we ended up reporting success to the client
without performing FUA.

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

diff --git a/src/plugins.c b/src/plugins.c
index 699e9c5..1b0816c 100644
--- a/src/plugins.c
+++ b/src/plugins.c
@@ -412,7 +412,7 @@ plugin_pwrite (struct backend *b, struct connection *conn,
     errno = EROFS;
     return -1;
   }
-  if (r == 0 && fua) {
+  if (r != -1 && fua) {
     assert (p->plugin.flush);
     r = plugin_flush (b, conn, 0);
   }
@@ -439,7 +439,7 @@ plugin_trim (struct backend *b, struct connection *conn,
     errno = EINVAL;
     return -1;
   }
-  if (r == 0 && fua) {
+  if (r != -1 && fua) {
     assert (p->plugin.flush);
     r = plugin_flush (b, conn, 0);
   }
@@ -503,7 +503,7 @@ plugin_zero (struct backend *b, struct connection *conn,
   errno = err;

  done:
-  if (!result && fua) {
+  if (result != -1 && fua) {
     assert (p->plugin.flush);
     result = plugin_flush (b, conn, 0);
   }
-- 
2.14.3

ACK series.

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/