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

[Richard W.M. Jones]

On Tue, Jun 04, 2019 at 06:35:13AM -0500, Eric Blake wrote:
> 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.

TBH I'd just assume latest version, and I'll try to get out 0.1.3 soon
(although not in the next few days).

> > 
> > 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).

This sounds like the right approach, thanks.

I'll push this with the s/\.$// fix you also mentioned below.

Rich.

> > 
> > 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
> 




-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-df lists disk usage of guests without needing to install any
software inside the virtual machine.  Supports Linux and Windows.
http://people.redhat.com/~rjones/virt-df/