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

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.

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