Libguestfs - Jul 2019 - Re: [libnbd] More thoughts on callbacks and more

More thoughts on callbacks, etc. following on from:
https://www.redhat.com/archives/libguestfs/2019-July/thread.html#00184

Closure lifetimes
-----------------

Closures could have a lifetime if we had a little bit of support from
the C library.  We would generate (from C only):

  nbd_set_free_<fn>_<closure> (nbd, free_closure);

which calls free_closure (user_data) as soon as the closure will no
longer be called by the library.  This function would be used to
decrement the refcount from Python or remove the global root from
OCaml.

Note this is a family of functions, eg:

  nbd_set_free_set_debug_callback_debug_fn

corresponding to the debug_fn arg of nbd_set_debug_callback.  Luckily
they can all be generated along with the internal machinery to call
them.

Buffer lifetimes
----------------

Similar to the above, persistent buffers (BytesPersist*) can
have lifetimes.

Remove nbd_add_close_callback
-----------------------------

The above changes (actually, just the closure change) lets us remove
nbd_add_close_callback.

Fixing callback / cookie problem
--------------------------------

I think an easier way to fix this would be to simply detect the
problematic situation and queue up the callback to be call the next
time the library is entered.  Detecting the situation is fairly easy -
we know the current cookie number, so can check it before we call the
completion function.  And calling the deferred callback(s) is easy
from the generated code.

The only questions in my mind are: (1) If this could cause a deadlock
because the caller is waiting for the completion signal before
reentering the library.  (2) If there's some implicit guarantee that
we should call the callback as easy as possible after the command
finishes.

I think (1) is not possible in ordinary code since callers must always
enter the main loop after calling one of the nbd_aio_* functions.

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

On Sat, Jul 20, 2019 at 07:38:45AM +0100, Richard W.M. Jones
wrote:
> More thoughts on callbacks, etc. following on from:
> https://www.redhat.com/archives/libguestfs/2019-July/thread.html#00184
> 
> Closure lifetimes
> -----------------
> 
> Closures could have a lifetime if we had a little bit of support from
> the C library.  We would generate (from C only):
> 
>   nbd_set_free_<fn>_<closure> (nbd, free_closure);
> 
> which calls free_closure (user_data) as soon as the closure will no
> longer be called by the library.  This function would be used to
> decrement the refcount from Python or remove the global root from
> OCaml.
> 
> Note this is a family of functions, eg:
> 
>   nbd_set_free_set_debug_callback_debug_fn
> 
> corresponding to the debug_fn arg of nbd_set_debug_callback.  Luckily
> they can all be generated along with the internal machinery to call
> them.
As written above this doesn't quite work.  However it could work to
pass an optional free function with the closure.  In other words it
would look like:

  struct nbd_closure { .cl = my_debug_fn,
                       .user_data = foo, .free = my_free } cl;
  nbd_set_debug_callback (nbd, &cl);

cl->free (cl->user_data) is called if cl->free != NULL when the
closure is no longer used by the library.

This is a bit of a change to the API however.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-df lists disk usage of guests without needing to install any
software inside the virtual machine.  Supports Linux and Windows.
http://people.redhat.com/~rjones/virt-df/

On Mon, Jul 22, 2019 at 10:08:25AM +0100, Richard W.M. Jones
wrote:
> On Sat, Jul 20, 2019 at 07:38:45AM +0100, Richard W.M. Jones wrote:
> > More thoughts on callbacks, etc. following on from:
> > https://www.redhat.com/archives/libguestfs/2019-July/thread.html#00184
> > 
> > Closure lifetimes
> > -----------------
Here's a possibly better idea which still changes the API a bit but
not as invasively.

We overload the callback so that it can either be a callback function
or a "free function".  They are distinguished by an extra flag
argument passed to the callback:

  extern int nbd_set_debug_callback (
        struct nbd_handle *h,
        int (*debug_fn) (int valid_flag,   // <-- note
                         void *user_data,
	                 const char *context, const char *msg),
        void *user_data);

The extra ‘valid_flag’ contains one or both of LIBNBD_CALLBACK_VALID
and LIBNBD_CALLBACK_FREE.  Callback code would look like:

  int my_debug_fn (int valid_flag, void *user_data,
                   const char *context, const char *msg)
  {
    if (valid_flag & LIBNBD_CALLBACK_VALID) {
      // This is the normal callback as before.
      printf ("debug msg = %s\n", msg);
    }

    if (valid_flag & LIBNBD_CALLBACK_FREE) {
      // In this case only user_data is valid.  The other parameters
      // may be unspecified (unless VALID was also set).
      free (user_data);
    }
  }

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-builder quickly builds VMs from scratch
http://libguestfs.org/virt-builder.1.html