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

[Eric Blake]

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


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

>          cmd->error = error ? error : EPROTO;
>      }
> diff --git a/generator/states-reply-structured.c b/generator/states-reply-structured.c
> index f60232e..2ef8d20 100644
> --- a/generator/states-reply-structured.c
> +++ b/generator/states-reply-structured.c
> @@ -298,7 +298,7 @@
>           * current error rather than any earlier one. If the callback fails
>           * without setting errno, then use the server's error below.
>           */
> -        if (cmd->cb.fn.read (cmd->cb.fn_user_data,
> +        if (cmd->cb.fn.read (LIBNBD_CALLBACK_VALID, cmd->cb.fn_user_data,
>                               cmd->data + (offset - cmd->offset),

[2] Here, we know if we will NOT call the read callback again if
NBD_REPLY_FLAG_DONE is set (so if it is set, we could pass VALID|FREE
here; but if it is not set, we still have to take care of passing FREE
later).

> @@ -499,7 +499,7 @@
>        /* Call the caller's extent function. */
>        int error = cmd->error;
>  
> -      if (cmd->cb.fn.extent (cmd->cb.fn_user_data,
> +      if (cmd->cb.fn.extent (LIBNBD_CALLBACK_VALID, cmd->cb.fn_user_data,
>                               meta_context->name, cmd->offset,
>                               &h->bs_entries[1], (length-4) / 4, &error) == -1)

[2] Same - if NBD_REPLY_FLAG_DONE is set, we could pass VALID|FREE here,
but if it is clear, we have to defer FREE to later.  Note that state
REPLY.STRUCTURED_REPLY.FINISH would be such a spot where we could call
the FREE flag. On the other hand, if we let that state always call with
FREE, it's easier to code that than it is to code it to call FREE only
if the earlier code for chunk/extents did not call VALID|FREE. So for
the structured reply spots marked [2] above, calling with just VALID and
then having FINISH do FREE unconditionally is worth considering (the
simple reply can still do VALID|FREE at once).  But if we do that...

>          if (cmd->error == 0)
> diff --git a/generator/states-reply.c b/generator/states-reply.c
> index 1a0c149..b11479c 100644
> --- a/generator/states-reply.c
> +++ b/generator/states-reply.c
> @@ -184,7 +184,8 @@ save_reply_state (struct nbd_handle *h)
>    if (cmd->cb.callback) {
>      int error = cmd->error;
>  
> -    if (cmd->cb.callback (cmd->cb.user_data, cookie, &error) == -1 && error)
> +    if (cmd->cb.callback (LIBNBD_CALLBACK_VALID,
> +                          cmd->cb.user_data, cookie, &error) == -1 && error)

[1] ...we know that this is the only time we'll invoke this callback, so
we could have passed VALID|FREE here.  (And this part definitely means I
have to respin my proposal to let the callback return 1 to auto-retire).

>        cmd->error = error;
>    }
>  
> diff --git a/generator/states.c b/generator/states.c
> index 69aa431..374b8c5 100644
> --- a/generator/states.c
> +++ b/generator/states.c
> @@ -124,7 +124,8 @@ void abort_commands (struct nbd_handle *h,
>        int error = cmd->error ? cmd->error : ENOTCONN;
>  
>        assert (cmd->type != NBD_CMD_DISC);
> -      if (cmd->cb.callback (cmd->cb.user_data, cmd->cookie,
> +      if (cmd->cb.callback (LIBNBD_CALLBACK_VALID,
> +                            cmd->cb.user_data, cmd->cookie,

[1] another spot where we only call things once, and so we could pass
VALID|FREE.


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

[2] ...these FREE calls could instead live in REPLY.STRUCTURED_REPLY.FINISH

> +  if (cmd->cb.callback)
> +    cmd->cb.callback (LIBNBD_CALLBACK_FREE, cmd->cb.user_data,
> +                      0, NULL);

[1] and this spot is useless for my auto-retire patch, hence my idea of
using VALID|FREE at both spots where we utilize the callback, rather
than here where we retire the callback.

I'll have to look at your v2, and my ideas for rearranging the callbacks
could still be done as a patch on top of your initial implementation,
rather than making you have to spin a v3, since our mails have crossed.

> +
>    free (cmd);
>  
>    /* If the command was successful, return true. */
> 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?

-- 
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/00bec58e/attachment.sig>