[Libguestfs] [nbdkit PATCH v3 02/15] backend: Rework internal error return semantics

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(a)redhat.com&gt; --- 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