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

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