[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.
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
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> or
Maybe: 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
On Tue, Aug 13, 2019 at 10:06:06PM -0500, Eric Blake wrote:
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.
Sure. What would be the arrangement when all 3 members are present? I guess { callback, user_data, free }?
+++ 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)
OK fixed in my copy. Interesting fact about emacs I-search: It doesn't let you search for double spaces for some reason.
+++ 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.
I was trying to write a generic macro (ie. just ‘NBD_NULL_CALLBACK’) for this, but I don't believe it's possible.
+++ 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?
It's tricky to format these. Even after splitting the line they still go over 7x chars. Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com libguestfs lets you edit virtual machines. Supports shell scripting, bindings from many languages. http://libguestfs.org
On 8/14/19 4:21 AM, Richard W.M. Jones wrote:
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.
Sure. What would be the arrangement when all 3 members are present? I guess { callback, user_data, free }?
Yes, I think that's the most convenient arrangement based on which fields are most likely to be left NULL.
+Some libnbd calls take callbacks (eg. C<nbd_set_debug_callback>,
2 spaces looks odd (emacs thinks eg. ended a sentence)
OK fixed in my copy.
Interesting fact about emacs I-search: It doesn't let you search for double spaces for some reason.
Yeah, emacs intentionally treats all runs of 1 or more spaces in text as a match to single space in I-search. It's often nicer that way, but does sometimes get in the way; Regexp I-search with '[ ]' is my trick for finding an exact space.
+++ 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.
I was trying to write a generic macro (ie. just ‘NBD_NULL_CALLBACK’) for this, but I don't believe it's possible.
Thinking about this more: As written, your patch allows: nbd_set_debug_callback(NBD_NULL_CALLBACK(debug)) to compile, even though it will fail at runtime. The problem? Your macro is too generic. Better would be if we had a macro per type used in OClosure (and no macro for types used only as Closure), where the decision is made at generator time rather than at C compilation time (the presence or absence of a macro then becomes a useful witness of whether that type of closure is optional). Taking this idea further, of our 4 callback types, only one is used as OClosure. For full genericity, we could enhance the generator to check which 'closure' instances are used where, and if used as an OClosure, then rely on String.uppercase_ascii closure.cbname to generate the appropriate C macro. Or, since we know we only have one such type, it's even faster to just hard-code a one-line addition to libnbd.h: #define NBD_NULL_COMPLETION (nbd_completion_callback) { NULL } or maybe #define NBD_NULL_COMPLETION \ (nbd_completion_callback) { .callback = NULL, .user_data = NULL } depending on how likely it is that someone might be using our header with a compiler mode that warns on an initializer that doesn't fully specify everything. Then, whether we go with a generic generator fix, or an open-coded one-liner, the rest of your patch changes via: s/NBD_NULL_CALLBACK(completion)/NBD_NULL_COMPLETION/
@@ -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?
It's tricky to format these. Even after splitting the line they still go over 7x chars.
Would it ease typing if we added a macro: NBD_COMPLETION (finished_read, &buffers[i]) although that gets trickier when we add an optional free callback (C already allows initializers with 1, 2, or 3 members set, but writing a variable-argument macro to do the same gets rather hairy - it can be done with the help of intermediate macros, but that's a lot to cram into libnbd.h). So I guess we just let this form stay longhand. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
On Wed, Aug 14, 2019 at 06:44:37AM -0500, Eric Blake wrote:
#define NBD_NULL_COMPLETION \ (nbd_completion_callback) { .callback = NULL, .user_data = NULL }
depending on how likely it is that someone might be using our header with a compiler mode that warns on an initializer that doesn't fully specify everything.
Then, whether we go with a generic generator fix, or an open-coded one-liner, the rest of your patch changes via:
s/NBD_NULL_CALLBACK(completion)/NBD_NULL_COMPLETION/
OK I'll fix this.
Would it ease typing if we added a macro:
NBD_COMPLETION (finished_read, &buffers[i])
This is not really much different from: (nbd_completion_callback) { finished_read, &buffers[i] } which is fine to use now that I've rearranged the struct fields (in my local copy). Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com Fedora Windows cross-compiler. Compile Windows programs, test, and build Windows installers. Over 100 libraries supported. http://fedoraproject.org/wiki/MinGW
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
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 call
maybe '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
On Wed, Aug 14, 2019 at 08:03:21AM -0500, Eric Blake wrote:
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.
Yes I'll push it rebased and we can fix the free stuff in a later patch. Thanks, Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com Fedora Windows cross-compiler. Compile Windows programs, test, and build Windows installers. Over 100 libraries supported. http://fedoraproject.org/wiki/MinGW
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
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
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
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
participants (2)
-
Eric Blake -
Richard W.M. Jones