[Libguestfs] [libnbd PATCH] block-status: Make callback usage consistent with pread_structured
Eric Blake
eblake at redhat.com
Thu Jun 27 14:31:35 UTC 2019
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
More information about the Libguestfs
mailing list