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

[Richard W.M. Jones]

On Wed, Jul 24, 2019 at 03:30:51PM -0500, Eric Blake wrote:
> On 7/24/19 11:54 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.
> > 
> 
> > +++ b/lib/aio.c
> > @@ -90,6 +90,17 @@ nbd_unlocked_aio_command_completed (struct nbd_handle *h,
> >    else
> >      h->cmds_done = cmd->next;
> >  
> > +  /* Free the callbacks. */
> > +  if (cmd->type != NBD_CMD_READ && cmd->cb.fn.extent)
> > +    cmd->cb.fn.extent (LIBNBD_CALLBACK_FREE, cmd->cb.fn_user_data,
> > +                       NULL, 0, NULL, 0, NULL);
> > +  if (cmd->type == NBD_CMD_READ && cmd->cb.fn.read)
> > +    cmd->cb.fn.read (LIBNBD_CALLBACK_FREE, cmd->cb.fn_user_data,
> > +                     NULL, 0, 0, 0, NULL);
> > +  if (cmd->cb.callback)
> > +    cmd->cb.callback (LIBNBD_CALLBACK_FREE, cmd->cb.user_data,
> > +                      0, NULL);
> > +
> >    free (cmd);
> 
> nbd_aio_command_completed is skipped when a user calls nbd_close.  While
> we've documented that nbd_close loses the exit status of any unretired
> command (different from the fact that completion callbacks run on
> transition to DEAD but the handle is still around), it is still probably
> worth tweaking nbd_close's use of free_cmd_list() to still call FREE on
> any pending callbacks on commands stranded by abrupt close to allow the
> user to still avoid memory leaks.

Yes absolutely it must do this, and it's a bug in my v2
patch that it doesn't.  I'll try to fix all this up tomorrow.

> > +++ b/lib/debug.c
> > @@ -40,9 +40,11 @@ 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 *),
> > -                                 void *data)
> > +                                 debug_fn debug_fn, void *data)
> >  {
> > +  if (h->debug_fn)
> > +    /* ignore return value */
> > +    h->debug_fn (LIBNBD_CALLBACK_FREE, h->debug_data, NULL, NULL);
> 
> Is it worth outputting a debug message just before freeing things? Or
> put differently, should this be something like
> 
> h->debug_fn (LIBNBD_CALLBACK_VALID | LIBNBD_CALLBACK_FREE,
>              h->debug_data, context, "changing debug callback");

It's basically free so why not.

> Also, should nbd_close() output a debug message about the handle
> disappearing (and if so, obviously prior to calling callback(FREE)).

OK

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