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

[Eric Blake]

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.

> +++ 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");

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

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libguestfs/attachments/20190724/155bcf18/attachment.sig>