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

Eric Blake
2019-07-24T15:02:04Z

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