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

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?
> -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 *')
>  | 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"]) ];

> @@ -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.
>          *)
>         if persistent then (
>           pr "static void\n";
> -         pr "free_%s_user_data (void *vp)\n" name;
> +         pr "free_%s_%s_user_data (void *vp)\n" name cbname;
>           pr "{\n";
> -         pr "  struct %s_user_data *user_data = vp;\n" name;
> +         pr "  PyObject *user_data = vp;\n";
>           pr "\n";
> -         List.iter (
> -           fun { cbname } ->
> -             pr "  Py_DECREF (user_data->%s);\n" cbname
> -         ) cls;
> -         pr "  free (user_data);\n";
> +         pr "  Py_DECREF (user_data);\n";
>           pr "}\n";
>           pr "\n";
>         );
... (lots of churn due to reindentation, such is life)
> +       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.
> +           | Closure _
> +           | Flags _ | Mutable _
> +           | Path _ | SockAddrAndLen _ | StringList _
> +           | UInt _ | UInt32 _ -> assert false
> +         ) cbargs;
> +       pr " \")\"";
> +       List.iter (
> +         function
> +         | ArrayAndLen (UInt32 n, _) -> pr ", py_%s" n
> +         | BytesIn (n, len) -> pr ", %s, (int) %s" n len
> +         | Mutable (Int n) -> pr ", py_%s" n
> +         | Int n | Int64 n
> +           | String n
> +           | UInt64 n -> pr ", %s" n
> +         (* The following not yet implemented for callbacks XXX *)
> +         | ArrayAndLen _ | Bool _ | BytesOut _
> +           | BytesPersistIn _ | BytesPersistOut _
> +           | Closure _
> +           | Flags _ | Mutable _
> +           | Path _ | SockAddrAndLen _ | StringList _
> +           | UInt _ | UInt32 _ -> assert false
More instances of what appears to be emacs indenting differently from
your preferences.

> @@ -4031,19 +3998,6 @@ let print_python_binding name { args; ret } >   
) args;
>    pr "\n";
>  
> -  (* Allocate the persistent closure user_data. *)
> -  List.iter (
> -    function
> -    | Closure (false, _) -> ()
> -    | Closure (true, cls) ->
> -       pr "  user_data = malloc (sizeof *user_data);\n";
> -       pr "  if (user_data == NULL) {\n";
> -       pr "    PyErr_NoMemory ();\n";
> -       pr "    return NULL;\n";
> -       pr "  }\n"
> -    | _ -> ()
> -  ) args;
Nice that we get rid of this.

Overall, the patch makes sense; I assume you can fix any problems I
pointed out above without needing to send v2 (especially since this is
mostly OCaml, where I defer to your experience anyway)

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

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

On Wed, Jul 24, 2019 at 05:17:51PM +0100, Richard W.M. Jones
wrote:
>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:
>> > +       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.
>
So I only found this:

tuareg-electric-indent is a variable defined in ‘tuareg.el’.
Its value is nil

Documentation:
Whether to automatically indent the line after typing one of
the words in ‘tuareg-electric-indent-keywords’.  Lines starting
with ‘|’, ‘)’, ‘]‘, and ‘}’ are always indented when the
‘electric-indent-mode’ is turned on.

I did not change any tuareg settings from the default, neither can I find any in
the spacemacs ocaml layer, and I'm not using ocp-indent for the indentation
when
typing (it is way different than the style used here).

So I tried whether it's related to the electric-indent-mode, but no matter
whether I disable or enable it, it does not change the alignment.

So I created new $HOME, ran `emacs -Q`, added melpa to the repository list,
installed tuareg and nothing else.  And it still Just Works™.  Maybe updating
your tuareg.el would do the trick? O:-)