5:36 p.m.
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