[Libguestfs] [PATCH libnbd v2 06/10] ocaml: Remove NBD.Buffer.free function, use the completion callback instead.

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