[Libguestfs] [nbdkit PATCH v3 02/15] backend: Rework internal error return semantics
Eric Blake
eblake at redhat.com
Thu Mar 8 23:02:58 UTC 2018
Previously, we let a plugin set an error in either thread-local
storage (nbdkit_set_error()) or errno, then connections.c would
decode which error to use. But with filters in the mix, it is
very difficult for a filter to know what error was set by the
plugin (particularly since nbdkit_set_error() has no public
counterpart for reading the thread-local storage). What's more,
if a filter does any non-trivial processing after calling into
next_ops, it is very probable that errno might be corrupted,
which would affect the error returned by a plugin that relied
on errno but not the error stored in thread-local storage.
Furthermore, in connections.c, handle_request() already has to
return a positive error value, as recv_request_send_reply()
then uses that value for determining what to send over the wire
to the client; so getting to the positive value sooner rather
than later can simplify the call stack.
We make the change in two steps: this patch changes the internal
interface to return the error value in a separate parameter,
and guarantees that filters see a valid value in errno; then a
later patch adjusts the public filter API to be more useful and
to more closely mirror the internal API. The err parameter must
be set if the return is -1, and is ignored otherwise; for now,
it is only set by plugins.c (as the filter code needs to expose
it to the public filter callback API before it can be usefully
manipulated). With the change in decoding location, the backend
interface no longer needs an .errno_is_preserved() callback.
Signed-off-by: Eric Blake <eblake at redhat.com>
---
v3: split into two parts, with error value sent by parameter
rather than by return value; more commit message details; fix
pod rendering errors
v2: more wording tweaks to documentation
---
src/internal.h | 16 ++++---
src/connections.c | 39 +++++++----------
src/filters.c | 128 ++++++++++++++++++++++++++++++++++--------------------
src/plugins.c | 83 +++++++++++++++++++++--------------
4 files changed, 157 insertions(+), 109 deletions(-)
diff --git a/src/internal.h b/src/internal.h
index 3cbfde5..be79e10 100644
--- a/src/internal.h
+++ b/src/internal.h
@@ -176,7 +176,6 @@ struct backend {
void (*dump_fields) (struct backend *);
void (*config) (struct backend *, const char *key, const char *value);
void (*config_complete) (struct backend *);
- int (*errno_is_preserved) (struct backend *);
int (*open) (struct backend *, struct connection *conn, int readonly);
int (*prepare) (struct backend *, struct connection *conn);
int (*finalize) (struct backend *, struct connection *conn);
@@ -186,11 +185,16 @@ struct backend {
int (*can_flush) (struct backend *, struct connection *conn);
int (*is_rotational) (struct backend *, struct connection *conn);
int (*can_trim) (struct backend *, struct connection *conn);
- int (*pread) (struct backend *, struct connection *conn, void *buf, uint32_t count, uint64_t offset, uint32_t flags);
- int (*pwrite) (struct backend *, struct connection *conn, const void *buf, uint32_t count, uint64_t offset, uint32_t flags);
- int (*flush) (struct backend *, struct connection *conn, uint32_t flags);
- int (*trim) (struct backend *, struct connection *conn, uint32_t count, uint64_t offset, uint32_t flags);
- int (*zero) (struct backend *, struct connection *conn, uint32_t count, uint64_t offset, uint32_t flags);
+ int (*pread) (struct backend *, struct connection *conn, void *buf,
+ uint32_t count, uint64_t offset, uint32_t flags, int *err);
+ int (*pwrite) (struct backend *, struct connection *conn, const void *buf,
+ uint32_t count, uint64_t offset, uint32_t flags, int *err);
+ int (*flush) (struct backend *, struct connection *conn, uint32_t flags,
+ int *err);
+ int (*trim) (struct backend *, struct connection *conn, uint32_t count,
+ uint64_t offset, uint32_t flags, int *err);
+ int (*zero) (struct backend *, struct connection *conn, uint32_t count,
+ uint64_t offset, uint32_t flags, int *err);
};
/* plugins.c */
diff --git a/src/connections.c b/src/connections.c
index c25d316..03f59f7 100644
--- a/src/connections.c
+++ b/src/connections.c
@@ -43,6 +43,7 @@
#include <endian.h>
#include <sys/types.h>
#include <stddef.h>
+#include <assert.h>
#include <pthread.h>
@@ -911,18 +912,6 @@ validate_request (struct connection *conn,
return true; /* Command validates. */
}
-/* Grab the appropriate error value.
- */
-static int
-get_error (struct connection *conn)
-{
- int ret = threadlocal_get_error ();
-
- if (!ret && backend->errno_is_preserved (backend))
- 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
@@ -940,34 +929,35 @@ handle_request (struct connection *conn,
{
uint32_t f = 0;
bool fua = conn->can_flush && (flags & NBD_CMD_FLAG_FUA);
+ int err = 0;
- /* The plugin should call nbdkit_set_error() to request a particular
- error, otherwise we fallback to errno or EIO. */
+ /* Clear the error, so that we know if the plugin calls
+ * nbdkit_set_error() or relied on errno. */
threadlocal_set_error (0);
switch (cmd) {
case NBD_CMD_READ:
- if (backend->pread (backend, conn, buf, count, offset, 0) == -1)
- return get_error (conn);
+ if (backend->pread (backend, conn, buf, count, offset, 0, &err) == -1)
+ return err;
break;
case NBD_CMD_WRITE:
if (fua)
f |= NBDKIT_FLAG_FUA;
- if (backend->pwrite (backend, conn, buf, count, offset, f) == -1)
- return get_error (conn);
+ if (backend->pwrite (backend, conn, buf, count, offset, f, &err) == -1)
+ return err;
break;
case NBD_CMD_FLUSH:
- if (backend->flush (backend, conn, 0) == -1)
- return get_error (conn);
+ if (backend->flush (backend, conn, 0, &err) == -1)
+ return err;
break;
case NBD_CMD_TRIM:
if (fua)
f |= NBDKIT_FLAG_FUA;
- if (backend->trim (backend, conn, count, offset, f) == -1)
- return get_error (conn);
+ if (backend->trim (backend, conn, count, offset, f, &err) == -1)
+ return err;
break;
case NBD_CMD_WRITE_ZEROES:
@@ -975,8 +965,8 @@ handle_request (struct connection *conn,
f |= NBDKIT_FLAG_MAY_TRIM;
if (fua)
f |= NBDKIT_FLAG_FUA;
- if (backend->zero (backend, conn, count, offset, f) == -1)
- return get_error (conn);
+ if (backend->zero (backend, conn, count, offset, f, &err) == -1)
+ return err;
break;
default:
@@ -1129,6 +1119,7 @@ recv_request_send_reply (struct connection *conn)
else {
lock_request (conn);
error = handle_request (conn, cmd, flags, offset, count, buf);
+ assert ((int) error >= 0);
unlock_request (conn);
}
diff --git a/src/filters.c b/src/filters.c
index 6949548..ae5edfb 100644
--- a/src/filters.c
+++ b/src/filters.c
@@ -106,14 +106,6 @@ filter_thread_model (struct backend *b)
/* These are actually passing through to the final plugin, hence
* the function names.
*/
-static int
-plugin_errno_is_preserved (struct backend *b)
-{
- struct backend_filter *f = container_of (b, struct backend_filter, backend);
-
- return f->backend.next->errno_is_preserved (f->backend.next);
-}
-
static const char *
plugin_name (struct backend *b)
{
@@ -300,28 +292,46 @@ static int
next_pread (void *nxdata, void *buf, uint32_t count, uint64_t offset)
{
struct b_conn *b_conn = nxdata;
- return b_conn->b->pread (b_conn->b, b_conn->conn, buf, count, offset, 0);
+ int err = 0;
+ int r = b_conn->b->pread (b_conn->b, b_conn->conn, buf, count, offset, 0,
+ &err);
+ if (r == -1)
+ errno = err;
+ return r;
}
static int
next_pwrite (void *nxdata, const void *buf, uint32_t count, uint64_t offset)
{
struct b_conn *b_conn = nxdata;
- return b_conn->b->pwrite (b_conn->b, b_conn->conn, buf, count, offset, 0);
+ int err = 0;
+ int r = b_conn->b->pwrite (b_conn->b, b_conn->conn, buf, count, offset, 0,
+ &err);
+ if (r == -1)
+ errno = err;
+ return r;
}
static int
next_flush (void *nxdata)
{
struct b_conn *b_conn = nxdata;
- return b_conn->b->flush (b_conn->b, b_conn->conn, 0);
+ int err = 0;
+ int r = b_conn->b->flush (b_conn->b, b_conn->conn, 0, &err);
+ if (r == -1)
+ errno = err;
+ return r;
}
static int
next_trim (void *nxdata, uint32_t count, uint64_t offset)
{
struct b_conn *b_conn = nxdata;
- return b_conn->b->trim (b_conn->b, b_conn->conn, count, offset, 0);
+ int err = 0;
+ int r = b_conn->b->trim (b_conn->b, b_conn->conn, count, offset, 0, &err);
+ if (r == -1)
+ errno = err;
+ return r;
}
static int
@@ -329,11 +339,16 @@ next_zero (void *nxdata, uint32_t count, uint64_t offset, int may_trim)
{
struct b_conn *b_conn = nxdata;
uint32_t f = 0;
+ int err = 0;
+ int r;
if (may_trim)
f |= NBDKIT_FLAG_MAY_TRIM;
- return b_conn->b->zero (b_conn->b, b_conn->conn, count, offset, f);
+ r = b_conn->b->zero (b_conn->b, b_conn->conn, count, offset, f, &err);
+ if (r == -1)
+ errno = err;
+ return r;
}
static struct nbdkit_next_ops next_ops = {
@@ -457,7 +472,7 @@ filter_can_trim (struct backend *b, struct connection *conn)
static int
filter_pread (struct backend *b, struct connection *conn,
void *buf, uint32_t count, uint64_t offset,
- uint32_t flags)
+ uint32_t flags, int *err)
{
struct backend_filter *f = container_of (b, struct backend_filter, backend);
void *handle = connection_get_handle (conn, f->backend.i);
@@ -465,41 +480,50 @@ filter_pread (struct backend *b, struct connection *conn,
assert (flags == 0);
- debug ("pread count=%" PRIu32 " offset=%" PRIu64, count, offset);
+ debug ("pread count=%" PRIu32 " offset=%" PRIu64 " flags=0x%" PRIx32,
+ count, offset, flags);
- if (f->filter.pread)
- return f->filter.pread (&next_ops, &nxdata, handle,
- buf, count, offset);
+ if (f->filter.pread) {
+ int r = f->filter.pread (&next_ops, &nxdata, handle,
+ buf, count, offset);
+ if (r == -1)
+ *err = errno;
+ return r;
+ }
else
return f->backend.next->pread (f->backend.next, conn,
- buf, count, offset, flags);
+ buf, count, offset, flags, err);
}
static int
filter_pwrite (struct backend *b, struct connection *conn,
const void *buf, uint32_t count, uint64_t offset,
- uint32_t flags)
+ uint32_t flags, int *err)
{
struct backend_filter *f = container_of (b, struct backend_filter, backend);
void *handle = connection_get_handle (conn, f->backend.i);
struct b_conn nxdata = { .b = f->backend.next, .conn = conn };
- bool fua = flags & NBDKIT_FLAG_FUA;
assert (!(flags & ~NBDKIT_FLAG_FUA));
- debug ("pwrite count=%" PRIu32 " offset=%" PRIu64 " fua=%d",
- count, offset, fua);
+ debug ("pwrite count=%" PRIu32 " offset=%" PRIu64 " flags=0x%" PRIx32,
+ count, offset, flags);
- if (f->filter.pwrite)
- return f->filter.pwrite (&next_ops, &nxdata, handle,
- buf, count, offset);
+ if (f->filter.pwrite) {
+ int r = f->filter.pwrite (&next_ops, &nxdata, handle,
+ buf, count, offset);
+ if (r == -1)
+ *err = errno;
+ return r;
+ }
else
return f->backend.next->pwrite (f->backend.next, conn,
- buf, count, offset, flags);
+ buf, count, offset, flags, err);
}
static int
-filter_flush (struct backend *b, struct connection *conn, uint32_t flags)
+filter_flush (struct backend *b, struct connection *conn, uint32_t flags,
+ int *err)
{
struct backend_filter *f = container_of (b, struct backend_filter, backend);
void *handle = connection_get_handle (conn, f->backend.i);
@@ -507,18 +531,22 @@ filter_flush (struct backend *b, struct connection *conn, uint32_t flags)
assert (flags == 0);
- debug ("flush");
+ debug ("flush flags=0x%" PRIx32, flags);
- if (f->filter.flush)
- return f->filter.flush (&next_ops, &nxdata, handle);
+ if (f->filter.flush) {
+ int r = f->filter.flush (&next_ops, &nxdata, handle);
+ if (r == -1)
+ *err = errno;
+ return r;
+ }
else
- return f->backend.next->flush (f->backend.next, conn, flags);
+ return f->backend.next->flush (f->backend.next, conn, flags, err);
}
static int
filter_trim (struct backend *b, struct connection *conn,
uint32_t count, uint64_t offset,
- uint32_t flags)
+ uint32_t flags, int *err)
{
struct backend_filter *f = container_of (b, struct backend_filter, backend);
void *handle = connection_get_handle (conn, f->backend.i);
@@ -526,34 +554,43 @@ filter_trim (struct backend *b, struct connection *conn,
assert (flags == 0);
- debug ("trim count=%" PRIu32 " offset=%" PRIu64, count, offset);
+ debug ("trim count=%" PRIu32 " offset=%" PRIu64 " flags=0x%" PRIx32,
+ count, offset, flags);
- if (f->filter.trim)
- return f->filter.trim (&next_ops, &nxdata, handle, count, offset);
+ if (f->filter.trim) {
+ int r = f->filter.trim (&next_ops, &nxdata, handle, count, offset);
+ if (r == -1)
+ *err = errno;
+ return r;
+ }
else
- return f->backend.next->trim (f->backend.next, conn, count, offset, flags);
+ return f->backend.next->trim (f->backend.next, conn, count, offset, flags,
+ err);
}
static int
filter_zero (struct backend *b, struct connection *conn,
- uint32_t count, uint64_t offset, uint32_t flags)
+ uint32_t count, uint64_t offset, uint32_t flags, int *err)
{
struct backend_filter *f = container_of (b, struct backend_filter, backend);
void *handle = connection_get_handle (conn, f->backend.i);
struct b_conn nxdata = { .b = f->backend.next, .conn = conn };
- int may_trim = (flags & NBDKIT_FLAG_MAY_TRIM) != 0;
assert (!(flags & ~(NBDKIT_FLAG_MAY_TRIM | NBDKIT_FLAG_FUA)));
- debug ("zero count=%" PRIu32 " offset=%" PRIu64 " may_trim=%d",
- count, offset, may_trim);
+ debug ("zero count=%" PRIu32 " offset=%" PRIu64 " flags=0x%" PRIx32,
+ count, offset, flags);
- if (f->filter.zero)
- return f->filter.zero (&next_ops, &nxdata, handle,
- count, offset, may_trim);
+ if (f->filter.zero) {
+ int r = f->filter.zero (&next_ops, &nxdata, handle,
+ count, offset, !!(flags & NBDKIT_FLAG_MAY_TRIM));
+ if (r == -1)
+ *err = errno;
+ return r;
+ }
else
return f->backend.next->zero (f->backend.next, conn,
- count, offset, flags);
+ count, offset, flags, err);
}
static struct backend filter_functions = {
@@ -566,7 +603,6 @@ static struct backend filter_functions = {
.dump_fields = filter_dump_fields,
.config = filter_config,
.config_complete = filter_config_complete,
- .errno_is_preserved = plugin_errno_is_preserved,
.open = filter_open,
.prepare = filter_prepare,
.finalize = filter_finalize,
diff --git a/src/plugins.c b/src/plugins.c
index 280887d..d5a2a65 100644
--- a/src/plugins.c
+++ b/src/plugins.c
@@ -226,14 +226,6 @@ plugin_config_complete (struct backend *b)
exit (EXIT_FAILURE);
}
-static int
-plugin_errno_is_preserved (struct backend *b)
-{
- struct backend_plugin *p = container_of (b, struct backend_plugin, backend);
-
- return p->plugin.errno_is_preserved;
-}
-
static int
plugin_open (struct backend *b, struct connection *conn, int readonly)
{
@@ -357,11 +349,25 @@ plugin_can_trim (struct backend *b, struct connection *conn)
return p->plugin.trim != NULL;
}
+/* Grab the appropriate error value.
+ */
+static int
+get_error (struct backend_plugin *p)
+{
+ int ret = threadlocal_get_error ();
+
+ if (!ret && p->plugin.errno_is_preserved)
+ ret = errno;
+ return ret ? ret : EIO;
+}
+
static int
plugin_pread (struct backend *b, struct connection *conn,
- void *buf, uint32_t count, uint64_t offset, uint32_t flags)
+ void *buf, uint32_t count, uint64_t offset, uint32_t flags,
+ int *err)
{
struct backend_plugin *p = container_of (b, struct backend_plugin, backend);
+ int r;
assert (connection_get_handle (conn, 0));
assert (p->plugin.pread != NULL);
@@ -369,30 +375,40 @@ plugin_pread (struct backend *b, struct connection *conn,
debug ("pread count=%" PRIu32 " offset=%" PRIu64, count, offset);
- return p->plugin.pread (connection_get_handle (conn, 0), buf, count, offset);
+ r = p->plugin.pread (connection_get_handle (conn, 0), buf, count, offset);
+ if (r == -1)
+ *err = get_error (p);
+ return r;
}
static int
-plugin_flush (struct backend *b, struct connection *conn, uint32_t flags)
+plugin_flush (struct backend *b, struct connection *conn, uint32_t flags,
+ int *err)
{
struct backend_plugin *p = container_of (b, struct backend_plugin, backend);
+ int r;
assert (connection_get_handle (conn, 0));
assert (!flags);
debug ("flush");
- if (p->plugin.flush != NULL)
- return p->plugin.flush (connection_get_handle (conn, 0));
+ if (p->plugin.flush != NULL) {
+ r = p->plugin.flush (connection_get_handle (conn, 0));
+ if (r == -1)
+ *err = get_error (p);
+ return r;
+ }
else {
- errno = EINVAL;
+ *err = EINVAL;
return -1;
}
}
static int
plugin_pwrite (struct backend *b, struct connection *conn,
- const void *buf, uint32_t count, uint64_t offset, uint32_t flags)
+ const void *buf, uint32_t count, uint64_t offset, uint32_t flags,
+ int *err)
{
int r;
struct backend_plugin *p = container_of (b, struct backend_plugin, backend);
@@ -408,19 +424,21 @@ plugin_pwrite (struct backend *b, struct connection *conn,
r = p->plugin.pwrite (connection_get_handle (conn, 0),
buf, count, offset);
else {
- errno = EROFS;
+ *err = EROFS;
return -1;
}
if (r != -1 && fua) {
assert (p->plugin.flush);
- r = plugin_flush (b, conn, 0);
+ r = p->plugin.flush (connection_get_handle (conn, 0));
}
+ if (r == -1)
+ *err = get_error (p);
return r;
}
static int
plugin_trim (struct backend *b, struct connection *conn,
- uint32_t count, uint64_t offset, uint32_t flags)
+ uint32_t count, uint64_t offset, uint32_t flags, int *err)
{
int r;
struct backend_plugin *p = container_of (b, struct backend_plugin, backend);
@@ -435,25 +453,26 @@ plugin_trim (struct backend *b, struct connection *conn,
if (p->plugin.trim != NULL)
r = p->plugin.trim (connection_get_handle (conn, 0), count, offset);
else {
- errno = EINVAL;
+ *err = EINVAL;
return -1;
}
if (r != -1 && fua) {
assert (p->plugin.flush);
- r = plugin_flush (b, conn, 0);
+ r = p->plugin.flush (connection_get_handle (conn, 0));
}
+ if (r == -1)
+ *err = get_error (p);
return r;
}
static int
plugin_zero (struct backend *b, struct connection *conn,
- uint32_t count, uint64_t offset, uint32_t flags)
+ uint32_t count, uint64_t offset, uint32_t flags, int *err)
{
struct backend_plugin *p = container_of (b, struct backend_plugin, backend);
char *buf;
uint32_t limit;
int result;
- int err = 0;
int may_trim = (flags & NBDKIT_FLAG_MAY_TRIM) != 0;
bool fua = flags & NBDKIT_FLAG_FUA;
@@ -469,12 +488,9 @@ plugin_zero (struct backend *b, struct connection *conn,
errno = 0;
result = p->plugin.zero (connection_get_handle (conn, 0),
count, offset, may_trim);
- if (result == -1) {
- err = threadlocal_get_error ();
- if (!err && plugin_errno_is_preserved (b))
- err = errno;
- }
- if (result == 0 || err != EOPNOTSUPP)
+ if (result == -1)
+ *err = get_error (p);
+ if (result == 0 || *err != EOPNOTSUPP)
goto done;
}
@@ -483,7 +499,7 @@ plugin_zero (struct backend *b, struct connection *conn,
limit = count < MAX_REQUEST_SIZE ? count : MAX_REQUEST_SIZE;
buf = calloc (limit, 1);
if (!buf) {
- errno = ENOMEM;
+ *err = ENOMEM;
return -1;
}
@@ -497,15 +513,17 @@ plugin_zero (struct backend *b, struct connection *conn,
limit = count;
}
- err = errno;
+ *err = errno;
free (buf);
- errno = err;
+ errno = *err;
done:
if (result != -1 && fua) {
assert (p->plugin.flush);
- result = plugin_flush (b, conn, 0);
+ result = p->plugin.flush (connection_get_handle (conn, 0));
}
+ if (result == -1)
+ *err = get_error (p);
return result;
}
@@ -519,7 +537,6 @@ static struct backend plugin_functions = {
.dump_fields = plugin_dump_fields,
.config = plugin_config,
.config_complete = plugin_config_complete,
- .errno_is_preserved = plugin_errno_is_preserved,
.open = plugin_open,
.prepare = plugin_prepare,
.finalize = plugin_finalize,
--
2.14.3
More information about the Libguestfs
mailing list