[Libguestfs] [PATCH libnbd proposal] api: Add semi-private function for freeing persistent data.

[Richard W.M. Jones]

On Mon, Aug 12, 2019 at 09:08:37AM -0500, Eric Blake wrote:
> On 8/11/19 8:03 AM, Richard W.M. Jones wrote:
> > This adds a C-only semi-private function for freeing various types of
> > persistent data passed to libnbd.
> > 
> > There are some similarities with nbd_add_close_callback which we
> > removed in commit 7f191b150b52ed50098976309a6af883d245fc56.
> > ---
> >  generator/generator | 46 +++++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 46 insertions(+)
> > 
> > diff --git a/generator/generator b/generator/generator
> > index 55c4dfc..0ea6b61 100755
> > --- a/generator/generator
> > +++ b/generator/generator
> > @@ -3284,6 +3284,7 @@ let generate_lib_libnbd_syms () =
> >        pr "LIBNBD_%d.%d {\n" major minor;
> >        pr "  global:\n";
> >        if (major, minor) = (1, 0) then (
> > +        pr "    nbd_add_free_callback;\n";
> >          pr "    nbd_create;\n";
> >          pr "    nbd_close;\n";
> >          pr "    nbd_get_errno;\n";
> > @@ -3581,6 +3582,12 @@ let generate_include_libnbd_h () =
> >    pr "extern int nbd_get_errno (void);\n";
> >    pr "#define LIBNBD_HAVE_NBD_GET_ERRNO 1\n";
> >    pr "\n";
> > +  pr "typedef void (*nbd_free_callback) (void *ptr);\n";
> > +  pr "extern int nbd_add_free_callback (struct nbd_handle *h,\n";
> > +  pr "                                  nbd_free_callback cb,\n";
> > +  pr "                                  void *ptr);\n";
> > +  pr "#define LIBNBD_HAVE_NBD_ADD_FREE_CALLBACK 1\n";
> > +  pr "\n";
> 
> Would this change the signature of callbacks? With this in place, you
> no longer need the VALID|FREE parameter, but can defer the free action
> to an added free callback.

Correct - the full patch series which I've now posted has a final step
where we can remove the valid_flag completely.

> How would you actually track when to call the free callback? I guess
> each time a pointer lifetime ends (basically, when retiring a completed
> command) we check each pointer grabbed by that command and compare it
> against the list of registered pointers for any free callbacks to use?
> How does that scale, if the user can register an arbitrary number of
> pointers because they queue up a large number of requests before
> starting the poll loop?

My implementation scales as O(log n).  However nbd_add_free_callback
is O(n) because we maintain the list of pointers in address order.
nbd_add_free_callback is only really used in a few places by language
bindings.

> Is the existing idea of a VALID|FREE parameter not sufficient?  If we
> can convince the C bindings for python nbd.aio_pread to increase the
> refcount of buf, and then install a C completion handler (whether the
> python user called nbd.aio_pread with no python completer, or whether
> they called nbd.aio_pread_complete) that does the decref when called
> with FREE, then that would do the same job, even without the need for
> nbd_add_free_callback, wouldn't it?  And there's no issue of scanning
> through a list of pointers with a free callback associated with them
> (storing registered pointers with some sort of hash may eventually get
> to amortized O(1) lookup, but that seems like a lot of overhead compared
> to the fact that we already have O(1) access to when to call the
> completion callback with the FREE flag).

It's true but then nbd.aio_pread would have to call the _callback
variant in all cases.

In any case I've posted the implementation of nbd_add_free_callback
for discussion, so we can take a look at it and see if it's better or
worse.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-p2v converts physical machines to virtual machines.  Boot with a
live CD or over the network (PXE) and turn machines into KVM guests.
http://libguestfs.org/virt-v2v