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