[Libguestfs] [libnbd PATCH 1/2] generator: Refactor handling of closures in unlocked functions

[Richard W.M. Jones]

On Mon, Sep 07, 2020 at 04:46:39PM -0500, Eric Blake wrote:
> We have a memory leak when a function with a closure exits early prior
> to registering that closure through some path that will guarantee
> cleanup.  The easiest way to fix it is to guarantee that once a
> closure is passed into a public API, it will be cleaned regardless of
> whether that API succeeds or fails.  But to avoid cleaning the closure
> more than once, we need to refactor our internal handling, in order to
> track when a closure has been handed off for later cleaning.  The
> easiest way to do this is to pass closures by reference to all
> internal functions, so that helpers in the call stack can modify the
> incoming pointer rather than their own copy.
> 
> This patch is just preparatory, with no semantic change.  The public
> API still passes closures by copy, but the generator then operates on
> the address of that closure through all internal nbd_unlocked_*
> functions, rather than making further copies through each additional
> function call.
> ---
>  generator/C.ml  | 35 ++++++++++++-----------
>  generator/C.mli |  1 +
>  lib/debug.c     |  6 ++--
>  lib/opt.c       | 26 ++++++++---------
>  lib/rw.c        | 76 ++++++++++++++++++++++++-------------------------
>  5 files changed, 73 insertions(+), 71 deletions(-)
> 
> diff --git a/generator/C.ml b/generator/C.ml
> index deb77fa..280b319 100644
> --- a/generator/C.ml
> +++ b/generator/C.ml
> @@ -99,16 +99,17 @@ let rec name_of_arg = function
>  | UInt64 n -> [n]
> 
>  let rec print_arg_list ?(wrap = false) ?maxcol ?handle ?types ?(parens = true)
> +          ?(closure_mark) args optargs =

You don't need parens around here, you can just write ?closure_mark

> +  if parens then pr "(";
> +  if wrap then
> +    pr_wrap ?maxcol ','
> +      (fun () -> print_arg_list' ?handle ?types ?closure_mark args optargs)
> +  else
> +    print_arg_list' ?handle ?types ?closure_mark args optargs;
> +  if parens then pr ")"
> +
> +and print_arg_list' ?(handle = false) ?(types = true) ?(closure_mark = "")
...
>            args optargs =
>        | Closure { cbname; cbargs } ->
>           if types then pr "nbd_%s_callback " cbname;
> -         pr "%s_callback" cbname
> +         pr "%s%s_callback" closure_mark cbname

Perhaps make it type safe?

  type closure_mark = AddressOf | Dereference

And then:

  pr "%s%c_callback"
     (match closure_mark with AddressOf -> '&' | Dereference -> '*')
     cbname

...
> @@ -385,7 +386,7 @@ let generate_lib_unlocked_h () =
>    pr "\n";
>    List.iter (
>      fun (name, { args; optargs; ret }) ->
> -      print_extern ~wrap:true ("unlocked_" ^ name) args optargs ret
> +      print_extern ~wrap:true ~closure_mark:"*" ("unlocked_" ^ name) args optargs ret

And at call sites like this one you'd use ~closure_mark:Dereference

Rest is fine.

ACK but definitely remove the superfluous parentheses above.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-p2v converts physical machines to virtual machines.  Boot with a
live CD or over the network (PXE) and turn machines into KVM guests.
http://libguestfs.org/virt-v2v