[Libguestfs] [nbdkit PATCH 1/2] server: Assert sane error responses

Our filters document that calling through next_ops should reliably set *err on failure, and in turn that the filter must set *err if returning -1. Since plugins.c wrapper ensures that *err is always set regardless of the plugin, the only place where *err could be left unset is in a broken filter. Our documentation also states that a filter must never observe or return EOPNOTSUPP from a failed .zero, since any fallback to .pwrite should have already happened (this restriction will be changed in future patches when the new NBD_CMD_FLAG_FAST_ZERO is added, but for now it is worth making sure we obey). Since filters are always in-tree, it is appropriate to assert that none of our backends are forgetting to set *err correctly. Signed-off-by: Eric Blake <eblake(a)redhat.com&gt; --- server/filters.c | 56 +++++++++++++++++++++++++++++++++++++---------- server/protocol.c | 32 ++++++++++++++++++++------- 2 files changed, 69 insertions(+), 19 deletions(-) diff --git a/server/filters.c b/server/filters.c index 3d9e1efa..14ca0cc6 100644 --- a/server/filters.c +++ b/server/filters.c @@ -347,8 +347,13 @@ next_pread (void *nxdata, void *buf, uint32_t count, uint64_t offset, uint32_t flags, int *err) { struct b_conn *b_conn = nxdata; - return b_conn->b->pread (b_conn->b, b_conn->conn, buf, count, offset, flags, - err); + int r; + + r = b_conn->b->pread (b_conn->b, b_conn->conn, buf, count, offset, flags, + err); + if (r == -1) + assert (*err); + return 1; } static int @@ -356,15 +361,25 @@ next_pwrite (void *nxdata, const void *buf, uint32_t count, uint64_t offset, uint32_t flags, int *err) { struct b_conn *b_conn = nxdata; - return b_conn->b->pwrite (b_conn->b, b_conn->conn, buf, count, offset, flags, - err); + int r; + + r = b_conn->b->pwrite (b_conn->b, b_conn->conn, buf, count, offset, flags, + err); + if (r == -1) + assert (*err); + return r; } static int next_flush (void *nxdata, uint32_t flags, int *err) { struct b_conn *b_conn = nxdata; - return b_conn->b->flush (b_conn->b, b_conn->conn, flags, err); + int r; + + r = b_conn->b->flush (b_conn->b, b_conn->conn, flags, err); + if (r == -1) + assert (*err); + return r; } static int @@ -372,7 +387,12 @@ next_trim (void *nxdata, uint32_t count, uint64_t offset, uint32_t flags, int *err) { struct b_conn *b_conn = nxdata; - return b_conn->b->trim (b_conn->b, b_conn->conn, count, offset, flags, err); + int r; + + r = b_conn->b->trim (b_conn->b, b_conn->conn, count, offset, flags, err); + if (r == -1) + assert (*err); + return r; } static int @@ -380,7 +400,12 @@ next_zero (void *nxdata, uint32_t count, uint64_t offset, uint32_t flags, int *err) { struct b_conn *b_conn = nxdata; - return b_conn->b->zero (b_conn->b, b_conn->conn, count, offset, flags, err); + int r; + + r = b_conn->b->zero (b_conn->b, b_conn->conn, count, offset, flags, err); + if (r == -1) + assert (*err && *err != ENOTSUP && *err != EOPNOTSUPP); + return r; } static int @@ -388,8 +413,13 @@ next_extents (void *nxdata, uint32_t count, uint64_t offset, uint32_t flags, struct nbdkit_extents *extents, int *err) { struct b_conn *b_conn = nxdata; - return b_conn->b->extents (b_conn->b, b_conn->conn, count, offset, flags, - extents, err); + int r; + + r = b_conn->b->extents (b_conn->b, b_conn->conn, count, offset, flags, + extents, err); + if (r == -1) + assert (*err); + return r; } static int @@ -397,8 +427,12 @@ next_cache (void *nxdata, uint32_t count, uint64_t offset, uint32_t flags, int *err) { struct b_conn *b_conn = nxdata; - return b_conn->b->cache (b_conn->b, b_conn->conn, count, offset, flags, - err); + int r; + + r = b_conn->b->cache (b_conn->b, b_conn->conn, count, offset, flags, err); + if (r == -1) + assert (*err); + return r; } static struct nbdkit_next_ops next_ops = { diff --git a/server/protocol.c b/server/protocol.c index 59676224..72f6f2a8 100644 --- a/server/protocol.c +++ b/server/protocol.c @@ -240,27 +240,35 @@ handle_request (struct connection *conn, switch (cmd) { case NBD_CMD_READ: - if (backend->pread (backend, conn, buf, count, offset, 0, &err) == -1) + if (backend->pread (backend, conn, buf, count, offset, 0, &err) == -1) { + assert (err); return err; + } break; case NBD_CMD_WRITE: if (fua) f |= NBDKIT_FLAG_FUA; - if (backend->pwrite (backend, conn, buf, count, offset, f, &err) == -1) + if (backend->pwrite (backend, conn, buf, count, offset, f, &err) == -1) { + assert (err); return err; + } break; case NBD_CMD_FLUSH: - if (backend->flush (backend, conn, 0, &err) == -1) + if (backend->flush (backend, conn, 0, &err) == -1) { + assert (err); return err; + } break; case NBD_CMD_TRIM: if (fua) f |= NBDKIT_FLAG_FUA; - if (backend->trim (backend, conn, count, offset, f, &err) == -1) + if (backend->trim (backend, conn, count, offset, f, &err) == -1) { + assert (err); return err; + } break; case NBD_CMD_CACHE: @@ -271,13 +279,17 @@ handle_request (struct connection *conn, while (count) { limit = MIN (count, sizeof buf); if (backend->pread (backend, conn, buf, limit, offset, flags, - &err) == -1) + &err) == -1) { + assert (err); return err; + } count -= limit; } } - else if (backend->cache (backend, conn, count, offset, 0, &err) == -1) + else if (backend->cache (backend, conn, count, offset, 0, &err) == -1) { + assert (err); return err; + } break; case NBD_CMD_WRITE_ZEROES: @@ -285,8 +297,10 @@ handle_request (struct connection *conn, f |= NBDKIT_FLAG_MAY_TRIM; if (fua) f |= NBDKIT_FLAG_FUA; - if (backend->zero (backend, conn, count, offset, f, &err) == -1) + if (backend->zero (backend, conn, count, offset, f, &err) == -1) { + assert (err && err != ENOTSUP && err != EOPNOTSUPP); return err; + } break; case NBD_CMD_BLOCK_STATUS: @@ -299,8 +313,10 @@ handle_request (struct connection *conn, if (flags & NBD_CMD_FLAG_REQ_ONE) f |= NBDKIT_FLAG_REQ_ONE; if (backend->extents (backend, conn, count, offset, f, - extents, &err) == -1) + extents, &err) == -1) { + assert (err); return err; + } } else { int r; -- 2.20.1