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
Apparently Analagous 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