[Libguestfs] [nbdkit PATCH v3 1/4] plugins: Don't use bogus errno from non-C plugins
Eric Blake
eblake at redhat.com
Fri Jan 27 02:58:34 UTC 2017
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 at 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
More information about the Libguestfs
mailing list