[Libguestfs] [PATCH libnbd 3/7] ocaml: Remove NBD.Buffer.free function, use a free callback instead.
Richard W.M. Jones
rjones at redhat.com
Mon Aug 12 16:08:47 UTC 2019
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. *)
-
val to_bytes : t -> bytes
(** Copy buffer to an OCaml [bytes] object. *)
@@ -5062,7 +5058,6 @@ let () =
module Buffer = struct
type t
external alloc : int -> t = \"nbd_internal_ocaml_buffer_alloc\"
- external free : t -> unit = \"nbd_internal_ocaml_buffer_free\"
external to_bytes : t -> bytes = \"nbd_internal_ocaml_buffer_to_bytes\"
external of_bytes : bytes -> t = \"nbd_internal_ocaml_buffer_of_bytes\"
external size : t -> int = \"nbd_internal_ocaml_buffer_size\"
@@ -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";
pr " return ret;\n";
pr "}\n";
@@ -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;
+ pr " free_root, %s_user_data) == -1)\n" n;
+ pr " caml_raise_out_of_memory ();\n"
| BytesPersistOut (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 " void *%s = %s_buf->data;\n" n n;
- pr " size_t %s = %s_buf->len;\n" count n
+ pr " size_t %s = %s_buf->len;\n" count n;
+ pr " if (nbd_add_free_callback (h, %s,\n" n;
+ pr " free_root, %s_user_data) == -1)\n" n;
+ pr " caml_raise_out_of_memory ();\n"
| Closure { cbname } ->
pr " /* The function may save a reference to the closure, so we\n";
pr " * must treat it as a possible GC root.\n";
diff --git a/ocaml/buffer.c b/ocaml/buffer.c
index 837eece..0c0fe00 100644
--- a/ocaml/buffer.c
+++ b/ocaml/buffer.c
@@ -36,10 +36,6 @@ nbd_buffer_finalize (value bv)
{
struct nbd_buffer *b = NBD_buffer_val (bv);
- /* Usually if this frees the buffer it indicates a bug in the
- * program. The buffer should have been manually freed before
- * this. But there's nothing we can do here, so free it.
- */
free (b->data);
}
@@ -60,21 +56,6 @@ nbd_internal_ocaml_buffer_alloc (value sizev)
CAMLreturn (rv);
}
-/* Free an NBD persistent buffer (only the C buffer, not the
- * OCaml object).
- */
-value
-nbd_internal_ocaml_buffer_free (value bv)
-{
- CAMLparam1 (bv);
- struct nbd_buffer *b = NBD_buffer_val (bv);
-
- free (b->data);
- b->data = NULL;
-
- CAMLreturn (Val_unit);
-}
-
/* Copy an NBD persistent buffer to an OCaml bytes. */
value
nbd_internal_ocaml_buffer_to_bytes (value bv)
@@ -83,9 +64,6 @@ nbd_internal_ocaml_buffer_to_bytes (value bv)
CAMLlocal1 (rv);
struct nbd_buffer *b = NBD_buffer_val (bv);
- if (b->data == NULL) /* Buffer has been freed. */
- caml_invalid_argument ("NBD.Buffer.to_bytes used on freed buffer");
-
rv = caml_alloc_string (b->len);
memcpy (Bytes_val (rv), b->data, b->len);
diff --git a/ocaml/examples/asynch_copy.ml b/ocaml/examples/asynch_copy.ml
index 7dbe976..50357f9 100644
--- a/ocaml/examples/asynch_copy.ml
+++ b/ocaml/examples/asynch_copy.ml
@@ -31,11 +31,9 @@ let asynch_copy src dst =
in
(* This callback is called when any pwrite to the destination
- * has completed. We have to manually free the buffer here
- * as it is no longer used.
+ * has completed.
*)
let write_completed buf _ =
- NBD.Buffer.free buf;
(* By returning 1 here we auto-retire the pwrite command. *)
1
in
diff --git a/ocaml/libnbd-ocaml.pod b/ocaml/libnbd-ocaml.pod
index f5f968c..5e161c6 100644
--- a/ocaml/libnbd-ocaml.pod
+++ b/ocaml/libnbd-ocaml.pod
@@ -34,28 +34,6 @@ which is the printable error message. The second is the raw C<errno>,
if available (see C<nbd_get_errno>). The raw C<errno> is not
compatible with errors in the OCaml C<Unix> module unfortunately.
-=head1 AIO BUFFERS
-
-Some libnbd asynchronous I/O (AIO) calls require a buffer parameter
-which persists after the call finishes. For example C<NBD.aio_pread>
-starts a command which continues reading into the C<NBD.Buffer.t>
-parameter in subsequent calls after the original C<aio_pread> call
-returns.
-
-For these cases you must create a C<NBD.Buffer.t> and ensure that it
-is not garbage collected until the command completes. The easiest way
-to do this is to use the C<*_callback> variants and free the buffer in
-the callback:
-
- let buf = NBD.Buffer.alloc 512 in
- NBD.aio_pread_callback h buf 0_L (
- (* This is called when the command has completed. *)
- fun _ ->
- NBD.Buffer.free buf;
- (* Returning 1 from this callback auto-retires the command. *)
- 1
- )
-
=head1 EXAMPLES
This directory contains examples written in OCaml:
diff --git a/ocaml/nbd-c.h b/ocaml/nbd-c.h
index ffd51d2..a84d64e 100644
--- a/ocaml/nbd-c.h
+++ 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);
+}
+
#endif /* LIBNBD_NBD_C_H */
diff --git a/ocaml/tests/test_590_aio_copy.ml b/ocaml/tests/test_590_aio_copy.ml
index 290ca93..effc299 100644
--- a/ocaml/tests/test_590_aio_copy.ml
+++ b/ocaml/tests/test_590_aio_copy.ml
@@ -53,11 +53,9 @@ let asynch_copy src dst =
in
(* This callback is called when any pwrite to the destination
- * has completed. We have to manually free the buffer here
- * as it is no longer used.
+ * has completed.
*)
let write_completed buf _ =
- NBD.Buffer.free buf;
bytes_written := !bytes_written + NBD.Buffer.size buf;
(* By returning 1 here we auto-retire the pwrite command. *)
1
--
2.22.0
More information about the Libguestfs
mailing list