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

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

On 7/22/19 6:50 AM, Richard W.M. Jones wrote:
> 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);
Would the 'valid_flag' be presented to non-C bindings, or is it only
needed for C code? At any rate, the idea makes sense as a lighter-weight
way for libnbd to always inform the callback about the last invocation.
For nbd_aio_pread_structured and nbd_aio_block_status, the chunk/extent
callback can either match server state (if the server replies replies
with NBD_REPLY_FLAG_DONE on an OFFSET_DATA/BLOCK_STATUS reply, we could
set VALID|FREE; if the server defers NBD_REPLY_FLAG_DONE to an
NBD_REPLY_TYPE_NONE packet then the two flags will definitely not be set
at the same time), or we could always defer the FREE flag until the
overall command is ready to retire; for nbd_aio_FOO_callback, it may
indeed be easier to set VALID|FREE on the single use of the callback at
the time it is ready to retire.  But this idea does make it possible for
libnbd to inform the callback about its last expected use.
> 
> 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);
>     }
>   }
I also like the idea of letting the callback handle both flags in a
single call where sensible, for fewer indirect function calls.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

On Mon, Jul 22, 2019 at 08:13:12AM -0500, Eric Blake
wrote:
> On 7/22/19 6:50 AM, Richard W.M. Jones wrote:
> > 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);
> 
> Would the 'valid_flag' be presented to non-C bindings, or is it
only
> needed for C code?
C only.  It's not needed for languages which have garbage collection.
For non-C languages without GC, well I guess we can decide on a
case-by-case basis.

> At any rate, the idea makes sense as a lighter-weight
> way for libnbd to always inform the callback about the last invocation.
I've almost got a working implementation.  Will post it for review as
soon as I can.

[..]

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-top is 'top' for virtual machines.  Tiny program with many
powerful monitoring features, net stats, disk stats, logging, etc.
http://people.redhat.com/~rjones/virt-top

This has an annoying subtlety around the fact that we can pass a
single user_data and multiple closures in one function call.

The LIBNBD_CALLBACK_FREE function would be called several times with
the same user_data in this case, which means the callback must do some
kind of reference counting before the user_data can be freed.

I propose that we split up Closure so it describes a single closure,
although that does mean that separate user_data must be passed with
each function pointer (which may not be a bad thing).

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