Change the way that we deal with freeing closures in language bindings: * The valid_flag is removed (mostly reverting commit 2d9b98e96772e282d51dafac07f16387dadc8afa). * An extra ‘free’ parameter is added to all callback structures. This is called by the library whenever the closure won't be called again (so the user_data can be freed). This is analogous to valid_flag == LIBNBD_CALLBACK_FREE in old code. * Language bindings are updated to deal with these changes. --- TODO | 2 - docs/libnbd.pod | 72 +++++---------- examples/glib-main-loop.c | 14 +-- examples/strict-structured-reads.c | 65 ++++++-------- generator/generator | 132 +++++++++++++--------------- generator/states-reply-simple.c | 5 +- generator/states-reply-structured.c | 64 +++++++------- generator/states-reply.c | 5 +- generator/states.c | 5 +- interop/dirty-bitmap.c | 5 +- interop/structured-read.c | 5 +- lib/aio.c | 24 ++--- lib/debug.c | 9 +- tests/closure-lifetimes.c | 103 ++++++++++++---------- tests/meta-base-allocation.c | 7 +- tests/oldstyle.c | 5 +- tests/server-death.c | 5 +- 17 files changed, 238 insertions(+), 289 deletions(-) diff --git a/TODO b/TODO index 8e067c0..03fda1f 100644 --- a/TODO +++ b/TODO @@ -26,8 +26,6 @@ Improve function trace output so that: Suggested API improvements: general: - synchronous APIs that have a timeout or can be cancelled - - separate free_callback corresponding to every Closure - (instead of using valid_flag) connecting: - nbd_connect_tcp: allow control over whether IPv4 or IPv6 is desired diff --git a/docs/libnbd.pod b/docs/libnbd.pod index 9177825..d1c9ff9 100644 --- a/docs/libnbd.pod +++ b/docs/libnbd.pod @@ -620,56 +620,25 @@ because you can use closures to achieve the same effect. =head2 Callback lifetimes -All callbacks have an C<unsigned valid_flag> parameter which is used -to help with the lifetime of the callback. C<valid_flag> contains the -I<logical or> of: +You can associate a free function with callbacks. Libnbd will call +this function when the callback will not be called again by libnbd. -=over 4 +This can be used to free associated C<user_data>. For example: -=item C<LIBNBD_CALLBACK_VALID> + void *my_data = malloc (...); + + nbd_aio_pread_structured (nbd, buf, sizeof buf, offset, + (nbd_chunk_callback) { .callback = my_fn, + .user_data = my_data, + .free = free }, + NBD_NULL_CALLBACK(completion), + 0); -The callback parameters are valid and this is a normal callback. +will call L<free(3)> on C<my_data> after the last time that the +S<C<chunk.callback = my_fn>> function is called. -=item C<LIBNBD_CALLBACK_FREE> - -This is the last time the library will call this function. Any data -associated with the callback can be freed. - -=item other bits - -Other bits in C<valid_flag> should be ignored. - -=back - -For example C<nbd_set_debug_callback> sets up a callback which you -could define like this: - - int my_debug_fn (unsigned valid_flag, void *user_data, - const char *context, const char *msg) - { - if (valid_flag & LIBNBD_CALLBACK_VALID) { - printf ("context = %s, msg = %s\n", context, msg); - } - if (valid_flag & LIBNBD_CALLBACK_FREE) { - /* If you need to free something, for example: */ - free (user_data); - } - return 0; - } - -If you don't care about the freeing feature then it is also correct to -write this: - - int my_debug_fn (unsigned valid_flag, void *user_data, - const char *context, const char *msg) - { - if (!(valid_flag & LIBNBD_CALLBACK_VALID)) return 0; - - // Rest of callback as normal. - } - -The valid flag is only present in the C API. It is not needed when -using garbage-collected programming languages. +The free function is only accessible in the C API as it is not needed +in garbage collected programming languages. =head2 Callbacks and locking @@ -683,10 +652,10 @@ All of the low-level commands have a completion callback variant that registers a callback function used right before the command is marked complete. -When C<valid_flag> includes C<LIBNBD_CALLBACK_VALID>, and the -completion callback returns C<1>, the command is automatically retired -(there is no need to call C<nbd_aio_command_completed>); for any other -return value, the command still needs to be retired. +When the completion callback returns C<1>, the command is +automatically retired (there is no need to call +C<nbd_aio_command_completed>); for any other return value, the command +still needs to be retired. =head2 Callbacks with C<int *error> parameter @@ -698,8 +667,7 @@ all of the completion callbacks, include a parameter C<error> containing the value of any error detected so far; if the callback function fails, it should assign back into C<error> and return C<-1> to change the resulting error of the overall command. Assignments -into C<error> are ignored for any other return value or when -C<valid_flag> did not contain C<LIBNBD_CALLBACK_VALID>; similarly, +into C<error> are ignored for any other return value; similarly, assigning C<0> into C<error> does not have an effect. =head1 SEE ALSO diff --git a/examples/glib-main-loop.c b/examples/glib-main-loop.c index a8e8ceb..9c279d3 100644 --- a/examples/glib-main-loop.c +++ b/examples/glib-main-loop.c @@ -268,9 +268,9 @@ static GMainLoop *loop; static void connected (struct NBDSource *source); static gboolean read_data (gpointer user_data); -static int finished_read (unsigned valid_flag, void *vp, int *error); +static int finished_read (void *vp, int *error); static gboolean write_data (gpointer user_data); -static int finished_write (unsigned valid_flag, void *vp, int *error); +static int finished_write (void *vp, int *error); int main (int argc, char *argv[]) @@ -395,13 +395,10 @@ read_data (gpointer user_data) /* This callback is called from libnbd when any read command finishes. */ static int -finished_read (unsigned valid_flag, void *vp, int *error) +finished_read (void *vp, int *error) { struct buffer *buffer = vp; - if (!(valid_flag & LIBNBD_CALLBACK_VALID)) - return 0; - if (gssrc == NULL) return 0; @@ -444,13 +441,10 @@ write_data (gpointer user_data) /* This callback is called from libnbd when any write command finishes. */ static int -finished_write (unsigned valid_flag, void *vp, int *error) +finished_write (void *vp, int *error) { struct buffer *buffer = vp; - if (!(valid_flag & LIBNBD_CALLBACK_VALID)) - return 0; - if (gsdest == NULL) return 0; diff --git a/examples/strict-structured-reads.c b/examples/strict-structured-reads.c index d7c3e1b..6ea1700 100644 --- a/examples/strict-structured-reads.c +++ b/examples/strict-structured-reads.c @@ -51,16 +51,13 @@ static int64_t total_bytes; static int total_success; static int -read_chunk (unsigned valid_flag, void *opaque, +read_chunk (void *opaque, const void *bufv, size_t count, uint64_t offset, unsigned status, int *error) { struct data *data = opaque; struct range *r, **prev; - if (!(valid_flag & LIBNBD_CALLBACK_VALID)) - return 0; - /* libnbd guarantees this: */ assert (offset >= data->offset); assert (offset + count <= data->offset + data->count); @@ -131,46 +128,40 @@ read_chunk (unsigned valid_flag, void *opaque, } static int -read_verify (unsigned valid_flag, void *opaque, int *error) +read_verify (void *opaque, int *error) { int ret = 0; + struct data *data = opaque; - if (valid_flag & LIBNBD_CALLBACK_VALID) { - struct data *data = opaque; - - ret = -1; - total_reads++; - total_chunks += data->chunks; - if (*error) - goto cleanup; - assert (data->chunks > 0); - if (data->flags & LIBNBD_CMD_FLAG_DF) { - total_df_reads++; - if (data->chunks > 1) { - fprintf (stderr, "buggy server: too many chunks for DF flag\n"); - *error = EPROTO; - goto cleanup; - } - } - if (data->remaining && !*error) { - fprintf (stderr, "buggy server: not enough chunks on success\n"); + ret = -1; + total_reads++; + total_chunks += data->chunks; + if (*error) + goto cleanup; + assert (data->chunks > 0); + if (data->flags & LIBNBD_CMD_FLAG_DF) { + total_df_reads++; + if (data->chunks > 1) { + fprintf (stderr, "buggy server: too many chunks for DF flag\n"); *error = EPROTO; goto cleanup; } - total_bytes += data->count; - total_success++; - ret = 0; - - cleanup: - while (data->remaining) { - struct range *r = data->remaining; - data->remaining = r->next; - free (r); - } } + if (data->remaining && !*error) { + fprintf (stderr, "buggy server: not enough chunks on success\n"); + *error = EPROTO; + goto cleanup; + } + total_bytes += data->count; + total_success++; + ret = 0; - if (valid_flag & LIBNBD_CALLBACK_FREE) - free (opaque); + cleanup: + while (data->remaining) { + struct range *r = data->remaining; + data->remaining = r->next; + free (r); + } return ret; } @@ -237,7 +228,7 @@ main (int argc, char *argv[]) .remaining = r, }; if (nbd_aio_pread_structured (nbd, buf, sizeof buf, offset, (nbd_chunk_callback) { .callback = read_chunk, .user_data = d }, - (nbd_completion_callback) { .callback = read_verify, .user_data = d }, + (nbd_completion_callback) { .callback = read_verify, .user_data = d, .free = free }, flags) == -1) { fprintf (stderr, "%s\n", nbd_get_error ()); exit (EXIT_FAILURE); diff --git a/generator/generator b/generator/generator index ea32929..553c4b8 100755 --- a/generator/generator +++ b/generator/generator @@ -3067,7 +3067,7 @@ module C : sig val generate_docs_libnbd_api_pod : unit -> unit val print_arg_list : ?handle:bool -> ?types:bool -> arg list -> optarg list -> unit - val print_cbarg_list : ?valid_flag:bool -> ?types:bool -> cbarg list -> unit + val print_cbarg_list : ?types:bool -> cbarg list -> unit val errcode_of_ret : ret -> string option val type_of_ret : ret -> string end = struct @@ -3284,13 +3284,8 @@ let print_extern name args optargs ret = print_call name args optargs ret; pr ";\n" -let print_cbarg_list ?(valid_flag = true) ?(types = true) cbargs = +let print_cbarg_list ?(types = true) cbargs = pr "("; - if valid_flag then ( - if types then pr "unsigned "; - pr "valid_flag"; - pr ", "; - ); if types then pr "void *"; pr "user_data"; @@ -3340,6 +3335,7 @@ let print_closure_structs () = pr " int (*callback) "; print_cbarg_list cbargs; pr ";\n"; + pr " void (*free) (void *user_data);\n"; pr "} nbd_%s_callback;\n" cbname; pr "\n"; ) all_closures; @@ -3418,9 +3414,6 @@ let generate_include_libnbd_h () = pr "#define %-40s %d\n" n i ) constants; pr "\n"; - pr "#define LIBNBD_CALLBACK_VALID 1\n"; - pr "#define LIBNBD_CALLBACK_FREE 2\n"; - pr "\n"; pr "extern struct nbd_handle *nbd_create (void);\n"; pr "#define LIBNBD_HAVE_NBD_CREATE 1\n"; pr "\n"; @@ -4032,26 +4025,25 @@ let print_python_closure_wrapper { cbname; cbargs } = pr "{\n"; pr " int ret = 0;\n"; pr "\n"; - pr " if (valid_flag & LIBNBD_CALLBACK_VALID) {\n"; - pr " PyGILState_STATE py_save = PyGILState_UNLOCKED;\n"; - pr " PyObject *py_args, *py_ret;\n"; + pr " PyGILState_STATE py_save = PyGILState_UNLOCKED;\n"; + pr " PyObject *py_args, *py_ret;\n"; List.iter ( function | CBArrayAndLen (UInt32 n, len) -> - pr " PyObject *py_%s = PyList_New (%s);\n" n len; - pr " for (size_t i = 0; i < %s; ++i)\n" len; - pr " PyList_SET_ITEM (py_%s, i, PyLong_FromUnsignedLong (%s[i]));\n" n n + pr " PyObject *py_%s = PyList_New (%s);\n" n len; + pr " for (size_t i = 0; i < %s; ++i)\n" len; + pr " PyList_SET_ITEM (py_%s, i, PyLong_FromUnsignedLong (%s[i]));\n" n n | CBBytesIn _ | CBInt _ | CBInt64 _ -> () | CBMutable (Int n) -> - pr " PyObject *py_%s_modname = PyUnicode_FromString (\"ctypes\");\n" n; - pr " if (!py_%s_modname) { PyErr_PrintEx (0); return -1; }\n" n; - pr " PyObject *py_%s_mod = PyImport_Import (py_%s_modname);\n" n n; - pr " Py_DECREF (py_%s_modname);\n" n; - pr " if (!py_%s_mod) { PyErr_PrintEx (0); return -1; }\n" n; - pr " PyObject *py_%s = PyObject_CallMethod (py_%s_mod, \"c_int\", \"i\", *%s);\n" n n n; - pr " if (!py_%s) { PyErr_PrintEx (0); return -1; }\n" n; + pr " PyObject *py_%s_modname = PyUnicode_FromString (\"ctypes\");\n" n; + pr " if (!py_%s_modname) { PyErr_PrintEx (0); return -1; }\n" n; + pr " PyObject *py_%s_mod = PyImport_Import (py_%s_modname);\n" n n; + pr " Py_DECREF (py_%s_modname);\n" n; + pr " if (!py_%s_mod) { PyErr_PrintEx (0); return -1; }\n" n; + pr " PyObject *py_%s = PyObject_CallMethod (py_%s_mod, \"c_int\", \"i\", *%s);\n" n n n; + pr " if (!py_%s) { PyErr_PrintEx (0); return -1; }\n" n; | CBString _ | CBUInt _ | CBUInt64 _ -> () @@ -4059,7 +4051,7 @@ let print_python_closure_wrapper { cbname; cbargs } = ) cbargs; pr "\n"; - pr " py_args = Py_BuildValue (\"(\""; + pr " py_args = Py_BuildValue (\"(\""; List.iter ( function | CBArrayAndLen (UInt32 n, len) -> pr " \"O\"" @@ -4084,53 +4076,55 @@ let print_python_closure_wrapper { cbname; cbargs } = | CBArrayAndLen _ | CBMutable _ -> assert false ) cbargs; pr ");\n"; - pr " Py_INCREF (py_args);\n"; + pr " Py_INCREF (py_args);\n"; pr "\n"; - pr " if (PyEval_ThreadsInitialized ())\n"; - pr " py_save = PyGILState_Ensure ();\n"; + pr " if (PyEval_ThreadsInitialized ())\n"; + pr " py_save = PyGILState_Ensure ();\n"; pr "\n"; - pr " py_ret = PyObject_CallObject ((PyObject *)user_data, py_args);\n"; + pr " py_ret = PyObject_CallObject ((PyObject *)user_data, py_args);\n"; pr "\n"; - pr " if (PyEval_ThreadsInitialized ())\n"; - pr " PyGILState_Release (py_save);\n"; + pr " if (PyEval_ThreadsInitialized ())\n"; + pr " PyGILState_Release (py_save);\n"; pr "\n"; - pr " Py_DECREF (py_args);\n"; + pr " Py_DECREF (py_args);\n"; pr "\n"; - pr " if (py_ret != NULL) {\n"; - pr " if (PyLong_Check (py_ret))\n"; - pr " ret = PyLong_AsLong (py_ret);\n"; - pr " else\n"; - pr " /* If it's not a long, just assume it's 0. */\n"; - pr " ret = 0;\n"; - pr " Py_DECREF (py_ret);\n"; - pr " }\n"; - pr " else {\n"; - pr " ret = -1;\n"; - pr " PyErr_PrintEx (0); /* print exception */\n"; - pr " };\n"; + pr " if (py_ret != NULL) {\n"; + pr " if (PyLong_Check (py_ret))\n"; + pr " ret = PyLong_AsLong (py_ret);\n"; + pr " else\n"; + pr " /* If it's not a long, just assume it's 0. */\n"; + pr " ret = 0;\n"; + pr " Py_DECREF (py_ret);\n"; + pr " }\n"; + pr " else {\n"; + pr " ret = -1;\n"; + pr " PyErr_PrintEx (0); /* print exception */\n"; + pr " };\n"; pr "\n"; List.iter ( function | CBArrayAndLen (UInt32 n, _) -> - pr " Py_DECREF (py_%s);\n" n + pr " Py_DECREF (py_%s);\n" n | CBMutable (Int n) -> - pr " PyObject *py_%s_ret = PyObject_GetAttrString (py_%s, \"value\");\n" n n; - pr " *%s = PyLong_AsLong (py_%s_ret);\n" n n; - pr " Py_DECREF (py_%s_ret);\n" n; - pr " Py_DECREF (py_%s);\n" n + pr " PyObject *py_%s_ret = PyObject_GetAttrString (py_%s, \"value\");\n" n n; + pr " *%s = PyLong_AsLong (py_%s_ret);\n" n n; + pr " Py_DECREF (py_%s_ret);\n" n; + pr " Py_DECREF (py_%s);\n" n | CBBytesIn _ | CBInt _ | CBInt64 _ | CBString _ | CBUInt _ | CBUInt64 _ -> () | CBArrayAndLen _ | CBMutable _ -> assert false ) cbargs; - pr " }\n"; - pr "\n"; - pr " if (valid_flag & LIBNBD_CALLBACK_FREE)\n"; - pr " Py_DECREF ((PyObject *)user_data);\n"; - pr "\n"; pr " return ret;\n"; pr "}\n"; + pr "\n"; + pr "/* Free for %s callback. */\n" cbname; + pr "static void\n"; + pr "%s_free (void *user_data)\n" cbname; + pr "{\n"; + pr " Py_DECREF (user_data);\n"; + pr "}\n"; pr "\n" (* Generate the Python binding. *) @@ -4156,8 +4150,8 @@ let print_python_binding name { args; optargs; ret; may_set_error } = n; pr " struct py_aio_buffer *%s_buf;\n" n | Closure { cbname } -> - pr " nbd_%s_callback %s = { .callback = %s_wrapper };\n" - cbname cbname cbname + pr " nbd_%s_callback %s = { .callback = %s_wrapper, .free = %s_free };\n" + cbname cbname cbname cbname | Enum (n, _) -> pr " int %s;\n" n | Flags (n, _) -> pr " uint32_t %s_u32;\n" n; @@ -4189,8 +4183,8 @@ let print_python_binding name { args; optargs; ret; may_set_error } = List.iter ( function | OClosure { cbname } -> - pr " nbd_%s_callback %s = { .callback = %s_wrapper };\n" - cbname cbname cbname + pr " nbd_%s_callback %s = { .callback = %s_wrapper, .free = %s_free };\n" + cbname cbname cbname cbname | OFlags (n, _) -> pr " uint32_t %s_u32;\n" n; pr " unsigned int %s; /* really uint32_t */\n" n @@ -4984,7 +4978,7 @@ let print_ocaml_closure_wrapper { cbname; cbargs } = pr "/* Wrapper for %s callback. */\n" cbname; pr "static int\n"; pr "%s_wrapper_locked " cbname; - C.print_cbarg_list ~valid_flag:false cbargs; + C.print_cbarg_list cbargs; pr "\n"; pr "{\n"; pr " CAMLparam0 ();\n"; @@ -5064,21 +5058,20 @@ let print_ocaml_closure_wrapper { cbname; cbargs } = pr "{\n"; pr " int ret = 0;\n"; pr "\n"; - pr " if (valid_flag & LIBNBD_CALLBACK_VALID) {\n"; pr " caml_leave_blocking_section ();\n"; pr " ret = %s_wrapper_locked " cbname; - C.print_cbarg_list ~valid_flag:false ~types:false cbargs; + C.print_cbarg_list ~types:false cbargs; pr ";\n"; 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 "\n"; pr " return ret;\n"; pr "}\n"; + + pr "static void\n"; + pr "%s_free (void *user_data)\n" cbname; + pr "{\n"; + pr " caml_remove_generational_global_root (user_data);\n"; + pr " free (user_data);\n"; + pr "}\n"; pr "\n" let print_ocaml_binding (name, { args; optargs; ret }) = @@ -5137,6 +5130,7 @@ let print_ocaml_binding (name, { args; optargs; ret }) = cbname cbname; pr " caml_register_generational_global_root (%s_callback.user_data);\n" cbname; pr " %s_callback.callback = %s_wrapper;\n" cbname cbname; + pr " %s_callback.free = %s_free;\n" cbname cbname; pr " }\n"; | OFlags (n, { flag_prefix }) -> pr " uint32_t %s;\n" n; @@ -5169,8 +5163,8 @@ let print_ocaml_binding (name, { args; optargs; ret }) = 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 " nbd_%s_callback %s_callback = { .callback = %s_wrapper };\n" - cbname cbname cbname; + pr " nbd_%s_callback %s_callback = { .callback = %s_wrapper, .free = %s_free };\n" + cbname cbname cbname cbname; pr " %s_callback.user_data = malloc (sizeof (value));\n" cbname; pr " if (%s_callback.user_data == NULL) caml_raise_out_of_memory ();\n" cbname; diff --git a/generator/states-reply-simple.c b/generator/states-reply-simple.c index f1d3c62..7f2775d 100644 --- a/generator/states-reply-simple.c +++ b/generator/states-reply-simple.c @@ -64,12 +64,13 @@ int error = 0; assert (cmd->error == 0); - if (cmd->cb.fn.chunk.callback (LIBNBD_CALLBACK_VALID|LIBNBD_CALLBACK_FREE, - cmd->cb.fn.chunk.user_data, + if (cmd->cb.fn.chunk.callback (cmd->cb.fn.chunk.user_data, cmd->data, cmd->count, cmd->offset, LIBNBD_READ_DATA, &error) == -1) cmd->error = error ? error : EPROTO; + if (cmd->cb.fn.chunk.free) + cmd->cb.fn.chunk.free (cmd->cb.fn.chunk.user_data); cmd->cb.fn.chunk.callback = NULL; /* because we've freed it */ } diff --git a/generator/states-reply-structured.c b/generator/states-reply-structured.c index 92d6b5f..7c4d63e 100644 --- a/generator/states-reply-structured.c +++ b/generator/states-reply-structured.c @@ -18,17 +18,6 @@ /* State machine for parsing structured replies from the server. */ -static unsigned -valid_flags (struct nbd_handle *h) -{ - unsigned valid = LIBNBD_CALLBACK_VALID; - uint16_t flags = be16toh (h->sbuf.sr.structured_reply.flags); - - if (flags & NBD_REPLY_FLAG_DONE) - valid |= LIBNBD_CALLBACK_FREE; - return valid; -} - /*----- End of prologue. -----*/ /* STATE MACHINE */ { @@ -306,20 +295,23 @@ valid_flags (struct nbd_handle *h) } if (cmd->type == NBD_CMD_READ && cmd->cb.fn.chunk.callback) { int scratch = error; - unsigned valid = valid_flags (h); + uint16_t flags = be16toh (h->sbuf.sr.structured_reply.flags); /* Different from successful reads: inform the callback about the * current error rather than any earlier one. If the callback fails * without setting errno, then use the server's error below. */ - if (cmd->cb.fn.chunk.callback (valid, cmd->cb.fn.chunk.user_data, + if (cmd->cb.fn.chunk.callback (cmd->cb.fn.chunk.user_data, cmd->data + (offset - cmd->offset), 0, offset, LIBNBD_READ_ERROR, &scratch) == -1) if (cmd->error == 0) cmd->error = scratch; - if (valid & LIBNBD_CALLBACK_FREE) + if (flags & NBD_REPLY_FLAG_DONE) { + if (cmd->cb.fn.chunk.free) + cmd->cb.fn.chunk.free (cmd->cb.fn.chunk.user_data); cmd->cb.fn.chunk.callback = NULL; /* because we've freed it */ + } } } @@ -401,16 +393,19 @@ valid_flags (struct nbd_handle *h) assert (cmd); /* guaranteed by CHECK */ if (cmd->cb.fn.chunk.callback) { int error = cmd->error; - unsigned valid = valid_flags (h); + uint16_t flags = be16toh (h->sbuf.sr.structured_reply.flags); - if (cmd->cb.fn.chunk.callback (valid, cmd->cb.fn.chunk.user_data, + if (cmd->cb.fn.chunk.callback (cmd->cb.fn.chunk.user_data, cmd->data + (offset - cmd->offset), length - sizeof offset, offset, LIBNBD_READ_DATA, &error) == -1) if (cmd->error == 0) cmd->error = error ? error : EPROTO; - if (valid & LIBNBD_CALLBACK_FREE) + if (flags & NBD_REPLY_FLAG_DONE) { + if (cmd->cb.fn.chunk.free) + cmd->cb.fn.chunk.free (cmd->cb.fn.chunk.user_data); cmd->cb.fn.chunk.callback = NULL; /* because we've freed it */ + } } SET_NEXT_STATE (%FINISH); @@ -466,16 +461,19 @@ valid_flags (struct nbd_handle *h) memset (cmd->data + offset, 0, length); if (cmd->cb.fn.chunk.callback) { int error = cmd->error; - unsigned valid = valid_flags (h); + uint16_t flags = be16toh (h->sbuf.sr.structured_reply.flags); - if (cmd->cb.fn.chunk.callback (valid, cmd->cb.fn.chunk.user_data, + if (cmd->cb.fn.chunk.callback (cmd->cb.fn.chunk.user_data, cmd->data + offset, length, cmd->offset + offset, LIBNBD_READ_HOLE, &error) == -1) if (cmd->error == 0) cmd->error = error ? error : EPROTO; - if (valid & LIBNBD_CALLBACK_FREE) + if (flags & NBD_REPLY_FLAG_DONE) { + if (cmd->cb.fn.chunk.free) + cmd->cb.fn.chunk.free (cmd->cb.fn.chunk.user_data); cmd->cb.fn.chunk.callback = NULL; /* because we've freed it */ + } } SET_NEXT_STATE(%FINISH); @@ -521,16 +519,19 @@ valid_flags (struct nbd_handle *h) if (meta_context) { /* Call the caller's extent function. */ int error = cmd->error; - unsigned valid = valid_flags (h); + uint16_t flags = be16toh (h->sbuf.sr.structured_reply.flags); - if (cmd->cb.fn.extent.callback (valid, cmd->cb.fn.extent.user_data, + if (cmd->cb.fn.extent.callback (cmd->cb.fn.extent.user_data, meta_context->name, cmd->offset, &h->bs_entries[1], (length-4) / 4, &error) == -1) if (cmd->error == 0) cmd->error = error ? error : EPROTO; - if (valid & LIBNBD_CALLBACK_FREE) + if (flags & NBD_REPLY_FLAG_DONE) { + if (cmd->cb.fn.extent.free) + cmd->cb.fn.extent.free (cmd->cb.fn.extent.user_data); cmd->cb.fn.extent.callback = NULL; /* because we've freed it */ + } } else /* Emit a debug message, but ignore it. */ @@ -547,14 +548,15 @@ valid_flags (struct nbd_handle *h) flags = be16toh (h->sbuf.sr.structured_reply.flags); if (flags & NBD_REPLY_FLAG_DONE) { - if (cmd->type == NBD_CMD_BLOCK_STATUS && cmd->cb.fn.extent.callback) - cmd->cb.fn.extent.callback (LIBNBD_CALLBACK_FREE, - cmd->cb.fn.extent.user_data, - NULL, 0, NULL, 0, NULL); - if (cmd->type == NBD_CMD_READ && cmd->cb.fn.chunk.callback) - cmd->cb.fn.chunk.callback (LIBNBD_CALLBACK_FREE, - cmd->cb.fn.chunk.user_data, - NULL, 0, 0, 0, NULL); + if (cmd->type == NBD_CMD_BLOCK_STATUS && cmd->cb.fn.extent.callback) { + if (cmd->cb.fn.extent.free) + cmd->cb.fn.extent.free (cmd->cb.fn.extent.user_data); + } + if (cmd->type == NBD_CMD_READ && cmd->cb.fn.chunk.callback) { + if (cmd->cb.fn.chunk.free) + cmd->cb.fn.chunk.free (cmd->cb.fn.chunk.user_data); + } + cmd->cb.fn.extent.callback = NULL; cmd->cb.fn.chunk.callback = NULL; SET_NEXT_STATE (%^FINISH_COMMAND); } diff --git a/generator/states-reply.c b/generator/states-reply.c index e6b479a..d6bd7be 100644 --- a/generator/states-reply.c +++ b/generator/states-reply.c @@ -173,8 +173,9 @@ save_reply_state (struct nbd_handle *h) int r; assert (cmd->type != NBD_CMD_DISC); - r = cmd->cb.completion.callback (LIBNBD_CALLBACK_VALID|LIBNBD_CALLBACK_FREE, - cmd->cb.completion.user_data, &error); + r = cmd->cb.completion.callback (cmd->cb.completion.user_data, &error); + if (cmd->cb.completion.free) + cmd->cb.completion.free (cmd->cb.completion.user_data); cmd->cb.completion.callback = NULL; /* because we've freed it */ switch (r) { case -1: diff --git a/generator/states.c b/generator/states.c index 22b0304..444a082 100644 --- a/generator/states.c +++ b/generator/states.c @@ -126,8 +126,9 @@ void abort_commands (struct nbd_handle *h, int r; assert (cmd->type != NBD_CMD_DISC); - r = cmd->cb.completion.callback (LIBNBD_CALLBACK_VALID|LIBNBD_CALLBACK_FREE, - cmd->cb.completion.user_data, &error); + r = cmd->cb.completion.callback (cmd->cb.completion.user_data, &error); + if (cmd->cb.completion.free) + cmd->cb.completion.free (cmd->cb.completion.user_data); cmd->cb.completion.callback = NULL; /* because we've freed it */ switch (r) { case -1: diff --git a/interop/dirty-bitmap.c b/interop/dirty-bitmap.c index 5a22adc..8077957 100644 --- a/interop/dirty-bitmap.c +++ b/interop/dirty-bitmap.c @@ -42,14 +42,11 @@ struct data { }; static int -cb (unsigned valid_flag, void *opaque, const char *metacontext, uint64_t offset, +cb (void *opaque, const char *metacontext, uint64_t offset, uint32_t *entries, size_t len, int *error) { struct data *data = opaque; - if (!(valid_flag & LIBNBD_CALLBACK_VALID)) - return 0; - /* libnbd does not actually verify that a server is fully compliant * to the spec; the asserts marked [qemu-nbd] are thus dependent on * the fact that qemu-nbd is compliant. diff --git a/interop/structured-read.c b/interop/structured-read.c index 31aadbe..569a56f 100644 --- a/interop/structured-read.c +++ b/interop/structured-read.c @@ -48,16 +48,13 @@ struct data { }; static int -read_cb (unsigned valid_flag, void *opaque, +read_cb (void *opaque, const void *bufv, size_t count, uint64_t offset, unsigned status, int *error) { struct data *data = opaque; const char *buf = bufv; - if (!(valid_flag & LIBNBD_CALLBACK_VALID)) - return 0; - /* The NBD spec allows chunks to be reordered; we are relying on the * fact that qemu-nbd does not do so. */ diff --git a/lib/aio.c b/lib/aio.c index 5017ee6..df493bc 100644 --- a/lib/aio.c +++ b/lib/aio.c @@ -32,18 +32,18 @@ void nbd_internal_retire_and_free_command (struct command *cmd) { /* Free the callbacks. */ - if (cmd->type == NBD_CMD_BLOCK_STATUS && cmd->cb.fn.extent.callback) - cmd->cb.fn.extent.callback (LIBNBD_CALLBACK_FREE, - cmd->cb.fn.extent.user_data, - NULL, 0, NULL, 0, NULL); - if (cmd->type == NBD_CMD_READ && cmd->cb.fn.chunk.callback) - cmd->cb.fn.chunk.callback (LIBNBD_CALLBACK_FREE, - cmd->cb.fn.chunk.user_data, - NULL, 0, 0, 0, NULL); - if (cmd->cb.completion.callback) - cmd->cb.completion.callback (LIBNBD_CALLBACK_FREE, - cmd->cb.completion.user_data, - NULL); + if (cmd->type == NBD_CMD_BLOCK_STATUS && cmd->cb.fn.extent.callback) { + if (cmd->cb.fn.extent.free) + cmd->cb.fn.extent.free (cmd->cb.fn.extent.user_data); + } + if (cmd->type == NBD_CMD_READ && cmd->cb.fn.chunk.callback) { + if (cmd->cb.fn.chunk.free) + cmd->cb.fn.chunk.free (cmd->cb.fn.chunk.user_data); + } + if (cmd->cb.completion.callback) { + if (cmd->cb.completion.free) + cmd->cb.completion.free (cmd->cb.completion.user_data); + } free (cmd); } diff --git a/lib/debug.c b/lib/debug.c index 7753394..1dd6240 100644 --- a/lib/debug.c +++ b/lib/debug.c @@ -42,9 +42,9 @@ int nbd_unlocked_clear_debug_callback (struct nbd_handle *h) { if (h->debug_callback.callback) - /* ignore return value */ - h->debug_callback.callback (LIBNBD_CALLBACK_FREE, - h->debug_callback.user_data, NULL, NULL); + if (h->debug_callback.free) + /* ignore return value */ + h->debug_callback.free (h->debug_callback.user_data); h->debug_callback.callback = NULL; return 0; } @@ -88,8 +88,7 @@ nbd_internal_debug (struct nbd_handle *h, const char *fs, ...) if (h->debug_callback.callback) /* ignore return value */ - h->debug_callback.callback (LIBNBD_CALLBACK_VALID, - h->debug_callback.user_data, context, msg); + h->debug_callback.callback (h->debug_callback.user_data, context, msg); else fprintf (stderr, "libnbd: debug: %s: %s: %s\n", h->hname, context ? : "unknown", msg); diff --git a/tests/closure-lifetimes.c b/tests/closure-lifetimes.c index e21a0e9..41c7f65 100644 --- a/tests/closure-lifetimes.c +++ b/tests/closure-lifetimes.c @@ -38,50 +38,58 @@ static char *nbdkit_delay[] = "delay-read=10", NULL }; -static unsigned debug_fn_valid; -static unsigned debug_fn_free; -static unsigned read_cb_valid; -static unsigned read_cb_free; -static unsigned completion_cb_valid; -static unsigned completion_cb_free; +static unsigned debug_fn_called; +static unsigned debug_fn_freed; +static unsigned read_cb_called; +static unsigned read_cb_freed; +static unsigned completion_cb_called; +static unsigned completion_cb_freed; static int -debug_fn (unsigned valid_flag, void *opaque, - const char *context, const char *msg) +debug_fn (void *opaque, const char *context, const char *msg) { - if (valid_flag & LIBNBD_CALLBACK_VALID) - debug_fn_valid++; - if (valid_flag & LIBNBD_CALLBACK_FREE) - debug_fn_free++; + debug_fn_called++; return 0; } +static void +debug_fn_free (void *opaque) +{ + debug_fn_freed++; +} + static int -read_cb (unsigned valid_flag, void *opaque, +read_cb (void *opaque, const void *subbuf, size_t count, uint64_t offset, unsigned status, int *error) { - assert (read_cb_free == 0); + assert (read_cb_freed == 0); - if (valid_flag & LIBNBD_CALLBACK_VALID) - read_cb_valid++; - if (valid_flag & LIBNBD_CALLBACK_FREE) - read_cb_free++; + read_cb_called++; return 0; } +static void +read_cb_free (void *opaque) +{ + read_cb_freed++; +} + static int -completion_cb (unsigned valid_flag, void *opaque, int *error) +completion_cb (void *opaque, int *error) { - assert (completion_cb_free == 0); + assert (completion_cb_freed == 0); - if (valid_flag & LIBNBD_CALLBACK_VALID) - completion_cb_valid++; - if (valid_flag & LIBNBD_CALLBACK_FREE) - completion_cb_free++; + completion_cb_called++; return 0; } +static void +completion_cb_free (void *opaque) +{ + completion_cb_freed++; +} + #define NBD_ERROR \ do { \ fprintf (stderr, "%s: %s\n", argv[0], nbd_get_error ()); \ @@ -101,15 +109,18 @@ main (int argc, char *argv[]) nbd = nbd_create (); if (nbd == NULL) NBD_ERROR; - nbd_set_debug_callback (nbd, (nbd_debug_callback) { .callback = debug_fn }); - assert (debug_fn_free == 0); + nbd_set_debug_callback (nbd, + (nbd_debug_callback) { .callback = debug_fn, + .free = debug_fn_free }); + assert (debug_fn_freed == 0); - nbd_set_debug_callback (nbd, (nbd_debug_callback) { .callback = debug_fn}); - assert (debug_fn_free == 1); + nbd_set_debug_callback (nbd, (nbd_debug_callback) { .callback = debug_fn, + .free = debug_fn_free }); + assert (debug_fn_freed == 1); - debug_fn_free = 0; + debug_fn_freed = 0; nbd_close (nbd); - assert (debug_fn_free == 1); + assert (debug_fn_freed == 1); /* Test command callbacks are freed when the command is retired. */ nbd = nbd_create (); @@ -117,20 +128,22 @@ main (int argc, char *argv[]) if (nbd_connect_command (nbd, nbdkit) == -1) NBD_ERROR; cookie = nbd_aio_pread_structured (nbd, buf, sizeof buf, 0, - (nbd_chunk_callback) { .callback = read_cb }, - (nbd_completion_callback) { .callback = completion_cb }, + (nbd_chunk_callback) { .callback = read_cb, + .free = read_cb_free }, + (nbd_completion_callback) { .callback = completion_cb, + .free = completion_cb_free }, 0); if (cookie == -1) NBD_ERROR; - assert (read_cb_free == 0); - assert (completion_cb_free == 0); + assert (read_cb_freed == 0); + assert (completion_cb_freed == 0); while (!nbd_aio_command_completed (nbd, cookie)) { if (nbd_poll (nbd, -1) == -1) NBD_ERROR; } - assert (read_cb_valid == 1); - assert (completion_cb_valid == 1); - assert (read_cb_free == 1); - assert (completion_cb_free == 1); + assert (read_cb_called == 1); + assert (completion_cb_called == 1); + assert (read_cb_freed == 1); + assert (completion_cb_freed == 1); nbd_kill_command (nbd, 0); nbd_close (nbd); @@ -138,22 +151,24 @@ main (int argc, char *argv[]) /* Test command callbacks are freed if the handle is closed without * running the commands. */ - read_cb_valid = read_cb_free = - completion_cb_valid = completion_cb_free = 0; + read_cb_called = read_cb_freed = + completion_cb_called = completion_cb_freed = 0; nbd = nbd_create (); if (nbd == NULL) NBD_ERROR; if (nbd_connect_command (nbd, nbdkit_delay) == -1) NBD_ERROR; cookie = nbd_aio_pread_structured (nbd, buf, sizeof buf, 0, - (nbd_chunk_callback) { .callback = read_cb }, - (nbd_completion_callback) { .callback = completion_cb }, + (nbd_chunk_callback) { .callback = read_cb, + .free = read_cb_free }, + (nbd_completion_callback) { .callback = completion_cb, + .free = completion_cb_free }, 0); if (cookie == -1) NBD_ERROR; nbd_kill_command (nbd, 0); nbd_close (nbd); - assert (read_cb_free == 1); - assert (completion_cb_free == 1); + assert (read_cb_freed == 1); + assert (completion_cb_freed == 1); exit (EXIT_SUCCESS); } diff --git a/tests/meta-base-allocation.c b/tests/meta-base-allocation.c index d4e331c..f6be463 100644 --- a/tests/meta-base-allocation.c +++ b/tests/meta-base-allocation.c @@ -30,7 +30,7 @@ #include <libnbd.h> -static int check_extent (unsigned valid_flag, void *data, +static int check_extent (void *data, const char *metacontext, uint64_t offset, uint32_t *entries, size_t nr_entries, int *error); @@ -134,7 +134,7 @@ main (int argc, char *argv[]) } static int -check_extent (unsigned valid_flag, void *data, +check_extent (void *data, const char *metacontext, uint64_t offset, uint32_t *entries, size_t nr_entries, int *error) @@ -142,9 +142,6 @@ check_extent (unsigned valid_flag, void *data, size_t i; int id; - if (!(valid_flag & LIBNBD_CALLBACK_VALID)) - return 0; - id = * (int *)data; printf ("extent: id=%d, metacontext=%s, offset=%" PRIu64 ", " diff --git a/tests/oldstyle.c b/tests/oldstyle.c index ff2ee97..64862b7 100644 --- a/tests/oldstyle.c +++ b/tests/oldstyle.c @@ -37,15 +37,12 @@ static char wbuf[512] = { 1, 2, 3, 4 }, rbuf[512]; static const char *progname; static int -pread_cb (unsigned valid_flag, void *data, +pread_cb (void *data, const void *buf, size_t count, uint64_t offset, unsigned status, int *error) { int *calls; - if (!(valid_flag & LIBNBD_CALLBACK_VALID)) - return 0; - calls = data; ++*calls; diff --git a/tests/server-death.c b/tests/server-death.c index f7684ac..11590ed 100644 --- a/tests/server-death.c +++ b/tests/server-death.c @@ -33,11 +33,8 @@ static bool trim_retired; static const char *progname; static int -callback (unsigned valid_flag, void *ignored, int *error) +callback (void *ignored, int *error) { - if (!(valid_flag & LIBNBD_CALLBACK_VALID)) - return 0; - if (*error != ENOTCONN) { fprintf (stderr, "%s: unexpected error in trim callback: %s\n", progname, strerror (*error)); -- 2.22.0