Richard W.M. Jones
2019-Aug-13 22:36 UTC
[Libguestfs] [PATCH libnbd 0/4] Add free function to callbacks.
Patches 1 & 2 are rather complex, but the end result is that we pass closures + user_data + free function in single struct parameters as I described previously in this email: https://www.redhat.com/archives/libguestfs/2019-August/msg00210.html Patch 3 adds a convenient FREE_CALLBACK macro which seems a worthwhile simplification if you buy into 1 & 2. Patch 4 adds another macro which is less obviously useful. Rich.
Richard W.M. Jones
2019-Aug-13 22:36 UTC
[Libguestfs] [PATCH libnbd 1/4] api: Combine callback and user_data into a single struct.
The definition of functions that take a callback is changed so that
the callback and user_data are combined into a single structure, eg:
int64_t nbd_aio_pread (struct nbd_handle *h,
void *buf, size_t count, uint64_t offset,
- int (*completion_callback) (/*..*/), void *user_data,
+ nbd_completion_callback completion_callback,
uint32_t flags);
Several nbd_*_callback structures are defined. The one corresponding
to the example above is:
typedef struct {
void *user_data;
int (*callback) (unsigned valid_flag, void *user_data, int *error);
} nbd_completion_callback;
The nbd_aio_pread function can now be called using:
nbd_aio_pread (nbd, buf, sizeof buf, offset,
(nbd_completion_callback) { .callback = my_fn,
.user_data = my_data },
0);
Note that the whole structure is passed by value not by reference.
For OClosure only, a NULL callback can be passed using this macro:
nbd_aio_pread (nbd, buf, sizeof buf, offset,
NBD_NULL_CALLBACK(completion), 0);
---
docs/libnbd.pod | 27 ++++++---
examples/batched-read-write.c | 5 +-
examples/glib-main-loop.c | 6 +-
examples/strict-structured-reads.c | 3 +-
examples/threaded-reads-and-writes.c | 6 +-
generator/generator | 89 ++++++++++++++--------------
generator/states-reply-simple.c | 13 ++--
generator/states-reply-structured.c | 64 ++++++++++----------
generator/states-reply.c | 8 +--
generator/states.c | 8 +--
interop/dirty-bitmap.c | 11 +++-
interop/structured-read.c | 9 ++-
lib/aio.c | 21 ++++---
lib/debug.c | 16 ++---
lib/internal.h | 3 -
lib/rw.c | 60 ++++++++-----------
tests/aio-parallel-load.c | 6 +-
tests/aio-parallel.c | 6 +-
tests/closure-lifetimes.c | 14 +++--
tests/errors.c | 9 ++-
tests/meta-base-allocation.c | 11 +++-
tests/oldstyle.c | 6 +-
tests/server-death.c | 7 ++-
23 files changed, 226 insertions(+), 182 deletions(-)
diff --git a/docs/libnbd.pod b/docs/libnbd.pod
index b38def0..9177825 100644
--- a/docs/libnbd.pod
+++ b/docs/libnbd.pod
@@ -598,14 +598,25 @@ will use your login name):
=head1 CALLBACKS
-Some libnbd calls take function pointers (eg.
-C<nbd_set_debug_callback>, C<nbd_aio_pread>). Libnbd can call
these
-functions while processing.
-
-Callbacks have an opaque C<void *user_data> pointer. This is passed
-as the second parameter to the callback. The opaque pointer is only
-used from the C API, since in other languages you can use closures to
-achieve the same outcome.
+Some libnbd calls take callbacks (eg. C<nbd_set_debug_callback>,
+C<nbd_aio_pread>). Libnbd can call these functions while processing.
+
+In the C API these libnbd calls take a structure which contains the
+function pointer and an optional opaque C<void *user_data> pointer:
+
+ nbd_aio_pread (nbd, buf, sizeof buf, offset,
+ (nbd_completion_callback) { .callback = my_fn,
+ .user_data = my_data },
+ 0);
+
+If you don't want the callback, either set C<.callback> to
C<NULL> or
+use the equivalent macro C<NBD_NULL_CALLBACK()> defined in
C<libnbd.h>:
+
+ nbd_aio_pread (nbd, buf, sizeof buf, offset,
+ NBD_NULL_CALLBACK(completion), 0);
+
+From other languages the structure and opaque pointer are not needed
+because you can use closures to achieve the same effect.
=head2 Callback lifetimes
diff --git a/examples/batched-read-write.c b/examples/batched-read-write.c
index 378c2e0..a6063af 100644
--- a/examples/batched-read-write.c
+++ b/examples/batched-read-write.c
@@ -53,12 +53,13 @@ try_deadlock (void *arg)
/* Issue commands. */
cookies[0] = nbd_aio_pread (nbd, in, packetsize, 0,
- NULL, NULL, 0);
+ NBD_NULL_CALLBACK(completion), 0);
if (cookies[0] == -1) {
fprintf (stderr, "%s\n", nbd_get_error ());
goto error;
}
- cookies[1] = nbd_aio_pwrite (nbd, out, packetsize, packetsize, NULL, NULL,
0);
+ cookies[1] = nbd_aio_pwrite (nbd, out, packetsize, packetsize,
+ NBD_NULL_CALLBACK(completion), 0);
if (cookies[1] == -1) {
fprintf (stderr, "%s\n", nbd_get_error ());
goto error;
diff --git a/examples/glib-main-loop.c b/examples/glib-main-loop.c
index 7b4d215..a8e8ceb 100644
--- a/examples/glib-main-loop.c
+++ b/examples/glib-main-loop.c
@@ -384,7 +384,8 @@ read_data (gpointer user_data)
if (nbd_aio_pread (gssrc->nbd, buffers[i].data,
BUFFER_SIZE, buffers[i].offset,
- finished_read, &buffers[i], 0) == -1) {
+ (nbd_completion_callback) { .callback = finished_read,
.user_data = &buffers[i] },
+ 0) == -1) {
fprintf (stderr, "%s\n", nbd_get_error ());
exit (EXIT_FAILURE);
}
@@ -428,7 +429,8 @@ write_data (gpointer user_data)
buffer->state = BUFFER_WRITING;
if (nbd_aio_pwrite (gsdest->nbd, buffer->data,
BUFFER_SIZE, buffer->offset,
- finished_write, buffer, 0) == -1) {
+ (nbd_completion_callback) { .callback = finished_write,
.user_data = buffer },
+ 0) == -1) {
fprintf (stderr, "%s\n", nbd_get_error ());
exit (EXIT_FAILURE);
}
diff --git a/examples/strict-structured-reads.c
b/examples/strict-structured-reads.c
index 4bc63b8..d7c3e1b 100644
--- a/examples/strict-structured-reads.c
+++ b/examples/strict-structured-reads.c
@@ -236,7 +236,8 @@ main (int argc, char *argv[])
*d = (struct data) { .offset = offset, .count = maxsize, .flags = flags,
.remaining = r, };
if (nbd_aio_pread_structured (nbd, buf, sizeof buf, offset,
- read_chunk, d, read_verify, d,
+ (nbd_chunk_callback) { .callback =
read_chunk, .user_data = d },
+ (nbd_completion_callback) { .callback =
read_verify, .user_data = d },
flags) == -1) {
fprintf (stderr, "%s\n", nbd_get_error ());
exit (EXIT_FAILURE);
diff --git a/examples/threaded-reads-and-writes.c
b/examples/threaded-reads-and-writes.c
index 7626a02..6ae1dd2 100644
--- a/examples/threaded-reads-and-writes.c
+++ b/examples/threaded-reads-and-writes.c
@@ -252,9 +252,11 @@ start_thread (void *arg)
offset = rand () % (exportsize - size);
cmd = rand () & 1;
if (cmd == 0)
- cookie = nbd_aio_pwrite (nbd, buf, size, offset, NULL, NULL, 0);
+ cookie = nbd_aio_pwrite (nbd, buf, size, offset,
+ NBD_NULL_CALLBACK(completion), 0);
else
- cookie = nbd_aio_pread (nbd, buf, size, offset, NULL, NULL, 0);
+ cookie = nbd_aio_pread (nbd, buf, size, offset,
+ NBD_NULL_CALLBACK(completion), 0);
if (cookie == -1) {
fprintf (stderr, "%s\n", nbd_get_error ());
goto error;
diff --git a/generator/generator b/generator/generator
index 4d3d7ad..ea32929 100755
--- a/generator/generator
+++ b/generator/generator
@@ -3226,10 +3226,7 @@ let rec print_arg_list ?(handle = false) ?(types = true)
args optargs pr "%s" len
| Closure { cbname; cbargs } ->
if types then pr "nbd_%s_callback " cbname;
- pr "%s_callback" cbname;
- pr ", ";
- if types then pr "void *";
- pr "%s_user_data" cbname
+ pr "%s_callback" cbname
| Enum (n, _) ->
if types then pr "int ";
pr "%s" n
@@ -3271,10 +3268,7 @@ let rec print_arg_list ?(handle = false) ?(types = true)
args optargs match optarg with
| OClosure { cbname; cbargs } ->
if types then pr "nbd_%s_callback " cbname;
- pr "%s_callback" cbname;
- pr ", ";
- if types then pr "void *";
- pr "%s_user_data" cbname
+ pr "%s_callback" cbname
| OFlags (n, _) ->
if types then pr "uint32_t ";
pr "%s" n
@@ -3337,14 +3331,19 @@ let print_cbarg_list ?(valid_flag = true) ?(types =
true) cbargs ) cbargs;
pr ")"
-(* Callback typedefs in <libnbd.h> *)
-let print_closure_typedefs () +(* Callback structs/typedefs in <libnbd.h>
*)
+let print_closure_structs () List.iter (
fun { cbname; cbargs } ->
- pr "typedef int (*nbd_%s_callback) " cbname;
+ pr "typedef struct {\n";
+ pr " void *user_data;\n";
+ pr " int (*callback) ";
print_cbarg_list cbargs;
pr ";\n";
+ pr "} nbd_%s_callback;\n" cbname;
+ pr "\n";
) all_closures;
+ pr "#define NBD_NULL_CALLBACK(name) ((nbd_## name ##_callback) {
.callback = NULL })\n";
pr "\n"
let print_extern_and_define name args optargs ret @@ -3434,7 +3433,7 @@ let
generate_include_libnbd_h () pr "extern int nbd_get_errno
(void);\n";
pr "#define LIBNBD_HAVE_NBD_GET_ERRNO 1\n";
pr "\n";
- print_closure_typedefs ();
+ print_closure_structs ();
List.iter (
fun (name, { args; optargs; ret }) ->
print_extern_and_define name args optargs ret
@@ -3566,7 +3565,7 @@ let generate_lib_api_c () let value = match
errcode with
| Some value -> value
| None -> assert false in
- pr " if (%s_callback == NULL) {\n" cbname;
+ pr " if (%s_callback.callback == NULL) {\n" cbname;
pr " set_error (EFAULT, \"%%s cannot be NULL\",
\"%s\");\n" cbname;
pr " ret = %s;\n" value;
pr " goto out;\n";
@@ -3678,7 +3677,8 @@ let generate_lib_api_c () ) args;
List.iter (
function
- | OClosure { cbname } -> pr ", %s_callback ?
\"<fun>\" : \"NULL\"" cbname
+ | OClosure { cbname } ->
+ pr ", %s_callback.callback ? \"<fun>\" :
\"NULL\"" cbname
| OFlags (n, _) -> pr ", %s" n
) optargs;
pr ");\n"
@@ -4156,7 +4156,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 " PyObject *%s_user_data;\n" cbname
+ pr " nbd_%s_callback %s = { .callback = %s_wrapper };\n"
+ cbname cbname cbname
| Enum (n, _) -> pr " int %s;\n" n
| Flags (n, _) ->
pr " uint32_t %s_u32;\n" n;
@@ -4188,7 +4189,8 @@ let print_python_binding name { args; optargs; ret;
may_set_error } List.iter (
function
| OClosure { cbname } ->
- pr " PyObject *%s_user_data;\n" cbname
+ pr " nbd_%s_callback %s = { .callback = %s_wrapper };\n"
+ cbname cbname cbname
| OFlags (n, _) ->
pr " uint32_t %s_u32;\n" n;
pr " unsigned int %s; /* really uint32_t */\n" n
@@ -4231,7 +4233,7 @@ let print_python_binding name { args; optargs; ret;
may_set_error } | BytesIn (n, _) | BytesPersistIn (n, _)
| BytesPersistOut (n, _) -> pr ", &%s" n
| BytesOut (_, count) -> pr ", &%s" count
- | Closure { cbname } -> pr ", &%s_user_data" cbname
+ | Closure { cbname } -> pr ", &%s.user_data" cbname
| Enum (n, _) -> pr ", &%s" n
| Flags (n, _) -> pr ", &%s" n
| Int n -> pr ", &%s" n
@@ -4246,7 +4248,7 @@ let print_python_binding name { args; optargs; ret;
may_set_error } ) args;
List.iter (
function
- | OClosure { cbname } -> pr ", &%s_user_data" cbname
+ | OClosure { cbname } -> pr ", &%s.user_data" cbname
| OFlags (n, _) -> pr ", &%s" n
) optargs;
pr "))\n";
@@ -4263,8 +4265,8 @@ let print_python_binding name { args; optargs; ret;
may_set_error } pr " %s_buf = nbd_internal_py_get_aio_buffer
(%s);\n" n n
| Closure { cbname } ->
pr " /* Increment refcount since pointer may be saved by libnbd.
*/\n";
- pr " Py_INCREF (%s_user_data);\n" cbname;
- pr " if (!PyCallable_Check (%s_user_data)) {\n" cbname;
+ pr " Py_INCREF (%s.user_data);\n" cbname;
+ pr " if (!PyCallable_Check (%s.user_data)) {\n" cbname;
pr " PyErr_SetString (PyExc_TypeError,\n";
pr " \"callback parameter %s is not
callable\");\n" cbname;
pr " return NULL;\n";
@@ -4289,15 +4291,17 @@ let print_python_binding name { args; optargs; ret;
may_set_error } List.iter (
function
| OClosure { cbname } ->
- pr " if (%s_user_data != Py_None) {\n" cbname;
+ pr " if (%s.user_data != Py_None) {\n" cbname;
pr " /* Increment refcount since pointer may be saved by libnbd.
*/\n";
- pr " Py_INCREF (%s_user_data);\n" cbname;
- pr " if (!PyCallable_Check (%s_user_data)) {\n" cbname;
+ pr " Py_INCREF (%s.user_data);\n" cbname;
+ pr " if (!PyCallable_Check (%s.user_data)) {\n" cbname;
pr " PyErr_SetString (PyExc_TypeError,\n";
pr " \"callback parameter %s is not
callable\");\n" cbname;
pr " return NULL;\n";
pr " }\n";
- pr " }\n"
+ pr " }\n";
+ pr " else\n";
+ pr " %s.callback = NULL; /* we're not going to call it
*/\n" cbname
| OFlags (n, _) -> pr " %s_u32 = %s;\n" n n
) optargs;
@@ -4310,9 +4314,7 @@ let print_python_binding name { args; optargs; ret;
may_set_error } | BytesOut (n, count) -> pr ", %s, %s" n count
| BytesPersistIn (n, _)
| BytesPersistOut (n, _) -> pr ", %s_buf->data,
%s_buf->len" n n
- | Closure { cbname } ->
- pr ", %s_wrapper" cbname;
- pr ", %s_user_data" cbname
+ | Closure { cbname } -> pr ", %s" cbname
| Enum (n, _) -> pr ", %s" n
| Flags (n, _) -> pr ", %s_u32" n
| Int n -> pr ", %s" n
@@ -4327,9 +4329,7 @@ let print_python_binding name { args; optargs; ret;
may_set_error } ) args;
List.iter (
function
- | OClosure { cbname } ->
- pr ", %s_user_data != Py_None ? %s_wrapper : NULL" cbname
cbname;
- pr ", %s_user_data != Py_None ? %s_user_data : NULL" cbname
cbname
+ | OClosure { cbname } -> pr ", %s" cbname
| OFlags (n, _) -> pr ", %s_u32" n
) optargs;
pr ");\n";
@@ -5125,17 +5125,18 @@ let print_ocaml_binding (name, { args; optargs; ret })
List.iter (
function
| OClosure { cbname } ->
- pr " const void *%s_callback = NULL;\n" cbname;
- pr " value *%s_user_data = NULL;\n" cbname;
+ pr " nbd_%s_callback %s_callback = {0};\n" cbname 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 " %s_user_data = malloc (sizeof (value));\n" cbname;
- pr " if (%s_user_data == NULL) caml_raise_out_of_memory
();\n" cbname;
- pr " *%s_user_data = Field (%sv, 0);\n" cbname cbname;
- pr " caml_register_generational_global_root
(%s_user_data);\n" cbname;
- pr " %s_callback = %s_wrapper;\n" cbname cbname;
+ pr " %s_callback.user_data = malloc (sizeof (value));\n"
cbname;
+ pr " if (%s_callback.user_data == NULL)\n" cbname;
+ pr " caml_raise_out_of_memory ();\n";
+ pr " *(value *)%s_callback.user_data = Field (%sv, 0);\n"
+ cbname cbname;
+ pr " caml_register_generational_global_root
(%s_callback.user_data);\n" cbname;
+ pr " %s_callback.callback = %s_wrapper;\n" cbname cbname;
pr " }\n";
| OFlags (n, { flag_prefix }) ->
pr " uint32_t %s;\n" n;
@@ -5168,12 +5169,14 @@ 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 " value *%s_user_data;\n" cbname;
- pr " %s_user_data = malloc (sizeof (value));\n" cbname;
- pr " if (%s_user_data == NULL) caml_raise_out_of_memory
();\n" cbname;
- pr " *%s_user_data = %sv;\n" cbname cbname;
- pr " caml_register_generational_global_root
(%s_user_data);\n" cbname;
- pr " const void *%s_callback = %s_wrapper;\n" cbname cbname
+ pr " nbd_%s_callback %s_callback = { .callback = %s_wrapper
};\n"
+ 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;
+ pr " *(value *)%s_callback.user_data = %sv;\n" cbname cbname;
+ pr " caml_register_generational_global_root
(%s_callback.user_data);\n"
+ cbname
| Enum (n, { enum_prefix }) ->
pr " int %s = %s_val (%sv);\n" n enum_prefix n
| Flags (n, { flag_prefix }) ->
diff --git a/generator/states-reply-simple.c b/generator/states-reply-simple.c
index 9b249ab..f1d3c62 100644
--- a/generator/states-reply-simple.c
+++ b/generator/states-reply-simple.c
@@ -60,16 +60,17 @@
case 0:
/* guaranteed by START */
assert (cmd);
- if (cmd->cb.fn.chunk) {
+ if (cmd->cb.fn.chunk.callback) {
int error = 0;
assert (cmd->error == 0);
- if (cmd->cb.fn.chunk (LIBNBD_CALLBACK_VALID|LIBNBD_CALLBACK_FREE,
- cmd->cb.fn_user_data,
- cmd->data, cmd->count,
- cmd->offset, LIBNBD_READ_DATA, &error) ==
-1)
+ if (cmd->cb.fn.chunk.callback
(LIBNBD_CALLBACK_VALID|LIBNBD_CALLBACK_FREE,
+ cmd->cb.fn.chunk.user_data,
+ cmd->data, cmd->count,
+ cmd->offset, LIBNBD_READ_DATA,
+ &error) == -1)
cmd->error = error ? error : EPROTO;
- cmd->cb.fn.chunk = NULL; /* because we've freed it */
+ cmd->cb.fn.chunk.callback = NULL; /* because we've freed it */
}
SET_NEXT_STATE (%^FINISH_COMMAND);
diff --git a/generator/states-reply-structured.c
b/generator/states-reply-structured.c
index cdd9f10..92d6b5f 100644
--- a/generator/states-reply-structured.c
+++ b/generator/states-reply-structured.c
@@ -168,7 +168,7 @@ valid_flags (struct nbd_handle *h)
set_error (0, "invalid length in NBD_REPLY_TYPE_BLOCK_STATUS");
return 0;
}
- if (cmd->cb.fn.extent == NULL) {
+ if (cmd->cb.fn.extent.callback == NULL) {
SET_NEXT_STATE (%.DEAD);
set_error (0, "not expecting NBD_REPLY_TYPE_BLOCK_STATUS
here");
return 0;
@@ -304,7 +304,7 @@ valid_flags (struct nbd_handle *h)
offset, cmd->offset, cmd->count);
return 0;
}
- if (cmd->type == NBD_CMD_READ && cmd->cb.fn.chunk) {
+ if (cmd->type == NBD_CMD_READ && cmd->cb.fn.chunk.callback)
{
int scratch = error;
unsigned valid = valid_flags (h);
@@ -312,13 +312,14 @@ valid_flags (struct nbd_handle *h)
* 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 (valid, cmd->cb.fn_user_data,
- cmd->data + (offset - cmd->offset),
- 0, offset, LIBNBD_READ_ERROR, &scratch) ==
-1)
+ if (cmd->cb.fn.chunk.callback (valid, 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)
- cmd->cb.fn.chunk = NULL; /* because we've freed it */
+ cmd->cb.fn.chunk.callback = NULL; /* because we've freed it */
}
}
@@ -398,18 +399,18 @@ valid_flags (struct nbd_handle *h)
offset = be64toh (h->sbuf.sr.payload.offset_data.offset);
assert (cmd); /* guaranteed by CHECK */
- if (cmd->cb.fn.chunk) {
+ if (cmd->cb.fn.chunk.callback) {
int error = cmd->error;
unsigned valid = valid_flags (h);
- if (cmd->cb.fn.chunk (valid, cmd->cb.fn_user_data,
- cmd->data + (offset - cmd->offset),
- length - sizeof offset, offset,
- LIBNBD_READ_DATA, &error) == -1)
+ if (cmd->cb.fn.chunk.callback (valid, 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)
- cmd->cb.fn.chunk = NULL; /* because we've freed it */
+ cmd->cb.fn.chunk.callback = NULL; /* because we've freed it */
}
SET_NEXT_STATE (%FINISH);
@@ -463,18 +464,18 @@ valid_flags (struct nbd_handle *h)
* them as an extension, and this works even when length == 0.
*/
memset (cmd->data + offset, 0, length);
- if (cmd->cb.fn.chunk) {
+ if (cmd->cb.fn.chunk.callback) {
int error = cmd->error;
unsigned valid = valid_flags (h);
- if (cmd->cb.fn.chunk (valid, cmd->cb.fn_user_data,
- cmd->data + offset, length,
- cmd->offset + offset,
- LIBNBD_READ_HOLE, &error) == -1)
+ if (cmd->cb.fn.chunk.callback (valid, 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)
- cmd->cb.fn.chunk = NULL; /* because we've freed it */
+ cmd->cb.fn.chunk.callback = NULL; /* because we've freed it */
}
SET_NEXT_STATE(%FINISH);
@@ -499,7 +500,7 @@ valid_flags (struct nbd_handle *h)
assert (cmd); /* guaranteed by CHECK */
assert (cmd->type == NBD_CMD_BLOCK_STATUS);
- assert (cmd->cb.fn.extent);
+ assert (cmd->cb.fn.extent.callback);
assert (h->bs_entries);
assert (length >= 12);
@@ -522,13 +523,14 @@ valid_flags (struct nbd_handle *h)
int error = cmd->error;
unsigned valid = valid_flags (h);
- if (cmd->cb.fn.extent (valid, cmd->cb.fn_user_data,
- meta_context->name, cmd->offset,
- &h->bs_entries[1], (length-4) / 4,
&error) == -1)
+ if (cmd->cb.fn.extent.callback (valid, 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)
- cmd->cb.fn.extent = NULL; /* because we've freed it */
+ cmd->cb.fn.extent.callback = NULL; /* because we've freed it */
}
else
/* Emit a debug message, but ignore it. */
@@ -545,13 +547,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)
- cmd->cb.fn.extent (LIBNBD_CALLBACK_FREE, cmd->cb.fn_user_data,
- NULL, 0, NULL, 0, NULL);
- if (cmd->type == NBD_CMD_READ && cmd->cb.fn.chunk)
- cmd->cb.fn.chunk (LIBNBD_CALLBACK_FREE, cmd->cb.fn_user_data,
- NULL, 0, 0, 0, NULL);
- cmd->cb.fn.chunk = NULL;
+ 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);
+ cmd->cb.fn.chunk.callback = NULL;
SET_NEXT_STATE (%^FINISH_COMMAND);
}
else {
diff --git a/generator/states-reply.c b/generator/states-reply.c
index 09adfed..e6b479a 100644
--- a/generator/states-reply.c
+++ b/generator/states-reply.c
@@ -168,14 +168,14 @@ save_reply_state (struct nbd_handle *h)
retire = cmd->type == NBD_CMD_DISC;
/* Notify the user */
- if (cmd->cb.completion) {
+ if (cmd->cb.completion.callback) {
int error = cmd->error;
int r;
assert (cmd->type != NBD_CMD_DISC);
- r = cmd->cb.completion (LIBNBD_CALLBACK_VALID|LIBNBD_CALLBACK_FREE,
- cmd->cb.user_data, &error);
- cmd->cb.completion = NULL; /* because we've freed it */
+ r = cmd->cb.completion.callback
(LIBNBD_CALLBACK_VALID|LIBNBD_CALLBACK_FREE,
+ cmd->cb.completion.user_data,
&error);
+ cmd->cb.completion.callback = NULL; /* because we've freed it */
switch (r) {
case -1:
if (error)
diff --git a/generator/states.c b/generator/states.c
index 9ed57ae..22b0304 100644
--- a/generator/states.c
+++ b/generator/states.c
@@ -121,14 +121,14 @@ void abort_commands (struct nbd_handle *h,
bool retire = cmd->type == NBD_CMD_DISC;
next = cmd->next;
- if (cmd->cb.completion) {
+ if (cmd->cb.completion.callback) {
int error = cmd->error ? cmd->error : ENOTCONN;
int r;
assert (cmd->type != NBD_CMD_DISC);
- r = cmd->cb.completion (LIBNBD_CALLBACK_VALID|LIBNBD_CALLBACK_FREE,
- cmd->cb.user_data, &error);
- cmd->cb.completion = NULL; /* because we've freed it */
+ r = cmd->cb.completion.callback
(LIBNBD_CALLBACK_VALID|LIBNBD_CALLBACK_FREE,
+ cmd->cb.completion.user_data,
&error);
+ cmd->cb.completion.callback = NULL; /* because we've freed it */
switch (r) {
case -1:
if (error)
diff --git a/interop/dirty-bitmap.c b/interop/dirty-bitmap.c
index aca0564..5a22adc 100644
--- a/interop/dirty-bitmap.c
+++ b/interop/dirty-bitmap.c
@@ -140,14 +140,17 @@ main (int argc, char *argv[])
}
data = (struct data) { .count = 2, };
- if (nbd_block_status (nbd, exportsize, 0, cb, &data, 0) == -1) {
+ if (nbd_block_status (nbd, exportsize, 0,
+ (nbd_extent_callback) { .callback = cb, .user_data =
&data },
+ 0) == -1) {
fprintf (stderr, "%s\n", nbd_get_error ());
exit (EXIT_FAILURE);
}
assert (data.seen_base && data.seen_dirty);
data = (struct data) { .req_one = true, .count = 2, };
- if (nbd_block_status (nbd, exportsize, 0, cb, &data,
+ if (nbd_block_status (nbd, exportsize, 0,
+ (nbd_extent_callback) { .callback = cb, .user_data =
&data },
LIBNBD_CMD_FLAG_REQ_ONE) == -1) {
fprintf (stderr, "%s\n", nbd_get_error ());
exit (EXIT_FAILURE);
@@ -156,7 +159,9 @@ main (int argc, char *argv[])
/* Trigger a failed callback, to prove connection stays up. */
data = (struct data) { .count = 2, .fail = true, };
- if (nbd_block_status (nbd, exportsize, 0, cb, &data, 0) != -1) {
+ if (nbd_block_status (nbd, exportsize, 0,
+ (nbd_extent_callback) { .callback = cb, .user_data =
&data },
+ 0) != -1) {
fprintf (stderr, "unexpected block status success\n");
exit (EXIT_FAILURE);
}
diff --git a/interop/structured-read.c b/interop/structured-read.c
index 0b189d1..31aadbe 100644
--- a/interop/structured-read.c
+++ b/interop/structured-read.c
@@ -147,7 +147,8 @@ main (int argc, char *argv[])
memset (rbuf, 2, sizeof rbuf);
data = (struct data) { .count = 2, };
- if (nbd_pread_structured (nbd, rbuf, sizeof rbuf, 2048, read_cb, &data,
+ if (nbd_pread_structured (nbd, rbuf, sizeof rbuf, 2048,
+ (nbd_chunk_callback) { .callback = read_cb,
.user_data = &data },
0) == -1) {
fprintf (stderr, "%s\n", nbd_get_error ());
exit (EXIT_FAILURE);
@@ -157,7 +158,8 @@ main (int argc, char *argv[])
/* Repeat with DF flag. */
memset (rbuf, 2, sizeof rbuf);
data = (struct data) { .df = true, .count = 1, };
- if (nbd_pread_structured (nbd, rbuf, sizeof rbuf, 2048, read_cb, &data,
+ if (nbd_pread_structured (nbd, rbuf, sizeof rbuf, 2048,
+ (nbd_chunk_callback) { .callback = read_cb,
.user_data = &data },
LIBNBD_CMD_FLAG_DF) == -1) {
fprintf (stderr, "%s\n", nbd_get_error ());
exit (EXIT_FAILURE);
@@ -170,7 +172,8 @@ main (int argc, char *argv[])
*/
memset (rbuf, 2, sizeof rbuf);
data = (struct data) { .count = 2, .fail = true, };
- if (nbd_pread_structured (nbd, rbuf, sizeof rbuf, 2048, read_cb, &data,
+ if (nbd_pread_structured (nbd, rbuf, sizeof rbuf, 2048,
+ (nbd_chunk_callback) { .callback = read_cb,
.user_data = &data },
0) != -1) {
fprintf (stderr, "unexpected pread callback success\n");
exit (EXIT_FAILURE);
diff --git a/lib/aio.c b/lib/aio.c
index c141de6..5017ee6 100644
--- a/lib/aio.c
+++ b/lib/aio.c
@@ -32,15 +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)
- cmd->cb.fn.extent (LIBNBD_CALLBACK_FREE, cmd->cb.fn_user_data,
- NULL, 0, NULL, 0, NULL);
- if (cmd->type == NBD_CMD_READ && cmd->cb.fn.chunk)
- cmd->cb.fn.chunk (LIBNBD_CALLBACK_FREE, cmd->cb.fn_user_data,
- NULL, 0, 0, 0, NULL);
- if (cmd->cb.completion)
- cmd->cb.completion (LIBNBD_CALLBACK_FREE, cmd->cb.user_data,
- NULL);
+ 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);
free (cmd);
}
diff --git a/lib/debug.c b/lib/debug.c
index c1decb2..7753394 100644
--- a/lib/debug.c
+++ b/lib/debug.c
@@ -41,23 +41,22 @@ nbd_unlocked_get_debug (struct nbd_handle *h)
int
nbd_unlocked_clear_debug_callback (struct nbd_handle *h)
{
- if (h->debug_callback)
+ if (h->debug_callback.callback)
/* ignore return value */
- h->debug_callback (LIBNBD_CALLBACK_FREE, h->debug_data, NULL, NULL);
- h->debug_callback = NULL;
- h->debug_data = NULL;
+ h->debug_callback.callback (LIBNBD_CALLBACK_FREE,
+ h->debug_callback.user_data, NULL, NULL);
+ h->debug_callback.callback = NULL;
return 0;
}
int
nbd_unlocked_set_debug_callback (struct nbd_handle *h,
- nbd_debug_callback debug_callback, void *data)
+ nbd_debug_callback debug_callback)
{
/* This can't fail at the moment - see implementation above. */
nbd_unlocked_clear_debug_callback (h);
h->debug_callback = debug_callback;
- h->debug_data = data;
return 0;
}
@@ -87,9 +86,10 @@ nbd_internal_debug (struct nbd_handle *h, const char *fs,
...)
if (r == -1)
goto out;
- if (h->debug_callback)
+ if (h->debug_callback.callback)
/* ignore return value */
- h->debug_callback (LIBNBD_CALLBACK_VALID, h->debug_data, context,
msg);
+ h->debug_callback.callback (LIBNBD_CALLBACK_VALID,
+ h->debug_callback.user_data, context, msg);
else
fprintf (stderr, "libnbd: debug: %s: %s: %s\n",
h->hname, context ? : "unknown", msg);
diff --git a/lib/internal.h b/lib/internal.h
index 301b798..5996a4f 100644
--- a/lib/internal.h
+++ b/lib/internal.h
@@ -85,7 +85,6 @@ struct nbd_handle {
/* For debugging. */
bool debug;
nbd_debug_callback debug_callback;
- void *debug_data;
/* State machine.
*
@@ -257,9 +256,7 @@ struct command_cb {
nbd_extent_callback extent;
nbd_chunk_callback chunk;
} fn;
- void *fn_user_data; /* associated with one of the fn callbacks above */
nbd_completion_callback completion;
- void *user_data; /* associated with the completion callback */
};
struct command {
diff --git a/lib/rw.c b/lib/rw.c
index dbd4e8c..9881701 100644
--- a/lib/rw.c
+++ b/lib/rw.c
@@ -49,7 +49,8 @@ nbd_unlocked_pread (struct nbd_handle *h, void *buf,
{
int64_t cookie;
- cookie = nbd_unlocked_aio_pread (h, buf, count, offset, NULL, NULL, flags);
+ cookie = nbd_unlocked_aio_pread (h, buf, count, offset,
+ NBD_NULL_CALLBACK(completion), flags);
if (cookie == -1)
return -1;
@@ -60,14 +61,15 @@ nbd_unlocked_pread (struct nbd_handle *h, void *buf,
int
nbd_unlocked_pread_structured (struct nbd_handle *h, void *buf,
size_t count, uint64_t offset,
- nbd_chunk_callback chunk, void *user_data,
+ nbd_chunk_callback chunk,
uint32_t flags)
{
int64_t cookie;
cookie = nbd_unlocked_aio_pread_structured (h, buf, count, offset,
- chunk, user_data,
- NULL, NULL, flags);
+ chunk,
+ NBD_NULL_CALLBACK(completion),
+ flags);
if (cookie == -1)
return -1;
@@ -81,7 +83,8 @@ nbd_unlocked_pwrite (struct nbd_handle *h, const void *buf,
{
int64_t cookie;
- cookie = nbd_unlocked_aio_pwrite (h, buf, count, offset, NULL, NULL, flags);
+ cookie = nbd_unlocked_aio_pwrite (h, buf, count, offset,
+ NBD_NULL_CALLBACK(completion), flags);
if (cookie == -1)
return -1;
@@ -94,7 +97,7 @@ nbd_unlocked_flush (struct nbd_handle *h, uint32_t flags)
{
int64_t cookie;
- cookie = nbd_unlocked_aio_flush (h, NULL, NULL, flags);
+ cookie = nbd_unlocked_aio_flush (h, NBD_NULL_CALLBACK(completion), flags);
if (cookie == -1)
return -1;
@@ -108,7 +111,8 @@ nbd_unlocked_trim (struct nbd_handle *h,
{
int64_t cookie;
- cookie = nbd_unlocked_aio_trim (h, count, offset, NULL, NULL, flags);
+ cookie = nbd_unlocked_aio_trim (h, count, offset,
+ NBD_NULL_CALLBACK(completion), flags);
if (cookie == -1)
return -1;
@@ -122,7 +126,8 @@ nbd_unlocked_cache (struct nbd_handle *h,
{
int64_t cookie;
- cookie = nbd_unlocked_aio_cache (h, count, offset, NULL, NULL, flags);
+ cookie = nbd_unlocked_aio_cache (h, count, offset,
+ NBD_NULL_CALLBACK(completion), flags);
if (cookie == -1)
return -1;
@@ -136,7 +141,8 @@ nbd_unlocked_zero (struct nbd_handle *h,
{
int64_t cookie;
- cookie = nbd_unlocked_aio_zero (h, count, offset, NULL, NULL, flags);
+ cookie = nbd_unlocked_aio_zero (h, count, offset,
+ NBD_NULL_CALLBACK(completion), flags);
if (cookie == -1)
return -1;
@@ -147,13 +153,13 @@ nbd_unlocked_zero (struct nbd_handle *h,
int
nbd_unlocked_block_status (struct nbd_handle *h,
uint64_t count, uint64_t offset,
- nbd_extent_callback extent, void *user_data,
+ nbd_extent_callback extent,
uint32_t flags)
{
int64_t cookie;
- cookie = nbd_unlocked_aio_block_status (h, count, offset, extent, user_data,
- NULL, NULL, flags);
+ cookie = nbd_unlocked_aio_block_status (h, count, offset, extent,
+ NBD_NULL_CALLBACK(completion),
flags);
if (cookie == -1)
return -1;
@@ -257,10 +263,9 @@ int64_t
nbd_unlocked_aio_pread (struct nbd_handle *h, void *buf,
size_t count, uint64_t offset,
nbd_completion_callback completion,
- void *user_data,
uint32_t flags)
{
- struct command_cb cb = { .completion = completion, .user_data = user_data, };
+ struct command_cb cb = { .completion = completion };
/* We could silently accept flag DF, but it really only makes sense
* with callbacks, because otherwise there is no observable change
@@ -279,15 +284,11 @@ int64_t
nbd_unlocked_aio_pread_structured (struct nbd_handle *h, void *buf,
size_t count, uint64_t offset,
nbd_chunk_callback chunk,
- void *read_user_data,
nbd_completion_callback completion,
- void *callback_user_data,
uint32_t flags)
{
struct command_cb cb = { .fn.chunk = chunk,
- .fn_user_data = read_user_data,
- .completion = completion,
- .user_data = callback_user_data, };
+ .completion = completion };
if ((flags & ~LIBNBD_CMD_FLAG_DF) != 0) {
set_error (EINVAL, "invalid flag: %" PRIu32, flags);
@@ -308,10 +309,9 @@ int64_t
nbd_unlocked_aio_pwrite (struct nbd_handle *h, const void *buf,
size_t count, uint64_t offset,
nbd_completion_callback completion,
- void *user_data,
uint32_t flags)
{
- struct command_cb cb = { .completion = completion, .user_data = user_data, };
+ struct command_cb cb = { .completion = completion };
if (nbd_unlocked_is_read_only (h) == 1) {
set_error (EINVAL, "server does not support write operations");
@@ -336,10 +336,9 @@ nbd_unlocked_aio_pwrite (struct nbd_handle *h, const void
*buf,
int64_t
nbd_unlocked_aio_flush (struct nbd_handle *h,
nbd_completion_callback completion,
- void *user_data,
uint32_t flags)
{
- struct command_cb cb = { .completion = completion, .user_data = user_data, };
+ struct command_cb cb = { .completion = completion };
if (nbd_unlocked_can_flush (h) != 1) {
set_error (EINVAL, "server does not support flush operations");
@@ -359,10 +358,9 @@ int64_t
nbd_unlocked_aio_trim (struct nbd_handle *h,
uint64_t count, uint64_t offset,
nbd_completion_callback completion,
- void *user_data,
uint32_t flags)
{
- struct command_cb cb = { .completion = completion, .user_data = user_data, };
+ struct command_cb cb = { .completion = completion };
if (nbd_unlocked_is_read_only (h) == 1) {
set_error (EINVAL, "server does not support write operations");
@@ -393,10 +391,9 @@ int64_t
nbd_unlocked_aio_cache (struct nbd_handle *h,
uint64_t count, uint64_t offset,
nbd_completion_callback completion,
- void *user_data,
uint32_t flags)
{
- struct command_cb cb = { .completion = completion, .user_data = user_data, };
+ struct command_cb cb = { .completion = completion };
/* Actually according to the NBD protocol document, servers do exist
* that support NBD_CMD_CACHE but don't advertise the
@@ -420,10 +417,9 @@ int64_t
nbd_unlocked_aio_zero (struct nbd_handle *h,
uint64_t count, uint64_t offset,
nbd_completion_callback completion,
- void *user_data,
uint32_t flags)
{
- struct command_cb cb = { .completion = completion, .user_data = user_data, };
+ struct command_cb cb = { .completion = completion };
if (nbd_unlocked_is_read_only (h) == 1) {
set_error (EINVAL, "server does not support write operations");
@@ -454,15 +450,11 @@ int64_t
nbd_unlocked_aio_block_status (struct nbd_handle *h,
uint64_t count, uint64_t offset,
nbd_extent_callback extent,
- void *extent_user_data,
nbd_completion_callback completion,
- void *callback_user_data,
uint32_t flags)
{
struct command_cb cb = { .fn.extent = extent,
- .fn_user_data = extent_user_data,
- .completion = completion,
- .user_data = callback_user_data };
+ .completion = completion };
if (!h->structured_replies) {
set_error (ENOTSUP, "server does not support structured
replies");
diff --git a/tests/aio-parallel-load.c b/tests/aio-parallel-load.c
index 1f48324..0d67c36 100644
--- a/tests/aio-parallel-load.c
+++ b/tests/aio-parallel-load.c
@@ -255,11 +255,13 @@ start_thread (void *arg)
offset = rand () % (EXPORTSIZE - buf_size);
cmd = rand () & 1;
if (cmd == 0) {
- cookie = nbd_aio_pwrite (nbd, buf, buf_size, offset, NULL, NULL, 0);
+ cookie = nbd_aio_pwrite (nbd, buf, buf_size, offset,
+ NBD_NULL_CALLBACK(completion), 0);
status->bytes_sent += buf_size;
}
else {
- cookie = nbd_aio_pread (nbd, buf, buf_size, offset, NULL, NULL, 0);
+ cookie = nbd_aio_pread (nbd, buf, buf_size, offset,
+ NBD_NULL_CALLBACK(completion), 0);
status->bytes_received += buf_size;
}
if (cookie == -1) {
diff --git a/tests/aio-parallel.c b/tests/aio-parallel.c
index fb4d695..f6f13e6 100644
--- a/tests/aio-parallel.c
+++ b/tests/aio-parallel.c
@@ -271,12 +271,14 @@ start_thread (void *arg)
+ (rand () % (status->length[i] - BUFFERSIZE));
cmd = rand () & 1;
if (cmd == 0) {
- cookie = nbd_aio_pwrite (nbd, buf, BUFFERSIZE, offset, NULL, NULL, 0);
+ cookie = nbd_aio_pwrite (nbd, buf, BUFFERSIZE, offset,
+ NBD_NULL_CALLBACK(completion), 0);
status->bytes_sent += BUFFERSIZE;
memcpy (&ramdisk[offset], buf, BUFFERSIZE);
}
else {
- cookie = nbd_aio_pread (nbd, buf, BUFFERSIZE, offset, NULL, NULL, 0);
+ cookie = nbd_aio_pread (nbd, buf, BUFFERSIZE, offset,
+ NBD_NULL_CALLBACK(completion), 0);
status->bytes_received += BUFFERSIZE;
}
if (cookie == -1) {
diff --git a/tests/closure-lifetimes.c b/tests/closure-lifetimes.c
index 60809d4..e21a0e9 100644
--- a/tests/closure-lifetimes.c
+++ b/tests/closure-lifetimes.c
@@ -101,10 +101,10 @@ main (int argc, char *argv[])
nbd = nbd_create ();
if (nbd == NULL) NBD_ERROR;
- nbd_set_debug_callback (nbd, debug_fn, NULL);
+ nbd_set_debug_callback (nbd, (nbd_debug_callback) { .callback = debug_fn });
assert (debug_fn_free == 0);
- nbd_set_debug_callback (nbd, debug_fn, NULL);
+ nbd_set_debug_callback (nbd, (nbd_debug_callback) { .callback = debug_fn});
assert (debug_fn_free == 1);
debug_fn_free = 0;
@@ -117,8 +117,9 @@ main (int argc, char *argv[])
if (nbd_connect_command (nbd, nbdkit) == -1) NBD_ERROR;
cookie = nbd_aio_pread_structured (nbd, buf, sizeof buf, 0,
- read_cb, NULL,
- completion_cb, NULL, 0);
+ (nbd_chunk_callback) { .callback = read_cb
},
+ (nbd_completion_callback) { .callback =
completion_cb },
+ 0);
if (cookie == -1) NBD_ERROR;
assert (read_cb_free == 0);
assert (completion_cb_free == 0);
@@ -144,8 +145,9 @@ main (int argc, char *argv[])
if (nbd_connect_command (nbd, nbdkit_delay) == -1) NBD_ERROR;
cookie = nbd_aio_pread_structured (nbd, buf, sizeof buf, 0,
- read_cb, NULL,
- completion_cb, NULL, 0);
+ (nbd_chunk_callback) { .callback = read_cb
},
+ (nbd_completion_callback) { .callback =
completion_cb },
+ 0);
if (cookie == -1) NBD_ERROR;
nbd_kill_command (nbd, 0);
nbd_close (nbd);
diff --git a/tests/errors.c b/tests/errors.c
index e442738..21ec919 100644
--- a/tests/errors.c
+++ b/tests/errors.c
@@ -222,7 +222,8 @@ main (int argc, char *argv[])
exit (EXIT_FAILURE);
}
check (ERANGE, "nbd_pread: ");
- if (nbd_aio_pwrite (nbd, buf, MAXSIZE, 0, NULL, NULL, 0) != -1) {
+ if (nbd_aio_pwrite (nbd, buf, MAXSIZE, 0,
+ NBD_NULL_CALLBACK(completion), 0) != -1) {
fprintf (stderr, "%s: test failed: "
"nbd_aio_pwrite did not fail with oversize request\n",
argv[0]);
@@ -245,11 +246,13 @@ main (int argc, char *argv[])
* command at a time and stalls on the first), then queue multiple
* disconnects.
*/
- if (nbd_aio_pwrite (nbd, buf, 2 * 1024 * 1024, 0, NULL, NULL, 0) == -1) {
+ if (nbd_aio_pwrite (nbd, buf, 2 * 1024 * 1024, 0,
+ NBD_NULL_CALLBACK(completion), 0) == -1) {
fprintf (stderr, "%s: %s\n", argv[0], nbd_get_error ());
exit (EXIT_FAILURE);
}
- if (nbd_aio_pwrite (nbd, buf, 2 * 1024 * 1024, 0, NULL, NULL, 0) == -1) {
+ if (nbd_aio_pwrite (nbd, buf, 2 * 1024 * 1024, 0,
+ NBD_NULL_CALLBACK(completion), 0) == -1) {
fprintf (stderr, "%s: %s\n", argv[0], nbd_get_error ());
exit (EXIT_FAILURE);
}
diff --git a/tests/meta-base-allocation.c b/tests/meta-base-allocation.c
index c36a77f..d4e331c 100644
--- a/tests/meta-base-allocation.c
+++ b/tests/meta-base-allocation.c
@@ -101,19 +101,24 @@ main (int argc, char *argv[])
/* Read the block status. */
id = 1;
- if (nbd_block_status (nbd, 65536, 0, check_extent, &id, 0) == -1) {
+ if (nbd_block_status (nbd, 65536, 0,
+ (nbd_extent_callback) { .callback = check_extent,
.user_data = &id },
+ 0) == -1) {
fprintf (stderr, "%s\n", nbd_get_error ());
exit (EXIT_FAILURE);
}
id = 2;
- if (nbd_block_status (nbd, 1024, 32768-512, check_extent, &id, 0) == -1)
{
+ if (nbd_block_status (nbd, 1024, 32768-512,
+ (nbd_extent_callback) { .callback = check_extent,
.user_data = &id },
+ 0) == -1) {
fprintf (stderr, "%s\n", nbd_get_error ());
exit (EXIT_FAILURE);
}
id = 3;
- if (nbd_block_status (nbd, 1024, 32768-512, check_extent, &id,
+ if (nbd_block_status (nbd, 1024, 32768-512,
+ (nbd_extent_callback) { .callback = check_extent,
.user_data = &id },
LIBNBD_CMD_FLAG_REQ_ONE) == -1) {
fprintf (stderr, "%s\n", nbd_get_error ());
exit (EXIT_FAILURE);
diff --git a/tests/oldstyle.c b/tests/oldstyle.c
index afbda61..ff2ee97 100644
--- a/tests/oldstyle.c
+++ b/tests/oldstyle.c
@@ -131,7 +131,8 @@ main (int argc, char *argv[])
/* Test again for callback operation. */
memset (rbuf, 0, sizeof rbuf);
if (nbd_pread_structured (nbd, rbuf, sizeof rbuf, 2 * sizeof rbuf,
- pread_cb, &calls, 0) == -1) {
+ (nbd_chunk_callback) { .callback = pread_cb,
.user_data = &calls },
+ 0) == -1) {
fprintf (stderr, "%s\n", nbd_get_error ());
exit (EXIT_FAILURE);
}
@@ -147,7 +148,8 @@ main (int argc, char *argv[])
/* Also test that callback errors are reflected correctly. */
if (nbd_pread_structured (nbd, rbuf, sizeof rbuf, 2 * sizeof rbuf,
- pread_cb, &calls, 0) != -1) {
+ (nbd_chunk_callback) { .callback = pread_cb,
.user_data = &calls },
+ 0) != -1) {
fprintf (stderr, "%s: expected failure from callback\n",
argv[0]);
exit (EXIT_FAILURE);
}
diff --git a/tests/server-death.c b/tests/server-death.c
index 1559753..f7684ac 100644
--- a/tests/server-death.c
+++ b/tests/server-death.c
@@ -77,12 +77,15 @@ main (int argc, char *argv[])
/* Issue a read and trim that should not complete yet. Set up the
* trim to auto-retire via callback.
*/
- if ((cookie = nbd_aio_pread (nbd, buf, sizeof buf, 0, NULL, NULL, 0)) == -1)
{
+ if ((cookie = nbd_aio_pread (nbd, buf, sizeof buf, 0,
+ NBD_NULL_CALLBACK(completion), 0)) == -1) {
fprintf (stderr, "%s: test failed: nbd_aio_pread: %s\n", argv[0],
nbd_get_error ());
exit (EXIT_FAILURE);
}
- if (nbd_aio_trim (nbd, sizeof buf, 0, callback, NULL, 0) == -1) {
+ if (nbd_aio_trim (nbd, sizeof buf, 0,
+ (nbd_completion_callback) { .callback = callback },
+ 0) == -1) {
fprintf (stderr, "%s: test failed: nbd_aio_trim: %s\n", argv[0],
nbd_get_error ());
exit (EXIT_FAILURE);
--
2.22.0
Richard W.M. Jones
2019-Aug-13 22:36 UTC
[Libguestfs] [PATCH libnbd 2/4] api: Add free function and remove valid_flag parameter.
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
Richard W.M. Jones
2019-Aug-13 22:37 UTC
[Libguestfs] [PATCH libnbd 3/4] lib: Add FREE_CALLBACK macro.
Simple macro encapsulating the process for freeing a callback.
---
generator/states-reply-simple.c | 4 +--
generator/states-reply-structured.c | 42 +++++++++--------------------
generator/states-reply.c | 4 +--
generator/states.c | 4 +--
lib/aio.c | 17 ++++--------
lib/debug.c | 6 +----
lib/internal.h | 14 ++++++++++
7 files changed, 35 insertions(+), 56 deletions(-)
diff --git a/generator/states-reply-simple.c b/generator/states-reply-simple.c
index 7f2775d..8905367 100644
--- a/generator/states-reply-simple.c
+++ b/generator/states-reply-simple.c
@@ -69,9 +69,7 @@
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 */
+ FREE_CALLBACK (cmd->cb.fn.chunk);
}
SET_NEXT_STATE (%^FINISH_COMMAND);
diff --git a/generator/states-reply-structured.c
b/generator/states-reply-structured.c
index 7c4d63e..62ae3ad 100644
--- a/generator/states-reply-structured.c
+++ b/generator/states-reply-structured.c
@@ -307,11 +307,8 @@
&scratch) == -1)
if (cmd->error == 0)
cmd->error = scratch;
- 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 */
- }
+ if (flags & NBD_REPLY_FLAG_DONE)
+ FREE_CALLBACK (cmd->cb.fn.chunk);
}
}
@@ -401,11 +398,8 @@
LIBNBD_READ_DATA, &error) == -1)
if (cmd->error == 0)
cmd->error = error ? error : EPROTO;
- 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 */
- }
+ if (flags & NBD_REPLY_FLAG_DONE)
+ FREE_CALLBACK (cmd->cb.fn.chunk);
}
SET_NEXT_STATE (%FINISH);
@@ -469,11 +463,8 @@
LIBNBD_READ_HOLE, &error) == -1)
if (cmd->error == 0)
cmd->error = error ? error : EPROTO;
- 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 */
- }
+ if (flags & NBD_REPLY_FLAG_DONE)
+ FREE_CALLBACK (cmd->cb.fn.chunk);
}
SET_NEXT_STATE(%FINISH);
@@ -527,11 +518,8 @@
&error) == -1)
if (cmd->error == 0)
cmd->error = error ? error : EPROTO;
- 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 */
- }
+ if (flags & NBD_REPLY_FLAG_DONE)
+ FREE_CALLBACK (cmd->cb.fn.extent);
}
else
/* Emit a debug message, but ignore it. */
@@ -548,16 +536,10 @@
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) {
- 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;
+ if (cmd->type == NBD_CMD_BLOCK_STATUS)
+ FREE_CALLBACK (cmd->cb.fn.extent);
+ if (cmd->type == NBD_CMD_READ)
+ FREE_CALLBACK (cmd->cb.fn.chunk);
SET_NEXT_STATE (%^FINISH_COMMAND);
}
else {
diff --git a/generator/states-reply.c b/generator/states-reply.c
index d6bd7be..b8bf0ce 100644
--- a/generator/states-reply.c
+++ b/generator/states-reply.c
@@ -174,9 +174,7 @@ save_reply_state (struct nbd_handle *h)
assert (cmd->type != NBD_CMD_DISC);
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 */
+ FREE_CALLBACK (cmd->cb.completion);
switch (r) {
case -1:
if (error)
diff --git a/generator/states.c b/generator/states.c
index 444a082..98c10b3 100644
--- a/generator/states.c
+++ b/generator/states.c
@@ -127,9 +127,7 @@ void abort_commands (struct nbd_handle *h,
assert (cmd->type != NBD_CMD_DISC);
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 */
+ FREE_CALLBACK (cmd->cb.completion);
switch (r) {
case -1:
if (error)
diff --git a/lib/aio.c b/lib/aio.c
index df493bc..38a27d0 100644
--- a/lib/aio.c
+++ b/lib/aio.c
@@ -32,18 +32,11 @@ 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) {
- 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);
- }
+ if (cmd->type == NBD_CMD_BLOCK_STATUS)
+ FREE_CALLBACK (cmd->cb.fn.extent);
+ if (cmd->type == NBD_CMD_READ)
+ FREE_CALLBACK (cmd->cb.fn.chunk);
+ FREE_CALLBACK (cmd->cb.completion);
free (cmd);
}
diff --git a/lib/debug.c b/lib/debug.c
index 1dd6240..e1ec675 100644
--- a/lib/debug.c
+++ b/lib/debug.c
@@ -41,11 +41,7 @@ nbd_unlocked_get_debug (struct nbd_handle *h)
int
nbd_unlocked_clear_debug_callback (struct nbd_handle *h)
{
- if (h->debug_callback.callback)
- if (h->debug_callback.free)
- /* ignore return value */
- h->debug_callback.free (h->debug_callback.user_data);
- h->debug_callback.callback = NULL;
+ FREE_CALLBACK (h->debug_callback);
return 0;
}
diff --git a/lib/internal.h b/lib/internal.h
index 5996a4f..305158e 100644
--- a/lib/internal.h
+++ b/lib/internal.h
@@ -273,6 +273,20 @@ struct command {
uint32_t error; /* Local errno value */
};
+/* 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.
+ */
+#define FREE_CALLBACK(cb) \
+ do { \
+ nbd_completion_callback *_cb = (nbd_completion_callback *)&(cb); \
+ if (_cb->callback != NULL && _cb->free != NULL)
\
+ _cb->free (_cb->user_data); \
+ _cb->callback = NULL; \
+ } while (0)
+
/* aio.c */
extern void nbd_internal_retire_and_free_command (struct command *);
--
2.22.0
Richard W.M. Jones
2019-Aug-13 22:37 UTC
[Libguestfs] [PATCH libnbd 4/4] lib: Add CALL_CALLBACK macro.
Another simple internal macro, this time encapsulating calling a
callback.
---
generator/states-reply-simple.c | 8 ++++----
generator/states-reply-structured.c | 31 ++++++++++++++---------------
generator/states-reply.c | 2 +-
generator/states.c | 2 +-
lib/debug.c | 2 +-
lib/internal.h | 4 ++++
6 files changed, 26 insertions(+), 23 deletions(-)
diff --git a/generator/states-reply-simple.c b/generator/states-reply-simple.c
index 8905367..8e3d7f1 100644
--- a/generator/states-reply-simple.c
+++ b/generator/states-reply-simple.c
@@ -64,10 +64,10 @@
int error = 0;
assert (cmd->error == 0);
- if (cmd->cb.fn.chunk.callback (cmd->cb.fn.chunk.user_data,
- cmd->data, cmd->count,
- cmd->offset, LIBNBD_READ_DATA,
- &error) == -1)
+ if (CALL_CALLBACK (cmd->cb.fn.chunk,
+ cmd->data, cmd->count,
+ cmd->offset, LIBNBD_READ_DATA,
+ &error) == -1)
cmd->error = error ? error : EPROTO;
FREE_CALLBACK (cmd->cb.fn.chunk);
}
diff --git a/generator/states-reply-structured.c
b/generator/states-reply-structured.c
index 62ae3ad..2e327ce 100644
--- a/generator/states-reply-structured.c
+++ b/generator/states-reply-structured.c
@@ -301,10 +301,10 @@
* 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 (cmd->cb.fn.chunk.user_data,
- cmd->data + (offset -
cmd->offset),
- 0, offset, LIBNBD_READ_ERROR,
- &scratch) == -1)
+ if (CALL_CALLBACK (cmd->cb.fn.chunk,
+ cmd->data + (offset - cmd->offset),
+ 0, offset, LIBNBD_READ_ERROR,
+ &scratch) == -1)
if (cmd->error == 0)
cmd->error = scratch;
if (flags & NBD_REPLY_FLAG_DONE)
@@ -392,10 +392,9 @@
int error = cmd->error;
uint16_t flags = be16toh (h->sbuf.sr.structured_reply.flags);
- 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 (CALL_CALLBACK (cmd->cb.fn.chunk, cmd->data + (offset -
cmd->offset),
+ length - sizeof offset, offset,
+ LIBNBD_READ_DATA, &error) == -1)
if (cmd->error == 0)
cmd->error = error ? error : EPROTO;
if (flags & NBD_REPLY_FLAG_DONE)
@@ -457,10 +456,10 @@
int error = cmd->error;
uint16_t flags = be16toh (h->sbuf.sr.structured_reply.flags);
- 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 (CALL_CALLBACK (cmd->cb.fn.chunk,
+ cmd->data + offset, length,
+ cmd->offset + offset,
+ LIBNBD_READ_HOLE, &error) == -1)
if (cmd->error == 0)
cmd->error = error ? error : EPROTO;
if (flags & NBD_REPLY_FLAG_DONE)
@@ -512,10 +511,10 @@
int error = cmd->error;
uint16_t flags = be16toh (h->sbuf.sr.structured_reply.flags);
- 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 (CALL_CALLBACK (cmd->cb.fn.extent,
+ meta_context->name, cmd->offset,
+ &h->bs_entries[1], (length-4) / 4,
+ &error) == -1)
if (cmd->error == 0)
cmd->error = error ? error : EPROTO;
if (flags & NBD_REPLY_FLAG_DONE)
diff --git a/generator/states-reply.c b/generator/states-reply.c
index b8bf0ce..41dcf8d 100644
--- a/generator/states-reply.c
+++ b/generator/states-reply.c
@@ -173,7 +173,7 @@ save_reply_state (struct nbd_handle *h)
int r;
assert (cmd->type != NBD_CMD_DISC);
- r = cmd->cb.completion.callback (cmd->cb.completion.user_data,
&error);
+ r = CALL_CALLBACK (cmd->cb.completion, &error);
FREE_CALLBACK (cmd->cb.completion);
switch (r) {
case -1:
diff --git a/generator/states.c b/generator/states.c
index 98c10b3..263f60e 100644
--- a/generator/states.c
+++ b/generator/states.c
@@ -126,7 +126,7 @@ void abort_commands (struct nbd_handle *h,
int r;
assert (cmd->type != NBD_CMD_DISC);
- r = cmd->cb.completion.callback (cmd->cb.completion.user_data,
&error);
+ r = CALL_CALLBACK (cmd->cb.completion, &error);
FREE_CALLBACK (cmd->cb.completion);
switch (r) {
case -1:
diff --git a/lib/debug.c b/lib/debug.c
index e1ec675..eec2051 100644
--- a/lib/debug.c
+++ b/lib/debug.c
@@ -84,7 +84,7 @@ nbd_internal_debug (struct nbd_handle *h, const char *fs, ...)
if (h->debug_callback.callback)
/* ignore return value */
- h->debug_callback.callback (h->debug_callback.user_data, context,
msg);
+ CALL_CALLBACK (h->debug_callback, context, msg);
else
fprintf (stderr, "libnbd: debug: %s: %s: %s\n",
h->hname, context ? : "unknown", msg);
diff --git a/lib/internal.h b/lib/internal.h
index 305158e..dc3676a 100644
--- a/lib/internal.h
+++ b/lib/internal.h
@@ -273,6 +273,10 @@ struct command {
uint32_t error; /* Local errno value */
};
+/* Call a callback. */
+#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
--
2.22.0
Eric Blake
2019-Aug-14 03:06 UTC
Re: [Libguestfs] [PATCH libnbd 1/4] api: Combine callback and user_data into a single struct.
On 8/13/19 5:36 PM, Richard W.M. Jones wrote:> The definition of functions that take a callback is changed so that > the callback and user_data are combined into a single structure, eg: > > int64_t nbd_aio_pread (struct nbd_handle *h, > void *buf, size_t count, uint64_t offset, > - int (*completion_callback) (/*..*/), void *user_data, > + nbd_completion_callback completion_callback, > uint32_t flags); > > Several nbd_*_callback structures are defined. The one corresponding > to the example above is: > > typedef struct { > void *user_data; > int (*callback) (unsigned valid_flag, void *user_data, int *error); > } nbd_completion_callback; > > The nbd_aio_pread function can now be called using: > > nbd_aio_pread (nbd, buf, sizeof buf, offset, > (nbd_completion_callback) { .callback = my_fn, > .user_data = my_data },Is it worth arranging the C struct to match the typical argument ordering of user_data last? It doesn't make any real difference (the struct size doesn't change, and the compiler handles out-of-order member initialization just fine), but doing it could allow the use of: nbd_completion_callback cb = { my_fn, my_data }; nbd_aio_pread (nbd, buf, sizeof buf, offset, cb, 0); where the omission of .member= designators may result in less typing, but only if the member order matches.> +++ b/docs/libnbd.pod > @@ -598,14 +598,25 @@ will use your login name): > > =head1 CALLBACKS > > -Some libnbd calls take function pointers (eg. > -C<nbd_set_debug_callback>, C<nbd_aio_pread>). Libnbd can call these > -functions while processing. > - > -Callbacks have an opaque C<void *user_data> pointer. This is passed > -as the second parameter to the callback. The opaque pointer is only > -used from the C API, since in other languages you can use closures to > -achieve the same outcome. > +Some libnbd calls take callbacks (eg. C<nbd_set_debug_callback>,2 spaces looks odd (emacs thinks eg. ended a sentence)> +C<nbd_aio_pread>). Libnbd can call these functions while processing. > + > +In the C API these libnbd calls take a structure which contains the > +function pointer and an optional opaque C<void *user_data> pointer: > + > + nbd_aio_pread (nbd, buf, sizeof buf, offset, > + (nbd_completion_callback) { .callback = my_fn, > + .user_data = my_data }, > + 0); > + > +If you don't want the callback, either set C<.callback> to C<NULL> orMaybe: s/If/For optional callbacks, if/ (coupled with the observation made earlier today that we still want a followup patch to better document Closure vs. OClosure on which callbacks do not allow a NULL fn in libnbd-api.3).> +++ b/examples/batched-read-write.c > @@ -53,12 +53,13 @@ try_deadlock (void *arg) > > /* Issue commands. */ > cookies[0] = nbd_aio_pread (nbd, in, packetsize, 0, > - NULL, NULL, 0); > + NBD_NULL_CALLBACK(completion), 0);A bit more verbose, but the macro cuts it down from something even longer to type. I can live with this conversion.> +++ b/examples/glib-main-loop.c > @@ -384,7 +384,8 @@ read_data (gpointer user_data) > > if (nbd_aio_pread (gssrc->nbd, buffers[i].data, > BUFFER_SIZE, buffers[i].offset, > - finished_read, &buffers[i], 0) == -1) { > + (nbd_completion_callback) { .callback = finished_read, .user_data = &buffers[i] }, > + 0) == -1) { > fprintf (stderr, "%s\n", nbd_get_error ()); > exit (EXIT_FAILURE); > } > @@ -428,7 +429,8 @@ write_data (gpointer user_data) > buffer->state = BUFFER_WRITING; > if (nbd_aio_pwrite (gsdest->nbd, buffer->data, > BUFFER_SIZE, buffer->offset, > - finished_write, buffer, 0) == -1) { > + (nbd_completion_callback) { .callback = finished_write, .user_data = buffer },Worth splitting the long lines?> +++ b/interop/structured-read.c > @@ -147,7 +147,8 @@ main (int argc, char *argv[]) > > memset (rbuf, 2, sizeof rbuf); > data = (struct data) { .count = 2, }; > - if (nbd_pread_structured (nbd, rbuf, sizeof rbuf, 2048, read_cb, &data, > + if (nbd_pread_structured (nbd, rbuf, sizeof rbuf, 2048, > + (nbd_chunk_callback) { .callback = read_cb, .user_data = &data }, > 0) == -1) { > fprintf (stderr, "%s\n", nbd_get_error ()); > exit (EXIT_FAILURE); > @@ -157,7 +158,8 @@ main (int argc, char *argv[]) > /* Repeat with DF flag. */ > memset (rbuf, 2, sizeof rbuf); > data = (struct data) { .df = true, .count = 1, }; > - if (nbd_pread_structured (nbd, rbuf, sizeof rbuf, 2048, read_cb, &data, > + if (nbd_pread_structured (nbd, rbuf, sizeof rbuf, 2048, > + (nbd_chunk_callback) { .callback = read_cb, .user_data = &data },When reusing the same (nbd_chunk_callback) more than once, we could hoist it into a helper variable to initialize it only once rather than repeating ourselves. Not essential to this patch (I like that you remained mechanical), but could be done as a later cleanup if desired. ACK -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
Eric Blake
2019-Aug-14 13:03 UTC
Re: [Libguestfs] [PATCH libnbd 2/4] api: Add free function and remove valid_flag parameter.
On 8/13/19 5:36 PM, Richard W.M. Jones wrote:> 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. > ---> +++ 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 callmaybe 'an optional free function'> +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),Needs rebasing based on the tweak to patch 1.> diff --git a/examples/strict-structured-reads.c b/examples/strict-structured-reads.c> 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;Pre-existing, but:> - } > - } > - 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); > - }I half wonder if this cleanup: label should have been part of the FREE flag...> } > + 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); > + }...in which case this loop should be moved into a dedicated free function...> > 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...rather than using free(). But it works as written (mainly because the completion callback is called exactly once), so I'm fine leaving this one as you have it.> @@ -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;Minor rebasing needed here too; I'm assuming that you've already done most of the rebasing work by the time you read this, so I'll quit pointing it out.> +++ 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 */ > + }In the old code, we tried as hard as possible to favor a single callback(VALID|FREE) to reduce the number of callbacks being made. But with a split callback function, I no longer see any advantage to using the free() callback as soon as possible; it may be easier to ONLY call the free() callback when retiring a command. That would simplify multiple spots in this file. However, I could also see keeping this patch as mechanical as possible, and doing that cleanup as a followup.> +++ 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); > + }If we consolidate callback freeing to JUST this function, it cleans up all the other spots in generator/* that were calling .free() and setting .callback=NULL as soon as possible. It also means we are less likely to need patch 3 as a convenience macro to make the pattern of freeing a closure easier to type, because we aren't repeating the pattern.> +++ 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;Here, we could add: assert(!debug_fn_freed)> } > > +static void > +debug_fn_free (void *opaque) > +{ > + debug_fn_freed++; > +}Maybe even here, too (although if we later assert debug_fn_freed == 1, it's the same effect). Overall, I think this is a good patch. I'm okay if you want to push your rebased version now, whether or not you save the consolidation of .free() into just retirement as a followup. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
Eric Blake
2019-Aug-14 13:05 UTC
Re: [Libguestfs] [PATCH libnbd 3/4] lib: Add FREE_CALLBACK macro.
On 8/13/19 5:37 PM, Richard W.M. Jones wrote:> Simple macro encapsulating the process for freeing a callback. > --- > generator/states-reply-simple.c | 4 +-- > generator/states-reply-structured.c | 42 +++++++++-------------------- > generator/states-reply.c | 4 +-- > generator/states.c | 4 +-- > lib/aio.c | 17 ++++-------- > lib/debug.c | 6 +---- > lib/internal.h | 14 ++++++++++ > 7 files changed, 35 insertions(+), 56 deletions(-) >Handy if we keep lots of .free() sites, but less useful if we only .free during retirement (see my commentary in 2).> +++ b/lib/internal.h > @@ -273,6 +273,20 @@ struct command { > uint32_t error; /* Local errno value */ > }; > > +/* 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. > + */ > +#define FREE_CALLBACK(cb) \ > + do { \ > + nbd_completion_callback *_cb = (nbd_completion_callback *)&(cb); \ > + if (_cb->callback != NULL && _cb->free != NULL) \ > + _cb->free (_cb->user_data); \ > + _cb->callback = NULL; \ > + } while (0) > +Some nasty type-punning (probably violates strict C) but I don't see any problem with it working in practice, if we still want to keep it. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
Eric Blake
2019-Aug-14 13:07 UTC
Re: [Libguestfs] [PATCH libnbd 4/4] lib: Add CALL_CALLBACK macro.
On 8/13/19 5:37 PM, Richard W.M. Jones wrote:> Another simple internal macro, this time encapsulating calling a > callback. > --- > generator/states-reply-simple.c | 8 ++++---- > generator/states-reply-structured.c | 31 ++++++++++++++--------------- > generator/states-reply.c | 2 +- > generator/states.c | 2 +- > lib/debug.c | 2 +- > lib/internal.h | 4 ++++ > 6 files changed, 26 insertions(+), 23 deletions(-) > > diff --git a/generator/states-reply-simple.c b/generator/states-reply-simple.c > index 8905367..8e3d7f1 100644 > --- a/generator/states-reply-simple.c > +++ b/generator/states-reply-simple.c > @@ -64,10 +64,10 @@ > int error = 0; > > assert (cmd->error == 0); > - if (cmd->cb.fn.chunk.callback (cmd->cb.fn.chunk.user_data, > - cmd->data, cmd->count, > - cmd->offset, LIBNBD_READ_DATA, > - &error) == -1) > + if (CALL_CALLBACK (cmd->cb.fn.chunk, > + cmd->data, cmd->count, > + cmd->offset, LIBNBD_READ_DATA, > + &error) == -1)Reduces line length, and we have lots of call sites; this one is actually more useful than 3 if we're going for ease of typing of a common pattern.> +++ b/lib/internal.h > @@ -273,6 +273,10 @@ struct command { > uint32_t error; /* Local errno value */ > }; > > +/* Call a callback. */ > +#define CALL_CALLBACK(cb, ...) \ > + (cb).callback ((cb).user_data, ##__VA_ARGS__)And this one doesn't risk any nasty type-punning issues. ACK -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
Possibly Parallel Threads
- [PATCH libnbd 0/7] Add free callbacks and remove valid_flag.
- [PATCH libnbd v3 0/2] lib: Implement closure lifetimes.
- [PATCH libnbd 0/3] Implement closure lifetimes.
- [PATCH libnbd v2 0/5] lib: Implement closure lifetimes.
- [libnbd PATCH 0/2] Fix memory leak with closures