11:58 a.m.
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