Libguestfs - Jan 2017 - Re: [nbdkit PATCH v2 4/6] plugins: Add new nbdkit_set_error() utility function

Fix some things I noticed while reviewing v1, and follow Rich's
idea to add a new nbdkit_set_error() utility function with a
binding for Python users to request a particular error (rather
than being forced to live with whatever stale value is in errno
after all the intermediate binding glue code).

I could not easily find out how to register a C function callable
from perl bindings, and have not got enough experience with Ruby
or Ocaml (those will probably require me to do lots of copy-and-paste
and language learning), but if the code looks good for python, I
suspect the other languages will take a similar approach.

Maybe we want to also make nbdkit.error() and nbdkit.debug()
callable from Python (so that we have a more complete binding
of all the utility functions that C plugins can use); this
can be used as a starting point for that.

Eric Blake (6):
  file: Optimize writing zeroes without holes
  protocol: Map EROFS to EPERM
  protocol: Support ESHUTDOWN error
  plugins: Add new nbdkit_set_error() utility function
  python: Expose nbdkit_set_error to python script
  python: Support zero callback

 docs/nbdkit-plugin.pod                  | 16 +++++++--
 include/nbdkit-plugin.h                 |  3 +-
 plugins/file/file.c                     | 19 +++++++---
 plugins/python/example.py               | 11 ++++++
 plugins/python/nbdkit-python-plugin.pod | 47 +++++++++++++++++++++---
 plugins/python/python.c                 | 64 +++++++++++++++++++++++++++++++++
 src/connections.c                       | 37 +++++++++++++++----
 src/errors.c                            |  6 ++++
 src/internal.h                          |  2 ++
 src/plugins.c                           | 10 ++++--
 src/protocol.h                          |  1 +
 src/tls.c                               | 28 +++++++++++++--
 12 files changed, 221 insertions(+), 23 deletions(-)

-- 
2.9.3

On newer Linux, fallocate() can more efficiently write zeroes
without punching holes.  When this mode is supported, use it
instead of falling back to write (while still gracefully falling
back to the write method if it fails with EOPNOTSUPP).

Also fix a typo in an error message in the previous commit.

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

