8:58 p.m.
The last patch fixed the plugin interface to not use a bogus value
of errno from other language bindings, but hard-coding things to
EIO is not nice either.
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, and is intentionally wiped before
each callback.
Signed-off-by: Eric Blake <eblake(a)redhat.com>
---
v2: rebase on top of .errno_is_implicit
---
docs/nbdkit-plugin.pod | 59 ++++++++++++++++++++++++++++++++-----------------
include/nbdkit-plugin.h | 1 +
src/connections.c | 8 +++++--
src/errors.c | 6 +++++
src/internal.h | 2 ++
src/plugins.c | 10 +++++++--
src/tls.c | 30 +++++++++++++++++++++++--
7 files changed, 90 insertions(+), 26 deletions(-)
diff --git a/docs/nbdkit-plugin.pod b/docs/nbdkit-plugin.pod
index 5790f9e..4b3a492 100644
--- a/docs/nbdkit-plugin.pod
+++ b/docs/nbdkit-plugin.pod
@@ -143,12 +143,14 @@ 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. When a callback fails,
-the NBD protocol sends an error code to the client; the
-C<.errno_is_reliable> callback controls whether this error code
-will default to something based on C<errno>, or if it will just
-be hard-coded to C<EIO>.
+C<nbdkit_error> to report an error message; additionally, if the
+callback is involved in serving data, the plugin should call
+C<nbdkit_set_error> to influence the error code that will be sent to
+the client. These two functions can be called in either order. Then,
+the callback should return the appropriate error indication, eg. NULL
+or -1. If the call to C<nbdkit_set_error> is omitted while serving
+data, then the C<.errno_is_reliable> callback controls whether to fall
+back to the value of C<errno> or a hard-coded C<EIO>.
C<nbdkit_error> has the following prototype and works like
L<printf(3)>:
@@ -158,6 +160,12 @@ L<printf(3)>:
For convenience, C<nbdkit_error> preserves the value of C<errno>.
+C<nbdkit_set_error> can be called at any time, but only has an impact
+during callbacks for serving data, and only when the callback returns
+an indication of failure. It has the following prototype:
+
+ void nbdkit_set_error (int err);
+
=head1 FILENAMES AND PATHS
The server usually (not always) changes directory to C</> before it
@@ -382,13 +390,18 @@ C<.trim> callback has been defined.
int errno_is_reliable (void *handle);
-This is called during the option negotiation phase to find out if the
-handle wants to reflect C<errno> to the client on failures, or to just
-hard-code C<EIO>. Typically, a plugin written in C will want to use
-C<errno>, while a plugin written in any other language generally
-cannot control the value of C<errno> after the glue code of the
-language binding wrappers; therefore, this callback is not needed in
-the bindings of other languages.
+This is called during the option negotiation phase to find out what
+errors to reflect to the client. An error explicitly reported to
+C<nbdkit_set_error> takes precedence, but plugins written in C can
+often get by with implicitly using the value stored in C<errno>, when
+this callback returns true. On the other hand, a plugin written in any
+other language generally cannot control the value of C<errno> after
+the glue code of the language binding wrappers; returning false forces
+the use of C<EIO> as the fallback error rather than using an
+unreliable value in C<errno>. This callback is not needed in the
+bindings of other languages. The results of this callback do not
+matter if all data serving callbacks correctly call
+C<nbdkit_set_error>.
If there is an error, C<.errno_is_reliable> should call C<nbdkit_error>
with an error message and return C<-1>.
@@ -413,7 +426,8 @@ to indicate there was I<no> error.
If there is an error (including a short read which couldn't be
recovered from), C<.pread> should call C<nbdkit_error> with an error
-message and return C<-1>.
+message, and C<nbdkit_set_error> to record an appropriate error
+(unless C<errno> is sufficient), then return C<-1>.
=head2 C<.pwrite>
@@ -432,7 +446,8 @@ callback should return C<0> to indicate there was I<no>
error.
If there is an error (including a short write which couldn't be
recovered from), C<.pwrite> should call C<nbdkit_error> with an error
-message and return C<-1>.
+message, and C<nbdkit_set_error> to record an appropriate error
+(unless C<errno> is sufficient), then return C<-1>.
=head2 C<.flush>
@@ -444,7 +459,8 @@ completely written to a permanent medium. If that is not possible
then you can omit this callback.
If there is an error, C<.flush> should call C<nbdkit_error> with an
-error message and return C<-1>.
+error message, and C<nbdkit_set_error> to record an appropriate error
+(unless C<errno> is sufficient), then return C<-1>.
=head2 C<.trim>
@@ -455,7 +471,8 @@ in the backing store. If that is not possible then you can omit this
callback.
If there is an error, C<.trim> should call C<nbdkit_error> with an
-error message and return C<-1>.
+error message, and C<nbdkit_set_error> to record an appropriate error
+(unless C<errno> is sufficient), then return C<-1>.
=head2 C<.zero>
@@ -465,8 +482,9 @@ During the data serving phase, this callback is used to write
C<count>
bytes of zeroes at C<offset> in the backing store. If C<may_trim> is
non-zero, 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<errno> set to
-EOPNOTSUPP, then C<.pwrite> will be used instead.
+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.
The callback must write the whole C<count> bytes if it can. The NBD
protocol doesn't allow partial writes (instead, these would be
@@ -474,7 +492,8 @@ errors). If the whole C<count> bytes was written successfully,
the
callback should return C<0> to indicate there was I<no> error.
If there is an error, C<.zero> should call C<nbdkit_error> with an
-error message and return C<-1>.
+error message, and C<nbdkit_set_error> to record an appropriate error
+(unless C<errno> is sufficient), then return C<-1>.
=head1 THREADS
diff --git a/include/nbdkit-plugin.h b/include/nbdkit-plugin.h
index 8cb33a0..28872e0 100644
--- a/include/nbdkit-plugin.h
+++ b/include/nbdkit-plugin.h
@@ -93,6 +93,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 23e82a9..cf06ac5 100644
--- a/src/connections.c
+++ b/src/connections.c
@@ -610,9 +610,9 @@ validate_request (struct connection *conn,
static int
get_error (struct connection *conn)
{
- int ret = 0;
+ int ret = tls_get_error ();
- if (conn->errno_is_reliable)
+ if (!ret && conn->errno_is_reliable)
ret = errno;
return ret ? ret : EIO;
}
@@ -642,6 +642,10 @@ _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);
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 2129337..aa7dbd3 100644
--- a/src/internal.h
+++ b/src/internal.h
@@ -169,6 +169,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 d486f2d..837a54f 100644
--- a/src/plugins.c
+++ b/src/plugins.c
@@ -536,7 +536,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);
@@ -546,11 +546,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 && conn->errno_is_reliable)
+ 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..5d380e3 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,26 @@ 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;
+}
+
+/* This preserves errno, for convenience.
+ */
+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