This cleans up the existing code base with regards to implicit use of errno from language bindings, then rebases the previous work in python on top of that. I'm still playing with the perl bindings, but got further after reading 'perldoc perlembed'. Eric Blake (4): plugins: Don't use bogus errno from non-C plugins plugins: Add new nbdkit_set_error() utility function python: Expose nbdkit_set_error to python script python: Support zero callback docs/nbdkit-plugin.pod | 64 +++++++++++++++++++++++---- include/nbdkit-plugin.h | 14 +++++- plugins/ocaml/nbdkit-ocaml-plugin.pod | 11 +++++ plugins/ocaml/ocaml.c | 12 ++++++ plugins/perl/nbdkit-perl-plugin.pod | 5 +++ plugins/perl/perl.c | 12 ++++++ plugins/python/example.py | 11 +++++ plugins/python/nbdkit-python-plugin.pod | 53 +++++++++++++++++++++-- plugins/python/python.c | 76 +++++++++++++++++++++++++++++++++ plugins/ruby/nbdkit-ruby-plugin.pod | 5 +++ plugins/ruby/ruby.c | 12 ++++++ src/connections.c | 36 ++++++++++++---- src/errors.c | 6 +++ src/internal.h | 4 ++ src/plugins.c | 25 ++++++++++- src/tls.c | 30 ++++++++++++- 16 files changed, 349 insertions(+), 27 deletions(-) -- 2.9.3
Eric Blake
2017-Jan-27 02:58 UTC
[Libguestfs] [nbdkit PATCH v3 1/4] plugins: Don't use bogus errno from non-C plugins
It is very unlikely that the value of errno after completing the
glue code in between the completion of the user's callback and
the point in time where we construct the client's reply is going
to be untouched, which means that we are likely to send the wrong
error code across the wire. Add a new callback which determines
whether we can trust errno; defaulting to true for C plugins, where
we can trace all code in between to ensure errno is not clobbered,
and always false for other language bindings.
Signed-off-by: Eric Blake <eblake@redhat.com>
---
docs/nbdkit-plugin.pod | 31 +++++++++++++++++++++++++++++--
include/nbdkit-plugin.h | 13 ++++++++++++-
plugins/ocaml/nbdkit-ocaml-plugin.pod | 11 +++++++++++
plugins/ocaml/ocaml.c | 12 ++++++++++++
plugins/perl/nbdkit-perl-plugin.pod | 5 +++++
plugins/perl/perl.c | 12 ++++++++++++
plugins/python/nbdkit-python-plugin.pod | 5 +++++
plugins/python/python.c | 12 ++++++++++++
plugins/ruby/nbdkit-ruby-plugin.pod | 5 +++++
plugins/ruby/ruby.c | 12 ++++++++++++
src/connections.c | 32 +++++++++++++++++++++++---------
src/internal.h | 2 ++
src/plugins.c | 15 +++++++++++++++
13 files changed, 155 insertions(+), 12 deletions(-)
diff --git a/docs/nbdkit-plugin.pod b/docs/nbdkit-plugin.pod
index ee6bbd5..5790f9e 100644
--- a/docs/nbdkit-plugin.pod
+++ b/docs/nbdkit-plugin.pod
@@ -144,12 +144,19 @@ is called once just before the plugin is unloaded from
memory.
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.
+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> has the following prototype and works like
L<printf(3)>:
void nbdkit_error (const char *fs, ...);
+ void nbdkit_verror (const char *fs, va_list args);
+
+For convenience, C<nbdkit_error> preserves the value of C<errno>.
=head1 FILENAMES AND PATHS
@@ -371,6 +378,24 @@ error message and return C<-1>.
This callback is not required. If omitted, then we return true iff a
C<.trim> callback has been defined.
+=head2 C<.errno_is_reliable>
+
+ 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.
+
+If there is an error, C<.errno_is_reliable> should call
C<nbdkit_error>
+with an error message and return C<-1>.
+
+This callback is not required. If omitted, then we return true for
+a plugin written in C, and false for plugins in other languages.
+
=head2 C<.pread>
int pread (void *handle, void *buf, uint32_t count, uint64_t offset);
@@ -440,7 +465,7 @@ 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 errno set to
+If this callback is omitted, or if it fails with C<errno> set to
EOPNOTSUPP, then C<.pwrite> will be used instead.
The callback must write the whole C<count> bytes if it can. The NBD
@@ -523,7 +548,9 @@ C<nbdkit_debug>, which has the following prototype and
works like
L<printf(3)>:
void nbdkit_debug (const char *fs, ...);
+ void nbdkit_vdebug (const char *fs, va_list args);
+For convenience, C<nbdkit_debug> preserves the value of C<errno>.
Note that C<nbdkit_debug> only prints things when the server is in
verbose mode (I<-v> option).
diff --git a/include/nbdkit-plugin.h b/include/nbdkit-plugin.h
index 3d25642..8cb33a0 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
@@ -47,10 +47,19 @@
#define NBDKIT_API_VERSION 1
struct nbdkit_plugin {
+ /* Do not set these fields directly; use NBDKIT_REGISTER_PLUGIN.
+ * They exist so that we can support plugins compiled against
+ * one version of the header with a runtime compiled against a
+ * different version with more (or fewer) fields.
+ */
uint64_t _struct_size;
int _api_version;
int _thread_model;
+ /* Plugins are responsible for these fields; see the documentation
+ * for semantics, and which fields are optional. New fields will
+ * only be added at the end of the struct.
+ */
const char *name;
const char *longname;
const char *version;
@@ -79,6 +88,8 @@ struct nbdkit_plugin {
int (*trim) (void *handle, uint32_t count, uint64_t offset);
int (*zero) (void *handle, uint32_t count, uint64_t offset, int may_trim);
+ int (*errno_is_reliable) (void *handle);
+
/* int (*set_exportname) (void *handle, const char *exportname); */
};
diff --git a/plugins/ocaml/nbdkit-ocaml-plugin.pod
b/plugins/ocaml/nbdkit-ocaml-plugin.pod
index 8f67d9b..250c1da 100644
--- a/plugins/ocaml/nbdkit-ocaml-plugin.pod
+++ b/plugins/ocaml/nbdkit-ocaml-plugin.pod
@@ -89,6 +89,17 @@ handle struct in your plugin:
printf "handle ID = %d\n" handle.h_id;
(* ... *)
+=head2 MISSING CALLBACKS
+
+=over 4
+
+=item Missing: C<errno_is_reliable>
+
+This is not needed because the process of gluing OCaml code into C cannot
+reliably use C<errno>.
+
+=back
+
=head2 THREADS
The first parameter of C<NBDKit.register_plugin> is the thread model,
diff --git a/plugins/ocaml/ocaml.c b/plugins/ocaml/ocaml.c
index e2b433e..5d7aeeb 100644
--- a/plugins/ocaml/ocaml.c
+++ b/plugins/ocaml/ocaml.c
@@ -499,6 +499,17 @@ SET(pwrite)
SET(flush)
SET(trim)
+/* We can't guarantee that errno is stable across language binding
+ * glue code, so this callback is implemented in C only, and not
+ * exposed in OCaml.
+ */
+static int
+plugin_ocaml_errno_is_reliable (void *handle)
+{
+ return 0;
+}
+
+/* We can't directly use NBDKIT_REGISTER_PLUGIN(). */
struct nbdkit_plugin *
plugin_init (void)
{
@@ -506,5 +517,6 @@ plugin_init (void)
fprintf (stderr, "error: OCaml code did not call
NBDKit.register_plugin\n");
exit (EXIT_FAILURE);
}
+ plugin.errno_is_reliable = plugin_ocaml_errno_is_reliable;
return &plugin;
}
diff --git a/plugins/perl/nbdkit-perl-plugin.pod
b/plugins/perl/nbdkit-perl-plugin.pod
index 3d0ce14..c02f8ef 100644
--- a/plugins/perl/nbdkit-perl-plugin.pod
+++ b/plugins/perl/nbdkit-perl-plugin.pod
@@ -282,6 +282,11 @@ If there is an error, the function should call
C<die>.
These are not needed because you can just use regular Perl C<BEGIN>
and C<END> constructs.
+=item Missing: C<errno_is_reliable>
+
+This is not needed because the process of gluing Perl code into C cannot
+reliably use C<errno>.
+
=item Missing: C<name>, C<version>, C<longname>,
C<description>, C<config_help>
These are not yet supported.
diff --git a/plugins/perl/perl.c b/plugins/perl/perl.c
index 935e1ba..ec82ecb 100644
--- a/plugins/perl/perl.c
+++ b/plugins/perl/perl.c
@@ -584,6 +584,16 @@ perl_trim (void *handle, uint32_t count, uint64_t offset)
return 0;
}
+/* We can't guarantee that errno is stable across language binding
+ * glue code, so this callback is implemented in C only, and not
+ * exposed in Perl.
+ */
+static int
+perl_errno_is_reliable (void *handle)
+{
+ return 0;
+}
+
#define perl_config_help \
"script=<FILENAME> (required) The Perl plugin to run.\n"
\
"[other arguments may be used by the plugin that you load]"
@@ -614,6 +624,8 @@ static struct nbdkit_plugin plugin = {
.pwrite = perl_pwrite,
.flush = perl_flush,
.trim = perl_trim,
+
+ .errno_is_reliable = perl_errno_is_reliable,
};
NBDKIT_REGISTER_PLUGIN(plugin)
diff --git a/plugins/python/nbdkit-python-plugin.pod
b/plugins/python/nbdkit-python-plugin.pod
index 9b0f0ef..63e5f96 100644
--- a/plugins/python/nbdkit-python-plugin.pod
+++ b/plugins/python/nbdkit-python-plugin.pod
@@ -207,6 +207,11 @@ backing store.
These are not needed because you can just use ordinary Python
constructs.
+=item Missing: C<errno_is_reliable>
+
+This is not needed because the process of gluing Python code into C cannot
+reliably use C<errno>.
+
=item Missing: C<name>, C<version>, C<longname>,
C<description>, C<config_help>
These are not yet supported.
diff --git a/plugins/python/python.c b/plugins/python/python.c
index 0504715..7e9fc1e 100644
--- a/plugins/python/python.c
+++ b/plugins/python/python.c
@@ -549,6 +549,16 @@ py_can_trim (void *handle)
return 0;
}
+/* We can't guarantee that errno is stable across language binding
+ * glue code, so this callback is implemented in C only, and not
+ * exposed in Python.
+ */
+static int
+py_errno_is_reliable (void *handle)
+{
+ return 0;
+}
+
#define py_config_help \
"script=<FILENAME> (required) The Python plugin to
run.\n" \
"[other arguments may be used by the plugin that you load]"
@@ -579,6 +589,8 @@ static struct nbdkit_plugin plugin = {
.pwrite = py_pwrite,
.flush = py_flush,
.trim = py_trim,
+
+ .errno_is_reliable = py_errno_is_reliable,
};
NBDKIT_REGISTER_PLUGIN(plugin)
diff --git a/plugins/ruby/nbdkit-ruby-plugin.pod
b/plugins/ruby/nbdkit-ruby-plugin.pod
index 8511479..3f49cb4 100644
--- a/plugins/ruby/nbdkit-ruby-plugin.pod
+++ b/plugins/ruby/nbdkit-ruby-plugin.pod
@@ -208,6 +208,11 @@ backing store.
These are not needed because you can just use ordinary Ruby
constructs.
+=item Missing: C<errno_is_reliable>
+
+This is not needed because the process of gluing Ruby code into C cannot
+reliably use C<errno>.
+
=item Missing: C<name>, C<version>, C<longname>,
C<description>, C<config_help>
These are not yet supported.
diff --git a/plugins/ruby/ruby.c b/plugins/ruby/ruby.c
index 6d1434c..2da14f7 100644
--- a/plugins/ruby/ruby.c
+++ b/plugins/ruby/ruby.c
@@ -417,6 +417,16 @@ plugin_rb_can_trim (void *handle)
return RTEST (rv);
}
+/* We can't guarantee that errno is stable across language binding
+ * glue code, so this callback is implemented in C only, and not
+ * exposed in Ruby.
+ */
+static int
+plugin_rb_errno_is_reliable (void *handle)
+{
+ return 0;
+}
+
#define plugin_rb_config_help \
"script=<FILENAME> (required) The Ruby plugin to run.\n"
\
"[other arguments may be used by the plugin that you load]"
@@ -450,6 +460,8 @@ static struct nbdkit_plugin plugin = {
.pwrite = plugin_rb_pwrite,
.flush = plugin_rb_flush,
.trim = plugin_rb_trim,
+
+ .errno_is_reliable = plugin_rb_errno_is_reliable,
};
NBDKIT_REGISTER_PLUGIN(plugin)
diff --git a/src/connections.c b/src/connections.c
index c0f0567..23e82a9 100644
--- a/src/connections.c
+++ b/src/connections.c
@@ -498,7 +498,10 @@ negotiate_handshake (struct connection *conn)
int r;
plugin_lock_request (conn);
- if (!newstyle)
+ conn->errno_is_reliable = plugin_errno_is_reliable (conn);
+ if (conn->errno_is_reliable < 0)
+ r = -1;
+ else if (!newstyle)
r = _negotiate_handshake_oldstyle (conn);
else
r = _negotiate_handshake_newstyle (conn);
@@ -602,6 +605,18 @@ validate_request (struct connection *conn,
return 1; /* Commands validates. */
}
+/* Grab the appropriate error value.
+ */
+static int
+get_error (struct connection *conn)
+{
+ int ret = 0;
+
+ if (conn->errno_is_reliable)
+ ret = errno;
+ return ret ? ret : EIO;
+}
+
/* 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,8 +626,7 @@ 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
- * set) and returns 0.
+ * On read/write errors, sets *error appropriately and returns 0.
*/
static int
_handle_request (struct connection *conn,
@@ -632,7 +646,7 @@ _handle_request (struct connection *conn,
case NBD_CMD_READ:
r = plugin_pread (conn, buf, count, offset);
if (r == -1) {
- *error = errno ? errno : EIO;
+ *error = get_error (conn);
return 0;
}
break;
@@ -640,7 +654,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 (conn);
return 0;
}
break;
@@ -648,7 +662,7 @@ _handle_request (struct connection *conn,
case NBD_CMD_FLUSH:
r = plugin_flush (conn);
if (r == -1) {
- *error = errno ? errno : EIO;
+ *error = get_error (conn);
return 0;
}
break;
@@ -656,7 +670,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 (conn);
return 0;
}
break;
@@ -664,7 +678,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 (conn);
return 0;
}
break;
@@ -676,7 +690,7 @@ _handle_request (struct connection *conn,
if (flush_after_command) {
r = plugin_flush (conn);
if (r == -1) {
- *error = errno ? errno : EIO;
+ *error = get_error (conn);
return 0;
}
}
diff --git a/src/internal.h b/src/internal.h
index 4632f51..2129337 100644
--- a/src/internal.h
+++ b/src/internal.h
@@ -120,6 +120,7 @@ struct connection {
int can_flush;
int is_rotational;
int can_trim;
+ int errno_is_reliable;
};
extern int handle_single_connection (int sockin, int sockout);
@@ -143,6 +144,7 @@ extern void plugin_unlock_request (struct connection *conn);
extern int plugin_open (struct connection *conn, int readonly);
extern void plugin_close (struct connection *conn);
extern int64_t plugin_get_size (struct connection *conn);
+extern int plugin_errno_is_reliable (struct connection *conn);
extern int plugin_can_write (struct connection *conn);
extern int plugin_can_flush (struct connection *conn);
extern int plugin_is_rotational (struct connection *conn);
diff --git a/src/plugins.c b/src/plugins.c
index 92f8505..d486f2d 100644
--- a/src/plugins.c
+++ b/src/plugins.c
@@ -395,6 +395,21 @@ plugin_get_size (struct connection *conn)
}
int
+plugin_errno_is_reliable (struct connection *conn)
+{
+ assert (dl);
+ assert (conn->handle);
+
+ debug ("errno_is_reliable");
+
+ if (plugin.errno_is_reliable)
+ return plugin.errno_is_reliable (conn->handle);
+
+ /* Default to 1, for backwards compatibility (correct for C plugins) */
+ return 1;
+}
+
+int
plugin_can_write (struct connection *conn)
{
assert (dl);
--
2.9.3
Eric Blake
2017-Jan-27 02:58 UTC
[Libguestfs] [nbdkit PATCH v3 2/4] plugins: Add new nbdkit_set_error() utility function
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@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
Eric Blake
2017-Jan-27 02:58 UTC
[Libguestfs] [nbdkit PATCH v3 3/4] python: Expose nbdkit_set_error to python script
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>
---
v2: tweak docs
---
plugins/python/nbdkit-python-plugin.pod | 30 +++++++++++++++++++++++++-----
plugins/python/python.c | 18 ++++++++++++++++++
2 files changed, 43 insertions(+), 5 deletions(-)
diff --git a/plugins/python/nbdkit-python-plugin.pod
b/plugins/python/nbdkit-python-plugin.pod
index 63e5f96..a1e3d2b 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; if omitted, the client will see an error of C<EIO>.
=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
@@ -210,7 +229,8 @@ constructs.
=item Missing: C<errno_is_reliable>
This is not needed because the process of gluing Python code into C cannot
-reliably use C<errno>.
+reliably use C<errno>. Instead, call C<nbdkit.set_error> when
reporting
+a failure.
=item Missing: C<name>, C<version>, C<longname>,
C<description>, C<config_help>
diff --git a/plugins/python/python.c b/plugins/python/python.c
index 7e9fc1e..631411e 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
Eric Blake
2017-Jan-27 02:58 UTC
[Libguestfs] [nbdkit PATCH v3 4/4] python: Support zero callback
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>
---
v2: rebase to .errno_is_reliable
---
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 a1e3d2b..1c57e15 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 631411e..895a361 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");
+ nbdkit_set_error (EOPNOTSUPP);
+ return -1;
+}
+
+static int
py_can_write (void *handle)
{
PyObject *obj = handle;
@@ -607,6 +652,7 @@ static struct nbdkit_plugin plugin = {
.pwrite = py_pwrite,
.flush = py_flush,
.trim = py_trim,
+ .zero = py_zero,
.errno_is_reliable = py_errno_is_reliable,
};
--
2.9.3
Richard W.M. Jones
2017-Jan-27 10:11 UTC
Re: [Libguestfs] [nbdkit PATCH v3 1/4] plugins: Don't use bogus errno from non-C plugins
On Thu, Jan 26, 2017 at 08:58:34PM -0600, Eric Blake wrote:> diff --git a/plugins/ocaml/ocaml.c b/plugins/ocaml/ocaml.c > index e2b433e..5d7aeeb 100644 > --- a/plugins/ocaml/ocaml.c > +++ b/plugins/ocaml/ocaml.c > @@ -499,6 +499,17 @@ SET(pwrite) > SET(flush) > SET(trim) > > +/* We can't guarantee that errno is stable across language binding > + * glue code, so this callback is implemented in C only, and not > + * exposed in OCaml. > + */ > +static int > +plugin_ocaml_errno_is_reliable (void *handle) > +{ > + return 0; > +}Actually OCaml is a real compiled language, and the call from C to OCaml code (via caml_callback_exn) is a short piece of asm which preserves errno. However you'll need to save errno around caml_enter_blocking_section since that unblocks and processes signals. IOW: static int pread_wrapper (void *h, void *buf, uint32_t count, uint64_t offset) { CAMLparam0 (); CAMLlocal3 (rv, strv, offsetv); + int saved_errno; ... rv = caml_callback3_exn (pread_fn, *(value *) h, strv, offsetv); + saved_errno = errno; if (Is_exception_result (rv)) { nbdkit_error ("%s", caml_format_exception (Extract_exception (rv))); caml_enter_blocking_section (); + errno = saved_errno; CAMLreturnT (int, -1); } Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-p2v converts physical machines to virtual machines. Boot with a live CD or over the network (PXE) and turn machines into KVM guests. http://libguestfs.org/virt-v2v
Richard W.M. Jones
2017-Jan-27 10:12 UTC
Re: [Libguestfs] [nbdkit PATCH v3 4/4] python: Support zero callback
Patch 2, 3 & 4 look fine. 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/
Apparently Analagous Threads
- Re: [nbdkit PATCH v3 1/4] plugins: Don't use bogus errno from non-C plugins
- [PATCH nbdkit v2 1/2] ocaml: Change pread method to avoid leaking heap memory.
- [PATCH nbdkit v2 0/2] Be careful not to leak server heap memory to the client.
- [nbdkit PATCH v3 0/4] bind .zero to Python
- [nbdkit PATCH v2 08/24] ocaml: Implement .cache script callback