diff --git a/plugins/file/file.c b/plugins/file/file.c
index bd7a9c1..2d32615 100644
--- a/plugins/file/file.c
+++ b/plugins/file/file.c
@@ -254,6 +254,7 @@ static int
 file_zero (void *handle, uint32_t count, uint64_t offset, int may_trim)
 {
   struct handle *h = handle;
+  int r = -1;

   if (wdelayms > 0) {
     const struct timespec ts = {
@@ -265,18 +266,28 @@ file_zero (void *handle, uint32_t count, uint64_t offset,
int may_trim)

 #ifdef FALLOC_FL_PUNCH_HOLE
   if (may_trim) {
-    int r = fallocate (h->fd, FALLOC_FL_PUNCH_HOLE | FALLOC_FL_KEEP_SIZE,
-		       offset, count);
+    r = fallocate (h->fd, FALLOC_FL_PUNCH_HOLE | FALLOC_FL_KEEP_SIZE,
+		   offset, count);
     if (r == -1 && errno != EOPNOTSUPP) {
-      nbdkit_error ("pwrite: %m");
+      nbdkit_error ("zero: %m");
     }
+    /* PUNCH_HOLE is older; if it is not supported, it is likely that
+       ZERO_RANGE will not work either, so fall back to write. */
     return r;
   }
 #endif

+#ifdef FALLOC_FL_ZERO_RANGE
+  r = fallocate (h->fd, FALLOC_FL_ZERO_RANGE, offset, count);
+  if (r == -1 && errno != EOPNOTSUPP) {
+    nbdkit_error ("zero: %m");
+  }
+#else
   /* Trigger a fall back to writing */
   errno = EOPNOTSUPP;
-  return -1;
+#endif
+
+  return r;
 }

 /* Flush the file to disk. */
-- 
2.9.3

The NBD Protocol requests an EPERM failure on attempts to write to
a read-only export. We were internally using EROFS, but our mapping
was slamming that to EINVAL.

Signed-off-by: Eric Blake <eblake@redhat.com>
---
 src/connections.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/src/connections.c b/src/connections.c
index 1b39547..e15a777 100644
--- a/src/connections.c
+++ b/src/connections.c
@@ -724,6 +724,7 @@ nbd_errno (int error)
   switch (error) {
   case 0:
     return NBD_SUCCESS;
+  case EROFS:
   case EPERM:
     return NBD_EPERM;
   case EIO:
-- 
2.9.3

The NBD specification was clarified to state that the server can
send ESHUTDOWN at any time that it wants to inform the client
that the server is about to close the connection and that all
further commands will fail, to give the client a chance to first
send NBD_CMD_DISC for a clean shutdown.  While nbdkit does not
(yet) directly send this errno, it is feasible that a plugin may
want this error value preserved across the wire rather than
converted to EINVAL.

Signed-off-by: Eric Blake <eblake@redhat.com>
---
 src/connections.c | 4 ++++
 src/protocol.h    | 1 +
 2 files changed, 5 insertions(+)

diff --git a/src/connections.c b/src/connections.c
index e15a777..c0f0567 100644
--- a/src/connections.c
+++ b/src/connections.c
@@ -737,6 +737,10 @@ nbd_errno (int error)
   case EFBIG:
   case ENOSPC:
     return NBD_ENOSPC;
+#ifdef ESHUTDOWN
+  case ESHUTDOWN:
+    return NBD_ESHUTDOWN;
+#endif
   case EINVAL:
   default:
     return NBD_EINVAL;
diff --git a/src/protocol.h b/src/protocol.h
index 4571a3a..74c4527 100644
--- a/src/protocol.h
+++ b/src/protocol.h
@@ -145,5 +145,6 @@ struct reply {
 #define NBD_ENOMEM     12
 #define NBD_EINVAL     22
 #define NBD_ENOSPC     28
+#define NBD_ESHUTDOWN 108

 #endif /* NBDKIT_PROTOCOL_H */
-- 
2.9.3

The plugin interface was impliticly using the last errno value
as the source for any error code sent over the wire to the client.
This is okay for C, but in other language bindings, it gets
rather awkward when you can't guarantee what errno will even be
set to by the time the binding glue has finished executing.

The solution is to expose an explicit utility function for setting
the preferred error value, while still falling back to errno for
backwards compatibility.  The saved error code has to be
thread-safe for use in multithreaded plugins.

Note that setting an error value but then returning success from
the plugin has no impact; the error value is only consulted when
the plugin reports failure.  Also, plugin_zero() has to be a bit
careful about using EOPNOTSUPP as its trigger to fall back to
.pwrite, so that the fallback does not inherit an accidental
improper error code.

Signed-off-by: Eric Blake <eblake@redhat.com>
---
 docs/nbdkit-plugin.pod  | 16 +++++++++++++---
 include/nbdkit-plugin.h |  3 ++-
 src/connections.c       | 32 +++++++++++++++++++++++++-------
 src/errors.c            |  6 ++++++
 src/internal.h          |  2 ++
 src/plugins.c           | 10 ++++++++--
 src/tls.c               | 28 ++++++++++++++++++++++++++--
 7 files changed, 82 insertions(+), 15 deletions(-)

diff --git a/docs/nbdkit-plugin.pod b/docs/nbdkit-plugin.pod
index ee6bbd5..ab26a6a 100644
--- a/docs/nbdkit-plugin.pod
+++ b/docs/nbdkit-plugin.pod
@@ -143,14 +143,23 @@ is called once just before the plugin is unloaded from
memory.
 =head1 ERROR HANDLING

 If there is an error in the plugin, the plugin should call
-C<nbdkit_error> with the error message, and then return an error
-indication from the callback, eg. NULL or -1.
+C<nbdkit_set_error> to influence the error code that will be set to
+the client, then C<nbdkit_error> to report an error message; the
+plugin should then return an error indication from the callback,
+eg. NULL or -1.  If the call to C<nbdkit_set_error> is omitted, then
+the value of C<errno> will be used instead.
+
+C<nbdkit_set_error> has the following prototype:
+
+ void nbdkit_set_error (int);

 C<nbdkit_error> has the following prototype and works like
 L<printf(3)>:

  void nbdkit_error (const char *fs, ...);

+For convenience, C<nbdkit_error> preserves the value of C<errno>.
+
 =head1 FILENAMES AND PATHS

 The server usually (not always) changes directory to C</> before it
@@ -525,7 +534,8 @@ L<printf(3)>:
  void nbdkit_debug (const char *fs, ...);

 Note that C<nbdkit_debug> only prints things when the server is in
-verbose mode (I<-v> option).
+verbose mode (I<-v> option).  For convenience, it preserves the value
+of C<errno>.

 =head1 INSTALLING THE PLUGIN

diff --git a/include/nbdkit-plugin.h b/include/nbdkit-plugin.h
index 3d25642..2d401bf 100644
--- a/include/nbdkit-plugin.h
+++ b/include/nbdkit-plugin.h
@@ -1,5 +1,5 @@
 /* nbdkit
- * Copyright (C) 2013-2016 Red Hat Inc.
+ * Copyright (C) 2013-2017 Red Hat Inc.
  * All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
@@ -82,6 +82,7 @@ struct nbdkit_plugin {
   /* int (*set_exportname) (void *handle, const char *exportname); */
 };

+extern void nbdkit_set_error (int err);
 extern void nbdkit_error (const char *msg, ...)
   __attribute__((format (printf, 1, 2)));
 extern void nbdkit_verror (const char *msg, va_list args);
diff --git a/src/connections.c b/src/connections.c
index c0f0567..83c863e 100644
--- a/src/connections.c
+++ b/src/connections.c
@@ -602,6 +602,19 @@ validate_request (struct connection *conn,
   return 1;                     /* Commands validates. */
 }

+/* Grab the appropriate error value.
+ */
+static int
+_get_error (void)
+{
+  int err = errno;
+  int ret = tls_get_error ();
+
+  if (!ret)
+    ret = err ? err : EIO;
+  return ret;
+}
+
 /* This is called with the request lock held to actually execute the
  * request (by calling the plugin).  Note that the request fields have
  * been validated already in 'validate_request' so we don't have to
@@ -611,7 +624,8 @@ validate_request (struct connection *conn,
  * Only returns -1 if there is a fatal error and the connection cannot
  * continue.
  *
- * On read/write errors, sets *error to errno (or EIO if errno is not
+ * On read/write errors, sets *error to the value stored in
+ * nbdkit_set_error(), falling back to errno (or EIO if errno is not
  * set) and returns 0.
  */
 static int
@@ -628,11 +642,15 @@ _handle_request (struct connection *conn,
   if (!conn->can_flush || conn->readonly)
     flush_after_command = false;

+  /* The plugin should call nbdkit_set_error() to request a particular
+     error, otherwise we fallback to errno or EIO. */
+  tls_set_error (0);
+
   switch (cmd) {
   case NBD_CMD_READ:
     r = plugin_pread (conn, buf, count, offset);
     if (r == -1) {
-      *error = errno ? errno : EIO;
+      *error = _get_error ();
       return 0;
     }
     break;
@@ -640,7 +658,7 @@ _handle_request (struct connection *conn,
   case NBD_CMD_WRITE:
     r = plugin_pwrite (conn, buf, count, offset);
     if (r == -1) {
-      *error = errno ? errno : EIO;
+      *error = _get_error ();
       return 0;
     }
     break;
@@ -648,7 +666,7 @@ _handle_request (struct connection *conn,
   case NBD_CMD_FLUSH:
     r = plugin_flush (conn);
     if (r == -1) {
-      *error = errno ? errno : EIO;
+      *error = _get_error ();
       return 0;
     }
     break;
@@ -656,7 +674,7 @@ _handle_request (struct connection *conn,
   case NBD_CMD_TRIM:
     r = plugin_trim (conn, count, offset);
     if (r == -1) {
-      *error = errno ? errno : EIO;
+      *error = _get_error ();
       return 0;
     }
     break;
@@ -664,7 +682,7 @@ _handle_request (struct connection *conn,
   case NBD_CMD_WRITE_ZEROES:
     r = plugin_zero (conn, count, offset, !(flags & NBD_CMD_FLAG_NO_HOLE));
     if (r == -1) {
-      *error = errno ? errno : EIO;
+      *error = _get_error ();
       return 0;
     }
     break;
@@ -676,7 +694,7 @@ _handle_request (struct connection *conn,
   if (flush_after_command) {
     r = plugin_flush (conn);
     if (r == -1) {
-      *error = errno ? errno : EIO;
+      *error = _get_error ();
       return 0;
     }
   }
diff --git a/src/errors.c b/src/errors.c
index adb2db7..4abab5a 100644
--- a/src/errors.c
+++ b/src/errors.c
@@ -131,3 +131,9 @@ nbdkit_error (const char *fs, ...)

   errno = err;
 }
+
+void
+nbdkit_set_error (int err)
+{
+  tls_set_error (err);
+}
diff --git a/src/internal.h b/src/internal.h
index cb15bab..f0c7768 100644
--- a/src/internal.h
+++ b/src/internal.h
@@ -123,6 +123,8 @@ extern void tls_set_instance_num (size_t instance_num);
 extern void tls_set_sockaddr (struct sockaddr *addr, socklen_t addrlen);
 extern const char *tls_get_name (void);
 extern size_t tls_get_instance_num (void);
+extern void tls_set_error (int err);
+extern int tls_get_error (void);
 /*extern void tls_get_sockaddr ();*/

 /* utils.c */
diff --git a/src/plugins.c b/src/plugins.c
index 574be03..ff7bf48 100644
--- a/src/plugins.c
+++ b/src/plugins.c
@@ -520,7 +520,7 @@ plugin_zero (struct connection *conn,
   char *buf;
   uint32_t limit;
   int result;
-  int err;
+  int err = 0;

   debug ("zero count=%" PRIu32 " offset=%" PRIu64 "
may_trim=%d",
          count, offset, may_trim);
@@ -530,11 +530,17 @@ plugin_zero (struct connection *conn,
   if (plugin.zero) {
     errno = 0;
     result = plugin.zero (conn->handle, count, offset, may_trim);
-    if (result == 0 || errno != EOPNOTSUPP)
+    if (result == -1) {
+      err = tls_get_error ();
+      if (!err)
+        err = errno;
+    }
+    if (result == 0 || err != EOPNOTSUPP)
       return result;
   }

   assert (plugin.pwrite);
+  tls_set_error (0);
   limit = count < MAX_REQUEST_SIZE ? count : MAX_REQUEST_SIZE;
   buf = calloc (limit, 1);
   if (!buf) {
diff --git a/src/tls.c b/src/tls.c
index 390b03e..e80e925 100644
--- a/src/tls.c
+++ b/src/tls.c
@@ -44,8 +44,10 @@
 #include "nbdkit-plugin.h"
 #include "internal.h"

-/* Note currently all thread-local storage data is informational.
- * It's mainly used for smart error and debug messages.
+/* Note that most thread-local storage data is informational, used for
+ * smart error and debug messages on the server side.  However, error
+ * tracking can be used to influence which error is sent to the client
+ * in a reply.
  *
  * The main thread does not have any associated TLS, *unless* it is
  * serving a request (the '-s' option).
@@ -56,6 +58,7 @@ struct tls {
   size_t instance_num;          /* Can be 0. */
   struct sockaddr *addr;
   socklen_t addrlen;
+  int err;
 };

 static pthread_key_t tls_key;
@@ -150,3 +153,24 @@ tls_get_instance_num (void)

   return tls->instance_num;
 }
+
+void
+tls_set_error (int err)
+{
+  struct tls *tls = pthread_getspecific (tls_key);
+
+  if (tls)
+    tls->err = err;
+  else
+    errno = err;
+}
+
+int
+tls_get_error (void)
+{
+  int err = errno;
+  struct tls *tls = pthread_getspecific (tls_key);
+
+  errno = err;
+  return tls ? tls->err : 0;
+}
-- 
2.9.3

In addition to calling python functions from C, we want to make
script writing easier by exposing C functions to python.  For
now, just wrap nbdkit_set_error(), as that will be needed for
an optimal implementation of a zero() callback.

Signed-off-by: Eric Blake <eblake@redhat.com>
---
 plugins/python/nbdkit-python-plugin.pod | 27 +++++++++++++++++++++++----
 plugins/python/python.c                 | 18 ++++++++++++++++++
 2 files changed, 41 insertions(+), 4 deletions(-)

diff --git a/plugins/python/nbdkit-python-plugin.pod
b/plugins/python/nbdkit-python-plugin.pod
index 9b0f0ef..8b3d08c 100644
--- a/plugins/python/nbdkit-python-plugin.pod
+++ b/plugins/python/nbdkit-python-plugin.pod
@@ -63,9 +63,22 @@ does not need to be executable.  In fact it's a good idea
not to do
 that, because running the plugin directly as a Python script won't
 work.

+=head2 METHODS
+
+Your script may use C<import nbdkit> to have access to the following
+methods in the C<nbdkit> module:
+
+ nbdkit.set_error(I<err>)
+
+Record C<err> as the reason you are about to throw an exception.
C<err>
+should correspond to usual errno values, where it may help to
+C<import errno>.
+
 =head2 EXCEPTIONS

-Python callbacks should throw exceptions to indicate errors.
+Python callbacks should throw exceptions to indicate errors.  Remember
+to use C<nbdkit.set_error> if you need to control which error is sent
+back to the client.

 =head2 PYTHON CALLBACKS

@@ -158,7 +171,8 @@ disk starting at C<offset>.

 NBD only supports whole reads, so your function should try to read
 the whole region (perhaps requiring a loop).  If the read fails or
-is partial, your function should throw an exception.
+is partial, your function should throw an exception, optionally using
+C<nbdkit.set_error> first.

 =item C<pwrite>

@@ -174,7 +188,8 @@ C<offset>.

 NBD only supports whole writes, so your function should try to
 write the whole region (perhaps requiring a loop).  If the write
-fails or is partial, your function should throw an exception.
+fails or is partial, your function should throw an exception,
+ optionally using C<nbdkit.set_error> first.

 =item C<flush>

@@ -186,6 +201,9 @@ fails or is partial, your function should throw an
exception.
 The body of your C<flush> function should do a L<sync(2)> or
 L<fdatasync(2)> or equivalent on the backing store.

+If the flush fails, your function should throw an exception, optionally
+using C<nbdkit.set_error> first.
+
 =item C<trim>

 (Optional)
@@ -194,7 +212,8 @@ L<fdatasync(2)> or equivalent on the backing store.
    # no return value

 The body of your C<trim> function should "punch a hole" in the
-backing store.
+backing store.  If the trim fails, your function should throw an
+exception, optionally using C<nbdkit.set_error> first.

 =back

diff --git a/plugins/python/python.c b/plugins/python/python.c
index 0504715..4a0ff50 100644
--- a/plugins/python/python.c
+++ b/plugins/python/python.c
@@ -54,6 +54,23 @@
 static const char *script;
 static PyObject *module;

+static PyObject *
+set_error (PyObject *self, PyObject *args)
+{
+  int err;
+
+  if (!PyArg_ParseTuple(args, "i", &err))
+    return NULL;
+  nbdkit_set_error (err);
+  Py_RETURN_NONE;
+}
+
+static PyMethodDef NbdkitMethods[] = {
+  { "set_error", set_error, METH_VARARGS,
+    "Store an errno value prior to throwing an exception" },
+  { NULL }
+};
+
 /* Is a callback defined? */
 static int
 callback_defined (const char *name, PyObject **obj_rtn)
@@ -91,6 +108,7 @@ static void
 py_load (void)
 {
   Py_Initialize ();
+  Py_InitModule("nbdkit", NbdkitMethods);
 }

 static void
-- 
2.9.3

Add a python language binding for the .zero callback, used for
implementing NBD_CMD_WRITE_ZEROES.  The caller doesn't have to
return anything, but should use nbdkit.set_error(errno.EOPNOTSUPP)
to get an automatic fallback to pwrite.

Enhance the example to show the use of the fallback mechanism,
and to serve as a test of nbdkit.set_error().

Signed-off-by: Eric Blake <eblake@redhat.com>
---
 plugins/python/example.py               | 11 ++++++++
 plugins/python/nbdkit-python-plugin.pod | 20 ++++++++++++++
 plugins/python/python.c                 | 46 +++++++++++++++++++++++++++++++++
 3 files changed, 77 insertions(+)

diff --git a/plugins/python/example.py b/plugins/python/example.py
index 184896e..8826eba 100644
--- a/plugins/python/example.py
+++ b/plugins/python/example.py
@@ -24,6 +24,9 @@
 #   ><fs> mount /dev/sda1 /
 #   ><fs> [etc]

+import nbdkit
+import errno
+
 # This is the string used to store the emulated disk (initially all
 # zero bytes).  There is one disk per nbdkit instance, so if you
 # reconnect to the same server you should see the same disk.  You
@@ -56,3 +59,11 @@ def pwrite(h, buf, offset):
     global disk
     end = offset + len (buf)
     disk[offset:end] = buf
+
+def zero(h, count, offset, may_trim):
+    global disk
+    if may_trim:
+        disk[offset:offset+count] = bytearray(count)
+    else:
+        nbdkit.set_error(errno.EOPNOTSUPP)
+        raise Exception
diff --git a/plugins/python/nbdkit-python-plugin.pod
b/plugins/python/nbdkit-python-plugin.pod
index 8b3d08c..b78c0e2 100644
--- a/plugins/python/nbdkit-python-plugin.pod
+++ b/plugins/python/nbdkit-python-plugin.pod
@@ -215,6 +215,26 @@ The body of your C<trim> function should "punch
a hole" in the
 backing store.  If the trim fails, your function should throw an
 exception, optionally using C<nbdkit.set_error> first.

+=item C<zero>
+
+(Optional)
+
+ def zero(h, count, offset, may_trim):
+   # no return value
+
+The body of your C<zero> function should ensure that C<count> bytes
+of the disk, starting at C<offset>, will read back as zero.  If
+C<may_trim> is true, the operation may be optimized as a trim as long
+as subsequent reads see zeroes.
+
+NBD only supports whole writes, so your function should try to
+write the whole region (perhaps requiring a loop).  If the write
+fails or is partial, your function should throw an exception,
+optionally using C<nbdkit.set_error> first.  In particular, if
+you would like to automatically fall back to C<pwrite> (perhaps
+because there is nothing to optimize if C<may_trim> is false),
+use C<nbdkit.set_error(errno.EOPNOTSUPP)>.
+
 =back

 =head2 MISSING CALLBACKS
diff --git a/plugins/python/python.c b/plugins/python/python.c
index 4a0ff50..644ced4 100644
--- a/plugins/python/python.c
+++ b/plugins/python/python.c
@@ -54,6 +54,8 @@
 static const char *script;
 static PyObject *module;

+static int last_error;
+
 static PyObject *
 set_error (PyObject *self, PyObject *args)
 {
@@ -62,6 +64,7 @@ set_error (PyObject *self, PyObject *args)
   if (!PyArg_ParseTuple(args, "i", &err))
     return NULL;
   nbdkit_set_error (err);
+  last_error = err;
   Py_RETURN_NONE;
 }

@@ -441,6 +444,48 @@ py_trim (void *handle, uint32_t count, uint64_t offset)
 }

 static int
+py_zero (void *handle, uint32_t count, uint64_t offset, int may_trim)
+{
+  PyObject *obj = handle;
+  PyObject *fn;
+  PyObject *args;
+  PyObject *r;
+
+  if (callback_defined ("zero", &fn)) {
+    PyErr_Clear ();
+
+    last_error = 0;
+    args = PyTuple_New (4);
+    Py_INCREF (obj); /* decremented by Py_DECREF (args) */
+    PyTuple_SetItem (args, 0, obj);
+    PyTuple_SetItem (args, 1, PyLong_FromUnsignedLongLong (count));
+    PyTuple_SetItem (args, 2, PyLong_FromUnsignedLongLong (offset));
+    PyTuple_SetItem (args, 3, PyBool_FromLong (may_trim));
+    r = PyObject_CallObject (fn, args);
+    Py_DECREF (fn);
+    Py_DECREF (args);
+    if (last_error == EOPNOTSUPP) {
+      /* When user requests this particular error, we want to
+         gracefully fall back, and to accomodate both a normal return
+         and an exception. */
+      nbdkit_debug ("zero requested falling back to pwrite");
+      if (r)
+        Py_DECREF (r);
+      PyErr_Clear ();
+      return -1;
+    }
+    if (check_python_failure ("zero") == -1)
+      return -1;
+    Py_DECREF (r);
+    return 0;
+  }
+
+  nbdkit_debug ("zero missing, falling back to pwrite");
+  errno = EOPNOTSUPP;
+  return -1;
+}
+
+static int
 py_can_write (void *handle)
 {
   PyObject *obj = handle;
@@ -597,6 +642,7 @@ static struct nbdkit_plugin plugin = {
   .pwrite            = py_pwrite,
   .flush             = py_flush,
   .trim              = py_trim,
+  .zero              = py_zero,
 };

 NBDKIT_REGISTER_PLUGIN(plugin)
-- 
2.9.3

On Wed, Jan 25, 2017 at 08:42:33PM -0600, Eric Blake
wrote:
> The NBD specification was clarified to state that the server can
> send ESHUTDOWN at any time that it wants to inform the client
> that the server is about to close the connection and that all
> further commands will fail, to give the client a chance to first
> send NBD_CMD_DISC for a clean shutdown.  While nbdkit does not
> (yet) directly send this errno, it is feasible that a plugin may
> want this error value preserved across the wire rather than
> converted to EINVAL.
> 
> Signed-off-by: Eric Blake <eblake@redhat.com>
I pushed up to this patch.

Thanks,

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
libguestfs lets you edit virtual machines.  Supports shell scripting,
bindings from many languages.  http://libguestfs.org

On Wed, Jan 25, 2017 at 08:42:34PM -0600, Eric Blake
wrote:
> +eg. NULL or -1.  If the call to C<nbdkit_set_error> is omitted, then
> +the value of C<errno> will be used instead.
[...]
> +/* Grab the appropriate error value.
> + */
> +static int
> +_get_error (void)
> +{
> +  int err = errno;
> +  int ret = tls_get_error ();
> +
> +  if (!ret)
> +    ret = err ? err : EIO;
> +  return ret;
> +}
I don't think we should use the implicit errno.

The reason is that we cannot be sure that errno is meaningful in
language bindings.  A lot of code could run between (eg) a Perl plugin
seeing a system call fail, and that plugin callback returning to
nbdkit code, and any of that code might touch errno.  Since some of
that code would be in the language interpreter, we cannot even be
careful about preserving errno along those paths.

So I think if the caller didn't call nbdkit_set_errno, we should
assume no errno value is available for us to use.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-top is 'top' for virtual machines.  Tiny program with many
powerful monitoring features, net stats, disk stats, logging, etc.
http://people.redhat.com/~rjones/virt-top

On Wed, Jan 25, 2017 at 08:42:35PM -0600, Eric Blake
wrote:
> In addition to calling python functions from C, we want to make
> script writing easier by exposing C functions to python.  For
> now, just wrap nbdkit_set_error(), as that will be needed for
> an optimal implementation of a zero() callback.
This one looks OK.  I didn't apply it because it requires
the previous patch.

Rich.
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
>  plugins/python/nbdkit-python-plugin.pod | 27 +++++++++++++++++++++++----
>  plugins/python/python.c                 | 18 ++++++++++++++++++
>  2 files changed, 41 insertions(+), 4 deletions(-)
> 
> diff --git a/plugins/python/nbdkit-python-plugin.pod
b/plugins/python/nbdkit-python-plugin.pod
> index 9b0f0ef..8b3d08c 100644
> --- a/plugins/python/nbdkit-python-plugin.pod
> +++ b/plugins/python/nbdkit-python-plugin.pod
> @@ -63,9 +63,22 @@ does not need to be executable.  In fact it's a good
idea not to do
>  that, because running the plugin directly as a Python script won't
>  work.
> 
> +=head2 METHODS
> +
> +Your script may use C<import nbdkit> to have access to the following
> +methods in the C<nbdkit> module:
> +
> + nbdkit.set_error(I<err>)
> +
> +Record C<err> as the reason you are about to throw an exception.
C<err>
> +should correspond to usual errno values, where it may help to
> +C<import errno>.
> +
>  =head2 EXCEPTIONS
> 
> -Python callbacks should throw exceptions to indicate errors.
> +Python callbacks should throw exceptions to indicate errors.  Remember
> +to use C<nbdkit.set_error> if you need to control which error is
sent
> +back to the client.
> 
>  =head2 PYTHON CALLBACKS
> 
> @@ -158,7 +171,8 @@ disk starting at C<offset>.
> 
>  NBD only supports whole reads, so your function should try to read
>  the whole region (perhaps requiring a loop).  If the read fails or
> -is partial, your function should throw an exception.
> +is partial, your function should throw an exception, optionally using
> +C<nbdkit.set_error> first.
> 
>  =item C<pwrite>
> 
> @@ -174,7 +188,8 @@ C<offset>.
> 
>  NBD only supports whole writes, so your function should try to
>  write the whole region (perhaps requiring a loop).  If the write
> -fails or is partial, your function should throw an exception.
> +fails or is partial, your function should throw an exception,
> + optionally using C<nbdkit.set_error> first.
> 
>  =item C<flush>
> 
> @@ -186,6 +201,9 @@ fails or is partial, your function should throw an
exception.
>  The body of your C<flush> function should do a L<sync(2)> or
>  L<fdatasync(2)> or equivalent on the backing store.
> 
> +If the flush fails, your function should throw an exception, optionally
> +using C<nbdkit.set_error> first.
> +
>  =item C<trim>
> 
>  (Optional)
> @@ -194,7 +212,8 @@ L<fdatasync(2)> or equivalent on the backing
store.
>     # no return value
> 
>  The body of your C<trim> function should "punch a hole" in
the
> -backing store.
> +backing store.  If the trim fails, your function should throw an
> +exception, optionally using C<nbdkit.set_error> first.
> 
>  =back
> 
> diff --git a/plugins/python/python.c b/plugins/python/python.c
> index 0504715..4a0ff50 100644
> --- a/plugins/python/python.c
> +++ b/plugins/python/python.c
> @@ -54,6 +54,23 @@
>  static const char *script;
>  static PyObject *module;
> 
> +static PyObject *
> +set_error (PyObject *self, PyObject *args)
> +{
> +  int err;
> +
> +  if (!PyArg_ParseTuple(args, "i", &err))
> +    return NULL;
> +  nbdkit_set_error (err);
> +  Py_RETURN_NONE;
> +}
> +
> +static PyMethodDef NbdkitMethods[] = {
> +  { "set_error", set_error, METH_VARARGS,
> +    "Store an errno value prior to throwing an exception" },
> +  { NULL }
> +};
> +
>  /* Is a callback defined? */
>  static int
>  callback_defined (const char *name, PyObject **obj_rtn)
> @@ -91,6 +108,7 @@ static void
>  py_load (void)
>  {
>    Py_Initialize ();
> +  Py_InitModule("nbdkit", NbdkitMethods);
>  }
> 
>  static void
> -- 
> 2.9.3
> 
> _______________________________________________
> Libguestfs mailing list
> Libguestfs@redhat.com
> https://www.redhat.com/mailman/listinfo/libguestfs
-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
libguestfs lets you edit virtual machines.  Supports shell scripting,
bindings from many languages.  http://libguestfs.org

On Wed, Jan 25, 2017 at 08:42:36PM -0600, Eric Blake
wrote:
> Add a python language binding for the .zero callback, used for
> implementing NBD_CMD_WRITE_ZEROES.  The caller doesn't have to
> return anything, but should use nbdkit.set_error(errno.EOPNOTSUPP)
> to get an automatic fallback to pwrite.
> 
> Enhance the example to show the use of the fallback mechanism,
> and to serve as a test of nbdkit.set_error().
Also looks good.

Rich.
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
>  plugins/python/example.py               | 11 ++++++++
>  plugins/python/nbdkit-python-plugin.pod | 20 ++++++++++++++
>  plugins/python/python.c                 | 46
+++++++++++++++++++++++++++++++++
>  3 files changed, 77 insertions(+)
> 
> diff --git a/plugins/python/example.py b/plugins/python/example.py
> index 184896e..8826eba 100644
> --- a/plugins/python/example.py
> +++ b/plugins/python/example.py
> @@ -24,6 +24,9 @@
>  #   ><fs> mount /dev/sda1 /
>  #   ><fs> [etc]
> 
> +import nbdkit
> +import errno
> +
>  # This is the string used to store the emulated disk (initially all
>  # zero bytes).  There is one disk per nbdkit instance, so if you
>  # reconnect to the same server you should see the same disk.  You
> @@ -56,3 +59,11 @@ def pwrite(h, buf, offset):
>      global disk
>      end = offset + len (buf)
>      disk[offset:end] = buf
> +
> +def zero(h, count, offset, may_trim):
> +    global disk
> +    if may_trim:
> +        disk[offset:offset+count] = bytearray(count)
> +    else:
> +        nbdkit.set_error(errno.EOPNOTSUPP)
> +        raise Exception
> diff --git a/plugins/python/nbdkit-python-plugin.pod
b/plugins/python/nbdkit-python-plugin.pod
> index 8b3d08c..b78c0e2 100644
> --- a/plugins/python/nbdkit-python-plugin.pod
> +++ b/plugins/python/nbdkit-python-plugin.pod
> @@ -215,6 +215,26 @@ The body of your C<trim> function should
"punch a hole" in the
>  backing store.  If the trim fails, your function should throw an
>  exception, optionally using C<nbdkit.set_error> first.
> 
> +=item C<zero>
> +
> +(Optional)
> +
> + def zero(h, count, offset, may_trim):
> +   # no return value
> +
> +The body of your C<zero> function should ensure that C<count>
bytes
> +of the disk, starting at C<offset>, will read back as zero.  If
> +C<may_trim> is true, the operation may be optimized as a trim as
long
> +as subsequent reads see zeroes.
> +
> +NBD only supports whole writes, so your function should try to
> +write the whole region (perhaps requiring a loop).  If the write
> +fails or is partial, your function should throw an exception,
> +optionally using C<nbdkit.set_error> first.  In particular, if
> +you would like to automatically fall back to C<pwrite> (perhaps
> +because there is nothing to optimize if C<may_trim> is false),
> +use C<nbdkit.set_error(errno.EOPNOTSUPP)>.
> +
>  =back
> 
>  =head2 MISSING CALLBACKS
> diff --git a/plugins/python/python.c b/plugins/python/python.c
> index 4a0ff50..644ced4 100644
> --- a/plugins/python/python.c
> +++ b/plugins/python/python.c
> @@ -54,6 +54,8 @@
>  static const char *script;
>  static PyObject *module;
> 
> +static int last_error;
> +
>  static PyObject *
>  set_error (PyObject *self, PyObject *args)
>  {
> @@ -62,6 +64,7 @@ set_error (PyObject *self, PyObject *args)
>    if (!PyArg_ParseTuple(args, "i", &err))
>      return NULL;
>    nbdkit_set_error (err);
> +  last_error = err;
>    Py_RETURN_NONE;
>  }
> 
> @@ -441,6 +444,48 @@ py_trim (void *handle, uint32_t count, uint64_t
offset)
>  }
> 
>  static int
> +py_zero (void *handle, uint32_t count, uint64_t offset, int may_trim)
> +{
> +  PyObject *obj = handle;
> +  PyObject *fn;
> +  PyObject *args;
> +  PyObject *r;
> +
> +  if (callback_defined ("zero", &fn)) {
> +    PyErr_Clear ();
> +
> +    last_error = 0;
> +    args = PyTuple_New (4);
> +    Py_INCREF (obj); /* decremented by Py_DECREF (args) */
> +    PyTuple_SetItem (args, 0, obj);
> +    PyTuple_SetItem (args, 1, PyLong_FromUnsignedLongLong (count));
> +    PyTuple_SetItem (args, 2, PyLong_FromUnsignedLongLong (offset));
> +    PyTuple_SetItem (args, 3, PyBool_FromLong (may_trim));
> +    r = PyObject_CallObject (fn, args);
> +    Py_DECREF (fn);
> +    Py_DECREF (args);
> +    if (last_error == EOPNOTSUPP) {
> +      /* When user requests this particular error, we want to
> +         gracefully fall back, and to accomodate both a normal return
> +         and an exception. */
> +      nbdkit_debug ("zero requested falling back to pwrite");
> +      if (r)
> +        Py_DECREF (r);
> +      PyErr_Clear ();
> +      return -1;
> +    }
> +    if (check_python_failure ("zero") == -1)
> +      return -1;
> +    Py_DECREF (r);
> +    return 0;
> +  }
> +
> +  nbdkit_debug ("zero missing, falling back to pwrite");
> +  errno = EOPNOTSUPP;
> +  return -1;
> +}
> +
> +static int
>  py_can_write (void *handle)
>  {
>    PyObject *obj = handle;
> @@ -597,6 +642,7 @@ static struct nbdkit_plugin plugin = {
>    .pwrite            = py_pwrite,
>    .flush             = py_flush,
>    .trim              = py_trim,
> +  .zero              = py_zero,
>  };
> 
>  NBDKIT_REGISTER_PLUGIN(plugin)
> -- 
> 2.9.3
> 
> _______________________________________________
> Libguestfs mailing list
> Libguestfs@redhat.com
> https://www.redhat.com/mailman/listinfo/libguestfs
-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
libguestfs lets you edit virtual machines.  Supports shell scripting,
bindings from many languages.  http://libguestfs.org