Re: [Libguestfs] [PATCH libnbd v2 2/4] generator: Callback returns int instead of void.

On 6/4/19 4:59 AM, Richard W.M. Jones wrote: > Callback functions now return an int instead of a void. This allows > in some cases for the callback to indicate that there was an error. > > This is a small change to the API: Indeed; and my work to let nbdkit-nbd use libnbd is slightly impacted. If I want to support both 0.1.2 and 0.1.x, I now have to do a conditional compilation (either based on a configure check or on hard-coded version information - we don't yet have a LIBNBD_VERSION macro in libnbd.h, but my recent addition LIBNBD_HAVE_NBD_SUPPORTS_URI can serve as a hack witness for 0.1.2 vs. later) where I declare a typedef to the two different function signatures, then call nbd_aio_block_status(..., (type)myfunc) where the cast to (type) is a no-op for 0.1.x and casts the return type of int to void for 0.1.2. On the other hand, changing my nbdkit-nbd patches to use pkg-config to require 0.1.3 or newer is also easy, at which point I don't have to worry about back-compat to the older API, and no one else is going to try to be compatible across all our various earlier pre-stable API. I'll leave it up to you when to actually bump the release version, but it looks like I won't be committing my changes to nbdkit-nbd until 0.1.3 is actually cut.

> > For nbd_set_debug_callback the signature has changed, but the return > value is ignored. > > For nbd_block_status and nbd_aio_block_status the extent function can > return an error, which causes the block status command to return an > error. Unfortunately this causes us to set the state to dead, > although with more effort we could recover from this. Because of this > behaviour I didn't document what happens in the error case as we may > want to change it in future. I can give a shot on top of this patch for recovery (my idea: record the error, then continue to accept additional reply chunks from the server until the final chunk, and merely skip calling the callback when an earlier callback error was recorded).

> > For Python and OCaml bindings, raising any kind of exception from the > callback is equivalent to returning -1 from the C callback. > --- > @@ -3957,24 +3963,34 @@ let print_ocaml_binding (name, { args; ret }) = > > pr " rv = caml_callbackN_exn (fnv, %d, args);\n" > (List.length argnames); > - pr " if (Is_exception_result (rv))\n"; > + pr " if (Is_exception_result (rv)) {\n"; > + pr " /* XXX This is not really an error as callbacks can return\n"; > + pr " * an error indication. But perhaps we should direct this\n"; > + pr " * to a more suitable place or formalize what exception.\n"; Spurious trailing '.' > + pr " * means error versus unexpected failure.\n"; > + pr " */\n"; > +++ b/generator/states-reply-structured.c > @@ -369,10 +369,16 @@ > if (context_id == meta_context->context_id) > break; > > - if (meta_context) > + if (meta_context) { > /* Call the caller's extent function. */ > - cmd->extent_fn (cmd->data, meta_context->name, cmd->offset, > - &h->bs_entries[1], (length-4) / 4); > + if (cmd->extent_fn (cmd->data, meta_context->name, cmd->offset, > + &h->bs_entries[1], (length-4) / 4) == -1) { > + SET_NEXT_STATE (%.DEAD); /* XXX We should be able to recover. */ > + if (errno == 0) errno = EPROTO; > + set_error (errno, "extent function failed"); > + return -1; > + } > + } If nothing else, our testsuite ought to provoke callback failure at least once. > else > /* Emit a debug message, but ignore it. */ > debug (h, "server sent unexpected meta context ID %" PRIu32, > diff --git a/interop/dirty-bitmap.c b/interop/dirty-bitmap.c > index b3a89d0..8d34173 100644 > --- a/interop/dirty-bitmap.c > +++ b/interop/dirty-bitmap.c > @@ -32,7 +32,7 @@ static const char *base_allocation = "base:allocation"; > > static int calls; /* Track which contexts passed through callback */ > > -static void > +static int > cb (void *data, const char *metacontext, uint64_t offset, > uint32_t *entries, size_t len) > { > @@ -71,6 +71,8 @@ cb (void *data, const char *metacontext, uint64_t offset, > fprintf (stderr, "unexpected context %s\n", metacontext); > exit (EXIT_FAILURE); > } > + > + return 0; In fact, having dirty-bitmap be the test to provoke failure, where we are expecting more than one context and can thus also prove that failure on the first context prevents the callback for the second context from being reached, seems like the right place. I'll see what it looks like on top of your patch. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org