[Libguestfs] [PATCH libnbd 1/3] generator: Change Closure so it describes single callbacks.

[Richard W.M. Jones]

On Wed, Jul 24, 2019 at 10:02:04AM -0500, Eric Blake wrote:
> On 7/24/19 7:17 AM, Richard W.M. Jones wrote:
> > In preparation for closure lifetimes, split up the Closure so it no
> > longer describes a list of closures, but a single callback.
> > 
> > This changes the API because functions which take 2 or more closures
> > now pass a separate user_data for each one.
> > ---
> >  docs/libnbd.pod                     |   3 +-
> >  examples/strict-structured-reads.c  |   2 +-
> >  generator/generator                 | 760 ++++++++++++----------------
> >  generator/states-reply-simple.c     |   2 +-
> >  generator/states-reply-structured.c |   8 +-
> >  lib/internal.h                      |   3 +-
> >  lib/rw.c                            |  32 +-
> >  7 files changed, 364 insertions(+), 446 deletions(-)
> > 
> > diff --git a/docs/libnbd.pod b/docs/libnbd.pod
> > index 5608e63..631bb3b 100644
> > --- a/docs/libnbd.pod
> > +++ b/docs/libnbd.pod
> > @@ -487,8 +487,7 @@ C<nbd_set_debug_callback>, C<nbd_pread_callback>).  Libnbd can call
> >  these functions while processing.
> >  
> >  Callbacks have an opaque C<void *user_data> pointer.  This is passed
> 
> Should we tweak this to explicitly call out "Callbacks in the C
> language", to differentiate it from bindings to other languages that do
> not do this?

Yes, the documentation is rather C-specific.  I'll add it (but as
another patch on top).

> > -as the first parameter to the callback.  libnbd functions that take
> > -two callback pointers share the same opaque data for both calls.
> > +as the first parameter to the callback.
> >  
> 
> > +++ b/generator/generator
> > @@ -849,10 +849,10 @@ and arg =
> >                                written by the function *)
> >  | BytesPersistIn of string * string (* same as above, but buffer persists *)
> >  | BytesPersistOut of string * string
> > -| Closure of bool * closure list (* void *opaque + one or more closures
> > -                              flag if true means callbacks persist
> > -                              in the handle, false means they only
> > -                              exist during the function call *)
> > +| Closure of bool * closure (* function pointer + void *opaque
> > +                               flag if true means callbacks persist
> > +                               in the handle, false means they only
> > +                               exist during the function call *)
> 
> Do we still need the closure record type, or can/should we simplify
> further to:
> 
> | Closure of bool * string * arg list
>
> (and later in the series, even further by dropping 'bool *')

There's actually no difference at the machine code level - both tuple
and struct have the same representation (same as it would be in C in
fact).  So it comes down to which is nicer, and I think it's slightly
nicer to have the named fields.

> >  | Flags of string          (* NBD_CMD_FLAG_* flags *)
> >  | Int of string            (* small int *)
> >  | Int64 of string          (* 64 bit signed int *)
> > @@ -921,8 +921,8 @@ Return the state of the debug flag on this handle.";
> >    "set_debug_callback", {
> >      default_call with
> >      args = [ Closure (true,
> > -                      [{ cbname="debug_fn";
> > -                         cbargs=[String "context"; String "msg"] }]) ];
> > +                      { cbname="debug_fn";
> > +                        cbargs=[String "context"; String "msg"] }) ];
> 
> Keeping the record type means that you still have to provide names, vs.
> directly using tuples where we'd write:
> 
> args = [ Closure (true, "debug_fn",
>                   [String "context"; String "msg"]) ];

Indeed.

> > @@ -3811,154 +3795,140 @@ let print_python_binding name { args; ret } =
> >     *)
> >    List.iter (
> >      function
> > -    | Closure (persistent, cls) ->
> > -       pr "struct %s_user_data {\n" name;
> > -       List.iter (
> > -         fun { cbname } ->
> > -           pr "  PyObject *%s;\n" cbname
> > -       ) cls;
> > -       pr "};\n";
> > -       pr "\n";
> > -
> > +    | Closure (persistent, { cbname; cbargs }) ->
> >         (* Persistent closures need an explicit function to decrement
> >          * the closure refcounts and free the user_data struct.
> 
> Comment needs a touchup now that there is no user_data struct.

This comment goes away in the following commit :-)

> > +       pr "  py_args = Py_BuildValue (\"(\"";
> > +       List.iter (
> > +         function
> > +         | ArrayAndLen (UInt32 n, len) -> pr " \"O\""
> > +         | BytesIn (n, len) -> pr " \"y#\""
> > +         | Int n -> pr " \"i\""
> > +         | Int64 n -> pr " \"L\""
> > +         | Mutable (Int n) -> pr " \"O\""
> > +         | String n -> pr " \"s\""
> > +         | UInt64 n -> pr " \"K\""
> > +         (* The following not yet implemented for callbacks XXX *)
> > +         | ArrayAndLen _ | Bool _ | BytesOut _
> > +           | BytesPersistIn _ | BytesPersistOut _
> 
> Not your usual indentation style.

I think at some point I gave up fighting tuareg mode :-(

I need to ask Martin for the right settings to make it indent in the
preferred way.

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