Now that we have a way to pass Mutable(Int "error") to callbacks, that's a much nicer way for language bindings to use than relying on the value of errno after returning -1. Update the semantics for block status to match what pread_structured does, getting errno out of the equation, and changing things to call the callback for a second context even after an earlier callback failure sets the error. This is an API/ABI break to existing clients, but that's okay because we have not yet declared stable API. (If you need to compile against libnbd 0.4 and 0.5 simultaneously, you can use #ifdef LIBNBD_HAVE_NBD_PREAD_STRUCTURED as a witness for the new signature rather than the old). --- This applies on top of my changes to let pread_structured use Mutable. I'll go ahead and push it, so we can cut a release soon. generator/generator | 17 +++++---- generator/states-reply-structured.c | 56 ++++++++++++++--------------- interop/dirty-bitmap.c | 15 ++++---- lib/internal.h | 2 +- ocaml/examples/extents.ml | 3 +- python/t/460-block-status.py | 3 +- tests/meta-base-allocation.c | 9 ++--- 7 files changed, 54 insertions(+), 51 deletions(-) diff --git a/generator/generator b/generator/generator index ec09836..c29460c 100755 --- a/generator/generator +++ b/generator/generator @@ -1527,7 +1527,8 @@ punching a hole."; Callback ("extent", [Opaque "data"; String "metacontext"; UInt64 "offset"; ArrayAndLen (UInt32 "entries", - "nr_entries")]); + "nr_entries"); + Mutable (Int "error")]); Flags "flags" ]; ret = RErr; permitted_states = [ Connected ]; @@ -1548,10 +1549,11 @@ supported by the server (see C<nbd_can_meta_context>) this call returns information about extents by calling back to the C<extent> function. The callback cannot call C<nbd_*> APIs on the same handle since it holds the handle lock and will -cause a deadlock. If the callback returns C<-1>, any remaining -contexts will be ignored and the overall block status command -will fail with the same value of C<errno> left after the -failing callback. +cause a deadlock. If the callback returns C<-1>, and no earlier +error has been detected, then the overall block status command +will fail with any non-zero value stored into the callback's +C<error> parameter (with a default of C<EPROTO>); but any further +contexts will still invoke the callback. The C<extent> function is called once per type of metadata available, with the C<data> passed to this function. The C<metacontext> @@ -1564,6 +1566,8 @@ NBD protocol document in the section about C<NBD_REPLY_TYPE_BLOCK_STATUS> describes the meaning of this array; for contexts known to libnbd, B<E<lt>libnbd.hE<gt>> contains constants beginning with C<LIBNBD_STATE_> that may help decipher the values. +On entry to the callback, the C<error> parameter contains the errno +value of any previously detected error. It is possible for the extent function to be called more times than you expect (if the server is buggy), @@ -1812,7 +1816,8 @@ in C<nbd_zero>."; CallbackPersist ("extent", [Opaque "data"; String "metacontext"; UInt64 "offset"; ArrayAndLen (UInt32 "entries", - "nr_entries")]); + "nr_entries"); + Mutable (Int "error")]); Flags "flags" ]; ret = RInt64; permitted_states = [ Connected ]; diff --git a/generator/states-reply-structured.c b/generator/states-reply-structured.c index fa11dd6..91c6215 100644 --- a/generator/states-reply-structured.c +++ b/generator/states-reply-structured.c @@ -478,37 +478,33 @@ assert (h->bs_entries); assert (length >= 12); - if (cmd->error) + /* Need to byte-swap the entries returned, but apart from that we + * don't validate them. + */ + for (i = 0; i < length/4; ++i) + h->bs_entries[i] = be32toh (h->bs_entries[i]); + + /* Look up the context ID. */ + context_id = h->bs_entries[0]; + for (meta_context = h->meta_contexts; + meta_context; + meta_context = meta_context->next) + if (context_id == meta_context->context_id) + break; + + if (meta_context) { + /* Call the caller's extent function. */ + int error = cmd->error; + + if (cmd->cb.fn.extent (cmd->cb.opaque, meta_context->name, cmd->offset, + &h->bs_entries[1], (length-4) / 4, &error) == -1) + if (cmd->error == 0) + cmd->error = error ? error : EPROTO; + } + else /* Emit a debug message, but ignore it. */ - debug (h, "ignoring meta context ID %" PRIu32 " after callback failure", - be32toh (h->bs_entries[0])); - else { - /* Need to byte-swap the entries returned, but apart from that we - * don't validate them. - */ - for (i = 0; i < length/4; ++i) - h->bs_entries[i] = be32toh (h->bs_entries[i]); - - /* Look up the context ID. */ - context_id = h->bs_entries[0]; - for (meta_context = h->meta_contexts; - meta_context; - meta_context = meta_context->next) - if (context_id == meta_context->context_id) - break; - - if (meta_context) { - /* Call the caller's extent function. */ - errno = 0; - if (cmd->cb.fn.extent (cmd->cb.opaque, meta_context->name, cmd->offset, - &h->bs_entries[1], (length-4) / 4) == -1) - cmd->error = errno ? errno : EPROTO; - } - else - /* Emit a debug message, but ignore it. */ - debug (h, "server sent unexpected meta context ID %" PRIu32, - context_id); - } + debug (h, "server sent unexpected meta context ID %" PRIu32, + context_id); SET_NEXT_STATE(%FINISH); } diff --git a/interop/dirty-bitmap.c b/interop/dirty-bitmap.c index 329fbea..a19fb64 100644 --- a/interop/dirty-bitmap.c +++ b/interop/dirty-bitmap.c @@ -41,7 +41,7 @@ struct data { static int cb (void *opaque, const char *metacontext, uint64_t offset, - uint32_t *entries, size_t len) + uint32_t *entries, size_t len, int *error) { struct data *data = opaque; @@ -50,6 +50,7 @@ cb (void *opaque, const char *metacontext, uint64_t offset, * the fact that qemu-nbd is compliant. */ assert (offset == 0); + assert (!*error || (data->fail && data->count == 1 && *error == EPROTO)); assert (data->count-- > 0); /* [qemu-nbd] */ if (strcmp (metacontext, LIBNBD_CONTEXT_BASE_ALLOCATION) == 0) { @@ -91,7 +92,8 @@ cb (void *opaque, const char *metacontext, uint64_t offset, } if (data->fail) { - errno = EPROTO; /* Something NBD servers can't send */ + /* Something NBD servers can't send */ + *error = data->count == 1 ? EPROTO : ECONNREFUSED; return -1; } return 0; @@ -147,17 +149,14 @@ main (int argc, char *argv[]) } assert (data.seen_base && data.seen_dirty); - /* Trigger a failed callback, to prove connection stays up. No way - * to know which context will respond first, but we can check that - * the other one did not get visited. - */ - data = (struct data) { .count = 1, .fail = true, }; + /* Trigger a failed callback, to prove connection stays up. */ + data = (struct data) { .count = 2, .fail = true, }; if (nbd_block_status (nbd, exportsize, 0, &data, cb, 0) != -1) { fprintf (stderr, "unexpected block status success\n"); exit (EXIT_FAILURE); } assert (nbd_get_errno () == EPROTO && nbd_aio_is_ready (nbd)); - assert (data.seen_base ^ data.seen_dirty); + assert (data.seen_base && data.seen_dirty); if (nbd_pread (nbd, &c, 1, 0, 0) == -1) { fprintf (stderr, "%s\n", nbd_get_error ()); diff --git a/lib/internal.h b/lib/internal.h index f827957..88ad703 100644 --- a/lib/internal.h +++ b/lib/internal.h @@ -232,7 +232,7 @@ struct socket { }; typedef int (*extent_fn) (void *data, const char *metacontext, uint64_t offset, - uint32_t *entries, size_t nr_entries); + uint32_t *entries, size_t nr_entries, int *error); typedef int (*read_fn) (void *data, const void *buf, size_t count, uint64_t offset, int *error, int status); diff --git a/ocaml/examples/extents.ml b/ocaml/examples/extents.ml index b49f98e..6681446 100644 --- a/ocaml/examples/extents.ml +++ b/ocaml/examples/extents.ml @@ -16,7 +16,8 @@ let () = (* Read the extents and print them. *) let size = NBD.get_size nbd in NBD.block_status nbd size 0_L () ( - fun () meta _ entries -> + fun () meta _ entries err -> + printf "err=%d\n" !err; if meta = "base:allocation" then ( printf "index\tlength\tflags\n"; for i = 0 to Array.length entries / 2 - 1 do diff --git a/python/t/460-block-status.py b/python/t/460-block-status.py index eeaa7b2..7e4ba2c 100644 --- a/python/t/460-block-status.py +++ b/python/t/460-block-status.py @@ -27,9 +27,10 @@ h.connect_command (["nbdkit", "-s", "--exit-with-parent", "-v", "sh", script]) entries = [] -def f (data, metacontext, offset, e): +def f (data, metacontext, offset, e, err): global entries assert data == 42 + assert err.value == 0 if metacontext != "base:allocation": return entries = e diff --git a/tests/meta-base-allocation.c b/tests/meta-base-allocation.c index a9ddbd0..8ae215e 100644 --- a/tests/meta-base-allocation.c +++ b/tests/meta-base-allocation.c @@ -30,7 +30,7 @@ static int check_extent (void *data, const char *metacontext, uint64_t offset, - uint32_t *entries, size_t nr_entries); + uint32_t *entries, size_t nr_entries, int *error); int main (int argc, char *argv[]) @@ -128,15 +128,16 @@ main (int argc, char *argv[]) static int check_extent (void *data, const char *metacontext, uint64_t offset, - uint32_t *entries, size_t nr_entries) + uint32_t *entries, size_t nr_entries, int *error) { size_t i; int id = * (int *)data; printf ("extent: id=%d, metacontext=%s, offset=%" PRIu64 ", " - "nr_entries=%zu\n", - id, metacontext, offset, nr_entries); + "nr_entries=%zu, error=%d\n", + id, metacontext, offset, nr_entries, *error); + assert (*error == 0); if (strcmp (metacontext, LIBNBD_CONTEXT_BASE_ALLOCATION) == 0) { for (i = 0; i < nr_entries; i += 2) { printf ("\t%zu\tlength=%" PRIu32 ", status=%" PRIu32 "\n", -- 2.20.1