[Libguestfs] [PATCH libnbd 2/3] lib: Implement closure lifetimes.
Richard W.M. Jones
rjones at redhat.com
Wed Jul 24 17:48:54 UTC 2019
On Wed, Jul 24, 2019 at 12:21:37PM -0500, Eric Blake wrote:
> 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...
This is fixed in v2, but ...
> > +++ 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.
That was actually my first version of the patch. However it runs into
the problem that we need to call the FREE version when a command is
aborted. It's complicated to know if we called cb.fn.read already.
But thinking about this a bit more, maybe we should call VALID|FREE
here and set cmd->cb.fn.read = NULL afterwards.
[...]
> > 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?
Yes nbd_close should check and call the callback again with FREE.
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
More information about the Libguestfs
mailing list