We have defined the concept of a "null callback" meaning one where the .callback field = NULL. The first two new macros just test this property, and the third one sets a callback to null. This change is neutral refactoring. --- generator/generator | 4 ++-- generator/states-reply-simple.c | 2 +- generator/states-reply-structured.c | 11 ++++++----- generator/states-reply.c | 2 +- generator/states.c | 2 +- lib/debug.c | 2 +- lib/internal.h | 9 +++++++-- 7 files changed, 19 insertions(+), 13 deletions(-) diff --git a/generator/generator b/generator/generator index ca97910..98c99e0 100755 --- a/generator/generator +++ b/generator/generator @@ -3576,7 +3576,7 @@ let generate_lib_api_c () = let value = match errcode with | Some value -> value | None -> assert false in - pr " if (%s_callback.callback == NULL) {\n" cbname; + pr " if (CALLBACK_IS_NULL (%s_callback)) {\n" cbname; pr " set_error (EFAULT, \"%%s cannot be NULL\", \"%s\");\n" cbname; pr " ret = %s;\n" value; pr " goto out;\n"; @@ -3689,7 +3689,7 @@ let generate_lib_api_c () = List.iter ( function | OClosure { cbname } -> - pr ", %s_callback.callback ? \"<fun>\" : \"NULL\"" cbname + pr ", CALLBACK_IS_NULL (%s_callback) ? \"<fun>\" : \"NULL\"" cbname | OFlags (n, _) -> pr ", %s" n ) optargs; pr ");\n" diff --git a/generator/states-reply-simple.c b/generator/states-reply-simple.c index 2f83e6f..5e5b631 100644 --- a/generator/states-reply-simple.c +++ b/generator/states-reply-simple.c @@ -60,7 +60,7 @@ case 0: /* guaranteed by START */ assert (cmd); - if (cmd->cb.fn.chunk.callback) { + if (CALLBACK_IS_NOT_NULL (cmd->cb.fn.chunk)) { int error = 0; assert (cmd->error == 0); diff --git a/generator/states-reply-structured.c b/generator/states-reply-structured.c index a1641d4..85f0775 100644 --- a/generator/states-reply-structured.c +++ b/generator/states-reply-structured.c @@ -157,7 +157,7 @@ set_error (0, "invalid length in NBD_REPLY_TYPE_BLOCK_STATUS"); return 0; } - if (cmd->cb.fn.extent.callback == NULL) { + if (CALLBACK_IS_NULL (cmd->cb.fn.extent)) { SET_NEXT_STATE (%.DEAD); set_error (0, "not expecting NBD_REPLY_TYPE_BLOCK_STATUS here"); return 0; @@ -293,7 +293,8 @@ offset, cmd->offset, cmd->count); return 0; } - if (cmd->type == NBD_CMD_READ && cmd->cb.fn.chunk.callback) { + if (cmd->type == NBD_CMD_READ && + CALLBACK_IS_NOT_NULL (cmd->cb.fn.chunk)) { int scratch = error; /* Different from successful reads: inform the callback about the @@ -385,7 +386,7 @@ offset = be64toh (h->sbuf.sr.payload.offset_data.offset); assert (cmd); /* guaranteed by CHECK */ - if (cmd->cb.fn.chunk.callback) { + if (CALLBACK_IS_NOT_NULL (cmd->cb.fn.chunk)) { int error = cmd->error; if (CALL_CALLBACK (cmd->cb.fn.chunk, cmd->data + (offset - cmd->offset), @@ -446,7 +447,7 @@ * them as an extension, and this works even when length == 0. */ memset (cmd->data + offset, 0, length); - if (cmd->cb.fn.chunk.callback) { + if (CALLBACK_IS_NOT_NULL (cmd->cb.fn.chunk)) { int error = cmd->error; if (CALL_CALLBACK (cmd->cb.fn.chunk, @@ -479,7 +480,7 @@ assert (cmd); /* guaranteed by CHECK */ assert (cmd->type == NBD_CMD_BLOCK_STATUS); - assert (cmd->cb.fn.extent.callback); + assert (CALLBACK_IS_NOT_NULL (cmd->cb.fn.extent)); assert (h->bs_entries); assert (length >= 12); diff --git a/generator/states-reply.c b/generator/states-reply.c index 575a6d1..14bb010 100644 --- a/generator/states-reply.c +++ b/generator/states-reply.c @@ -168,7 +168,7 @@ save_reply_state (struct nbd_handle *h) retire = cmd->type == NBD_CMD_DISC; /* Notify the user */ - if (cmd->cb.completion.callback) { + if (CALLBACK_IS_NOT_NULL (cmd->cb.completion)) { int error = cmd->error; int r; diff --git a/generator/states.c b/generator/states.c index cfa9957..32bf975 100644 --- a/generator/states.c +++ b/generator/states.c @@ -121,7 +121,7 @@ void abort_commands (struct nbd_handle *h, bool retire = cmd->type == NBD_CMD_DISC; next = cmd->next; - if (cmd->cb.completion.callback) { + if (CALLBACK_IS_NOT_NULL (cmd->cb.completion)) { int error = cmd->error ? cmd->error : ENOTCONN; int r; diff --git a/lib/debug.c b/lib/debug.c index eec2051..90d2f71 100644 --- a/lib/debug.c +++ b/lib/debug.c @@ -82,7 +82,7 @@ nbd_internal_debug (struct nbd_handle *h, const char *fs, ...) if (r == -1) goto out; - if (h->debug_callback.callback) + if (CALLBACK_IS_NOT_NULL (h->debug_callback)) /* ignore return value */ CALL_CALLBACK (h->debug_callback, context, msg); else diff --git a/lib/internal.h b/lib/internal.h index dc3676a..a18d581 100644 --- a/lib/internal.h +++ b/lib/internal.h @@ -273,6 +273,11 @@ struct command { uint32_t error; /* Local errno value */ }; +/* Test if a callback is "null" or not, and set it to null. */ +#define CALLBACK_IS_NULL(cb) ((cb).callback == NULL) +#define CALLBACK_IS_NOT_NULL(cb) (! CALLBACK_IS_NULL ((cb))) +#define SET_CALLBACK_TO_NULL(cb) ((cb).callback = NULL) + /* Call a callback. */ #define CALL_CALLBACK(cb, ...) \ (cb).callback ((cb).user_data, ##__VA_ARGS__) @@ -286,9 +291,9 @@ struct command { #define FREE_CALLBACK(cb) \ do { \ nbd_completion_callback *_cb = (nbd_completion_callback *)&(cb); \ - if (_cb->callback != NULL && _cb->free != NULL) \ + if (CALLBACK_IS_NOT_NULL (cb) && _cb->free != NULL) \ _cb->free (_cb->user_data); \ - _cb->callback = NULL; \ + SET_CALLBACK_TO_NULL (cb); \ } while (0) /* aio.c */ -- 2.22.0

Why did I need to cast cb to a particular type? I don't know. Fixes commit 9c8fccdf382c2c8483b557acc6b5d41a4e193990. --- lib/internal.h | 12 +++--------- 1 file changed, 3 insertions(+), 9 deletions(-) diff --git a/lib/internal.h b/lib/internal.h index a18d581..1344d98 100644 --- a/lib/internal.h +++ b/lib/internal.h @@ -282,17 +282,11 @@ struct command { #define CALL_CALLBACK(cb, ...) \ (cb).callback ((cb).user_data, ##__VA_ARGS__) -/* Free a callback. - * - * Note this works for any type of callback because the basic layout - * of the struct is the same for all of them. Therefore casting cb to - * nbd_completion_callback does not change the effective code. - */ +/* Free a callback. */ #define FREE_CALLBACK(cb) \ do { \ - nbd_completion_callback *_cb = (nbd_completion_callback *)&(cb); \ - if (CALLBACK_IS_NOT_NULL (cb) && _cb->free != NULL) \ - _cb->free (_cb->user_data); \ + if (CALLBACK_IS_NOT_NULL (cb) && (cb).free != NULL) \ + (cb).free ((cb).user_data); \ SET_CALLBACK_TO_NULL (cb); \ } while (0) -- 2.22.0

On 8/15/19 4:56 AM, Richard W.M. Jones wrote: Semantic change. In generator, you used CALLBACK_IS_NULL() for both Closure and OClosure. For OClosure, the new semantics are still correct. But for Closure, we now no longer return EFAULT when the callback itself is missing but a .free was provided. This changes pread_structured and block_status to accept NULL for the callback; which is probably not a good idea. In short, there are some points in the code that care only whether .callback is NULL, and others that care whether both pointers are NULL. This one is nice, though. The SET_CALLBACK_TO_NULL usage is mostly dead code during command retirement (as we are about to free() the struct containing cb in the first place), but still vital when clearing the debug callback. So this part is also fine. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org

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

--- generator/generator | 22 ++++++++++++++++++++-- 1 file changed, 20 insertions(+), 2 deletions(-) diff --git a/generator/generator b/generator/generator index 8c055a9..1252bdb 100755 --- a/generator/generator +++ b/generator/generator @@ -4262,6 +4262,20 @@ let print_python_binding name { args; optargs; ret; may_set_error } = | OFlags (n, _) -> pr " %s_u32 = %s;\n" n n ) optargs; + (* If there is a BytesPersistIn/Out parameter then we need to + * increment the refcount and save the pointer into + * completion_callback.user_data so we can decrement the + * refcount on command completion. + *) + List.iter ( + function + | BytesPersistIn (n, _) | BytesPersistOut (n, _) -> + pr " /* Increment refcount since buffer may be saved by libnbd. */\n"; + pr " Py_INCREF (%s);\n" n; + pr " completion_user_data->buf = %s;\n" n; + | _ -> () + ) args; + (* Call the underlying C function. *) pr " ret = nbd_%s (h" name; List.iter ( @@ -4389,7 +4403,8 @@ let generate_python_methods_c () = pr " * and freed in the free_user_data function below.\n"; pr " */\n"; pr "struct user_data {\n"; - pr " PyObject *fn; /* Pointer to Python function. */\n"; + pr " PyObject *fn; /* Optional pointer to Python function. */\n"; + pr " PyObject *buf; /* Optional pointer to persistent buffer. */\n"; pr "};\n"; pr "\n"; pr "static struct user_data *\n"; @@ -4408,7 +4423,10 @@ let generate_python_methods_c () = pr "{\n"; pr " struct user_data *data = user_data;\n"; pr "\n"; - pr " Py_DECREF (data->fn);\n"; + pr " if (data->fn != NULL)\n"; + pr " Py_DECREF (data->fn);\n"; + pr " if (data->buf != NULL)\n"; + pr " Py_DECREF (data->buf);\n"; pr " free (data);\n"; pr "}\n"; pr "\n"; -- 2.22.0

--- generator/generator | 25 +++++++++++++++++++++++++ 1 file changed, 25 insertions(+) diff --git a/generator/generator b/generator/generator index 1252bdb..13cb0b9 100755 --- a/generator/generator +++ b/generator/generator @@ -3121,6 +3121,31 @@ let () = failwithf "%s: first_version must be 1.x" name; if minor mod 2 <> 0 then failwithf "%s: first_version must refer to a stable release" name + ) handle_calls; + + (* Because of the way we use completion free callbacks to + * free persistent buffers in non-C languages, any function + * with a BytesPersistIn/Out parameter must have only one. + * And it must have an OClosure completion optarg. + *) + List.iter ( + fun (name, { args; optargs }) -> + let is_persistent_buffer_arg = function + | BytesPersistIn _ | BytesPersistOut _ -> true + | _ -> false + and is_oclosure_completion = function + | OClosure { cbname = "completion" } -> true + | _ -> false + in + if List.exists is_persistent_buffer_arg args then ( + let bpargs = List.filter is_persistent_buffer_arg args in + if List.length bpargs >= 2 then + failwithf "%s: multiple BytesPersistIn/Out params not supported" + name; + if not (List.exists is_oclosure_completion optargs) then + failwithf "%s: functions with BytesPersistIn/Out arg must have completion callback" + name + ) ) handle_calls let generate_lib_libnbd_syms () = -- 2.22.0