[Libguestfs] [PATCH libnbd v2 06/10] ocaml: Remove NBD.Buffer.free function, use the completion callback instead.
Richard W.M. Jones
rjones at redhat.com
Thu Aug 15 09:56:17 UTC 2019
By using the completion free callback function we don't need to
manually free the buffer.
This reverts part of commit fef1c281a65d061127bf178e5f8cfca0a2475c56.
---
generator/generator | 49 ++++++++++++++++++++------------
ocaml/buffer.c | 22 --------------
ocaml/examples/asynch_copy.ml | 4 +--
ocaml/libnbd-ocaml.pod | 22 --------------
ocaml/tests/test_590_aio_copy.ml | 4 +--
5 files changed, 33 insertions(+), 68 deletions(-)
diff --git a/generator/generator b/generator/generator
index 35936c6..f307485 100755
--- a/generator/generator
+++ b/generator/generator
@@ -4747,10 +4747,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. *)
@@ -4848,7 +4844,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\"
@@ -5085,18 +5080,18 @@ let print_ocaml_binding (name, { args; optargs; ret }) =
function
| OClosure { cbname } ->
pr " nbd_%s_callback %s_callback = {0};\n" cbname cbname;
+ pr " struct user_data *%s_user_data = alloc_user_data ();\n" cbname;
pr " if (%sv != Val_int (0)) { /* Some closure */\n" cbname;
pr " /* The function may save a reference to the closure, so we\n";
pr " * must treat it as a possible GC root.\n";
pr " */\n";
- pr " struct user_data *user_data = alloc_user_data ();\n";
- pr "\n";
- pr " user_data->fnv = Field (%sv, 0);\n" cbname;
- pr " caml_register_generational_global_root (&user_data->fnv);\n";
+ pr " %s_user_data->fnv = Field (%sv, 0);\n" cbname cbname;
+ pr " caml_register_generational_global_root (&%s_user_data->fnv);\n"
+ cbname;
pr " %s_callback.callback = %s_wrapper;\n" cbname cbname;
- pr " %s_callback.user_data = user_data;\n" cbname;
- pr " %s_callback.free = free_user_data;\n" cbname;
pr " }\n";
+ pr " %s_callback.user_data = %s_user_data;\n" cbname cbname;
+ pr " %s_callback.free = free_user_data;\n" cbname;
| OFlags (n, { flag_prefix }) ->
pr " uint32_t %s;\n" n;
pr " if (%sv != Val_int (0)) /* Some [ list of %s.t ] */\n"
@@ -5125,15 +5120,16 @@ let print_ocaml_binding (name, { args; optargs; ret }) =
pr " void *%s = %s_buf->data;\n" n n;
pr " size_t %s = %s_buf->len;\n" count n
| Closure { cbname } ->
+ pr " nbd_%s_callback %s_callback;\n" cbname cbname;
+ pr " struct user_data *%s_user_data = alloc_user_data ();\n" cbname;
pr " /* The function may save a reference to the closure, so we\n";
pr " * must treat it as a possible GC root.\n";
pr " */\n";
- pr " struct user_data *user_data = alloc_user_data ();\n";
- pr " user_data->fnv = %sv;\n" cbname;
- pr " caml_register_generational_global_root (&user_data->fnv);\n";
- pr " nbd_%s_callback %s_callback;\n" cbname cbname;
+ pr " %s_user_data->fnv = %sv;\n" cbname cbname;
+ pr " caml_register_generational_global_root (&%s_user_data->fnv);\n"
+ cbname;
pr " %s_callback.callback = %s_wrapper;\n" cbname cbname;
- pr " %s_callback.user_data = user_data;\n" cbname;
+ pr " %s_callback.user_data = %s_user_data;\n" cbname cbname;
pr " %s_callback.free = free_user_data;\n" cbname
| Enum (n, { enum_prefix }) ->
pr " int %s = %s_val (%sv);\n" n enum_prefix n
@@ -5159,6 +5155,19 @@ let print_ocaml_binding (name, { args; optargs; ret }) =
pr " uint64_t %s = Int64_val (%sv);\n" n n
) args;
+ (* If there is a BytesPersistIn/Out parameter then we need to
+ * register it as a global root and save that into the
+ * completion_callback.user_data so the root is removed on
+ * command completion.
+ *)
+ List.iter (
+ function
+ | BytesPersistIn (n, _) | BytesPersistOut (n, _) ->
+ pr " completion_user_data->bufv = %sv;\n" n;
+ pr " caml_register_generational_global_root (&completion_user_data->bufv);\n"
+ | _ -> ()
+ ) args;
+
let ret_c_type = C.type_of_ret ret and errcode = C.errcode_of_ret ret in
pr " %s r;\n" ret_c_type;
pr "\n";
@@ -5257,7 +5266,8 @@ let generate_ocaml_nbd_c () =
pr " * and freed in the free_user_data function below.\n";
pr " */\n";
pr "struct user_data {\n";
- pr " value fnv; /* GC root pointing to OCaml function. */\n";
+ pr " value fnv; /* Optional GC root pointing to OCaml function. */\n";
+ pr " value bufv; /* Optional GC root pointing to persistent buffer. */\n";
pr "};\n";
pr "\n";
pr "static struct user_data *\n";
@@ -5274,7 +5284,10 @@ let generate_ocaml_nbd_c () =
pr "{\n";
pr " struct user_data *data = user_data;\n";
pr "\n";
- pr " caml_remove_generational_global_root (&data->fnv);\n";
+ pr " if (data->fnv != 0)\n";
+ pr " caml_remove_generational_global_root (&data->fnv);\n";
+ pr " if (data->bufv != 0)\n";
+ pr " caml_remove_generational_global_root (&data->bufv);\n";
pr " free (data);\n";
pr "}\n";
pr "\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 8057118..d5dcc60 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 d370113..ae8fb88 100644
--- a/ocaml/libnbd-ocaml.pod
+++ b/ocaml/libnbd-ocaml.pod
@@ -33,28 +33,6 @@ which is the printable error message. The second is the raw C<errno>,
if available (see L<nbd_get_errno(3)>). 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/tests/test_590_aio_copy.ml b/ocaml/tests/test_590_aio_copy.ml
index defb4cb..ac490ef 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