[Libguestfs] [PATCH libnbd 2/3] lib: Implement closure lifetimes.

[Richard W.M. Jones]

On Wed, Jul 24, 2019 at 12:21:37PM -0500, Eric Blake wrote:
> On 7/24/19 7:17 AM, Richard W.M. Jones wrote:
> > Previously closures had a crude flag which tells if they are
> > persistent or transient.  Transient closures (flag = false) last for
> > the lifetime of the currently called libnbd function.  Persistent
> > closures had an indefinite lifetime which could last for as long as
> > the handle.  In language bindings handling persistent closures was
> > wasteful as we needed to register a "close callback" to free the
> > closure when the handle is closed.  But if you had submitted thousands
> > of asynchronous commands you would end up registering thousands of
> > close callbacks.
> 
> Our mails are crossing; I had typed this up (so I'm sending now) even
> though I already see that you've tweaked things in v2.  I've got two
> threads of thoughts below.
> 
> > 
> > (Note it is also possible for the library to call the callback with
> > valid_flag == LIBNBD_CALLBACK_VALID|LIBNBD_CALLBACK_FREE, meaning it's the
> > last valid call.)
> 
> [1] In my previous reply, I assumed that the nbd_aio_FOO_callback
> function would use this paradigm...
> 
> 
> > +++ b/examples/strict-structured-reads.c
> 
> > @@ -127,11 +131,14 @@ read_chunk (void *opaque, const void *bufv, size_t count, uint64_t offset,
> >  }
> >  
> >  static int
> > -read_verify (void *opaque, int64_t cookie, int *error)
> > +read_verify (int valid_flag, void *opaque, int64_t cookie, int *error)
> >  {
> >    struct data *data = opaque;
> >    int ret = -1;
> >  
> > +  if (!(valid_flag & LIBNBD_CALLBACK_VALID))
> > +    return 0;
> > +
> 
> [1] which in turn affected this code (calling free(data) on the first
> VALID call rather than on the FREE call is fishy), but now that I've
> actually read the rest of the patch...

This is fixed in v2, but ...

> > +++ b/generator/states-reply-simple.c
> > @@ -64,7 +64,8 @@
> >        int error = 0;
> >  
> >        assert (cmd->error == 0);
> > -      if (cmd->cb.fn.read (cmd->cb.fn_user_data, cmd->data, cmd->count,
> > +      if (cmd->cb.fn.read (LIBNBD_CALLBACK_VALID, cmd->cb.fn_user_data,
> > +                           cmd->data, cmd->count,
> >                             cmd->offset, LIBNBD_READ_DATA, &error) == -1)
> 
> [2] Here, we know we are calling the read callback exactly once, so we
> could pass DATA|FREE here.

That was actually my first version of the patch.  However it runs into
the problem that we need to call the FREE version when a command is
aborted.  It's complicated to know if we called cb.fn.read already.
But thinking about this a bit more, maybe we should call VALID|FREE
here and set cmd->cb.fn.read = NULL afterwards.

[...]

> > diff --git a/lib/debug.c b/lib/debug.c
> > index 12c10f7..56a9455 100644
> > --- a/lib/debug.c
> > +++ b/lib/debug.c
> > @@ -40,9 +40,12 @@ nbd_unlocked_get_debug (struct nbd_handle *h)
> >  
> >  int
> >  nbd_unlocked_set_debug_callback (struct nbd_handle *h,
> > -                                 int (*debug_fn) (void *, const char *, const char *),
> > +                                 int (*debug_fn) (int, void *, const char *, const char *),
> >                                   void *data)
> >  {
> > +  if (h->debug_fn)
> > +    /* ignore return value */
> > +    h->debug_fn (LIBNBD_CALLBACK_FREE, h->debug_data, NULL, NULL);
> 
> This frees the first debug callback when installing a second; but where
> does nbd_close free the second?  Also, do we allow C code to pass NULL
> to uninstall a handler? Should other languages be able to clear out a
> callback?

Yes nbd_close should check and call the callback again with FREE.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
Fedora Windows cross-compiler. Compile Windows programs, test, and
build Windows installers. Over 100 libraries supported.
http://fedoraproject.org/wiki/MinGW