[Libguestfs] [PATCH libnbd 3/7] ocaml: Remove NBD.Buffer.free function, use a free callback instead.

[Eric Blake]

On 8/12/19 11:08 AM, Richard W.M. Jones wrote:
> By using the free callback mechanism we don't need to manually free
> the buffer.
> 
> This reverts part of commit fef1c281a65d061127bf178e5f8cfca0a2475c56.
> ---
>  generator/generator              | 43 +++++++++++++++++++++-----------
>  ocaml/buffer.c                   | 22 ----------------
>  ocaml/examples/asynch_copy.ml    |  4 +--
>  ocaml/libnbd-ocaml.pod           | 22 ----------------
>  ocaml/nbd-c.h                    |  8 ++++++
>  ocaml/tests/test_590_aio_copy.ml |  4 +--
>  6 files changed, 39 insertions(+), 64 deletions(-)
> 
> diff --git a/generator/generator b/generator/generator
> index d6b9352..92ce170 100755
> --- a/generator/generator
> +++ b/generator/generator
> @@ -4961,10 +4961,6 @@ module Buffer : sig
>    (** Allocate an uninitialized buffer.  The parameter is the size
>        in bytes. *)
>  
> -  val free : t -> unit
> -  (** The buffer must be manually freed when it is no longer used.
> -      An AIO completion callback is usually a good place. *)
> -

Getting rid of this public function is nice.  Hopefully, how we get rid
of it is internal enough that we can switch between either a list of
free functions at the C level, or else a smarter forwarding of aio_pread
to the C nbd_aio_pread_completion, without affecting what else leaks
through to the OCaml view.


> @@ -5244,10 +5239,8 @@ let print_ocaml_binding (name, { args; optargs; ret }) =
>         pr "  caml_enter_blocking_section ();\n";
>         pr "  }\n";
>         pr "\n";
> -       pr "  if (valid_flag & LIBNBD_CALLBACK_FREE) {\n";
> -       pr "    caml_remove_generational_global_root ((value *)user_data);\n";
> -       pr "    free (user_data);\n";
> -       pr "  }\n";
> +       pr "  if (valid_flag & LIBNBD_CALLBACK_FREE)\n";
> +       pr "    free_root (NULL, user_data);\n";
>         pr "\n";

Here, I'm unfamiliar enough with the OCaml GC rules to know if you're
doing it right, but trust that you have been testing with valgrind or
similar with a forced GC run to catch obvious mistakes.

> @@ -5313,17 +5306,39 @@ let print_ocaml_binding (name, { args; optargs; ret }) =
>      | BytesIn (n, count) ->
>         pr "  const void *%s = Bytes_val (%sv);\n" n n;
>         pr "  size_t %s = caml_string_length (%sv);\n" count n
> +    | BytesOut (n, count) ->
> +       pr "  void *%s = Bytes_val (%sv);\n" n n;
> +       pr "  size_t %s = caml_string_length (%sv);\n" count n
>      | BytesPersistIn (n, count) ->
> +       pr "  /* The function may save a reference to the Buffer, so we\n";
> +       pr "   * must treat it as a possible GC root.\n";
> +       pr "   */\n";
> +       pr "  value *%s_user_data;\n" n;
> +       pr "  %s_user_data = malloc (sizeof (value));\n" n;
> +       pr "  if (%s_user_data == NULL) caml_raise_out_of_memory ();\n" n;
> +       pr "  *%s_user_data = %sv;\n" n n;
> +       pr "  caml_register_generational_global_root (%s_user_data);\n" n;
>         pr "  struct nbd_buffer *%s_buf = NBD_buffer_val (%sv);\n" n n;
>         pr "  const void *%s = %s_buf->data;\n" n n;
> -       pr "  size_t %s = %s_buf->len;\n" count n
> -    | BytesOut (n, count) ->
> -       pr "  void *%s = Bytes_val (%sv);\n" n n;
> -       pr "  size_t %s = caml_string_length (%sv);\n" count n
> +       pr "  size_t %s = %s_buf->len;\n" count n;
> +       pr "  if (nbd_add_free_callback (h, (void *)%s,\n" n;

Is the cast to void* necessary?  I guess so - it looks like you are
casting away const.

> +       pr "                             free_root, %s_user_data) == -1)\n" n;

Hmm. In patch 1, I questioned if we need a separate user_data argument
when registering a free callback.  But here, you are keying off of the
last use of one pointer (the buffer), but with a different pointer (the
user_data wrapper) to be passed to the free function...

> +++ b/ocaml/nbd-c.h
> @@ -107,4 +107,12 @@ struct callback_data {
>  extern char **nbd_internal_ocaml_string_list (value);
>  extern value nbd_internal_ocaml_alloc_int32_array (uint32_t *, size_t);
>  
> +/* Free a generational global root and its container. */
> +static inline void
> +free_root (void *ptr /* unused */, void *rootp)
> +{
> +  caml_remove_generational_global_root ((value *)rootp);
> +  free (rootp);
> +}

...and indeed, the free function is ignoring the first pointer and
instead acting on the second.  If we change the C signature to take only
a single pointer, then you have to be a bit more creative on h ow to
pass the rootp as the pointer to be adjusted when the buffer lifetime
ends.  So maybe this is the answer to my question in patch 1 as to why
you needed two arguments. Or maybe it is all the more reason to try
harder to get the generator to call nbd_aio_pread_callback even when the
OCaml language did not pass a callback.

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

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libguestfs/attachments/20190812/762cf4e9/attachment.sig>