[Libguestfs] [PATCH libnbd 2/3] lib: Implement closure lifetimes.
Richard W.M. Jones
rjones at redhat.com
Wed Jul 24 12:17:50 UTC 2019
Previously closures had a crude flag which tells if they are
persistent or transient. Transient closures (flag = false) last for
the lifetime of the currently called libnbd function. Persistent
closures had an indefinite lifetime which could last for as long as
the handle. In language bindings handling persistent closures was
wasteful as we needed to register a "close callback" to free the
closure when the handle is closed. But if you had submitted thousands
of asynchronous commands you would end up registering thousands of
close callbacks.
We actually have far more information about the lifetime of closures.
We know precisely when they will no longer be needed by the library.
Callers can use this information to free up associated user data
earlier. In particular in language bindings we can remove roots /
decrement reference counts at the right place to free the closure,
without waiting for the handle to be closed.
The solution to this is to introduce the concept of a closure
lifetime. The callback is called with an extra valid_flag parameter
which is a bitmap containing LIBNBD_CALLBACK_VALID and/or
LIBNBD_CALLBACK_FREE. LIBNBD_CALLBACK_VALID corresponds to a normal
call of the callback function by the library. After the library has
finished with the callback (declaring that this callback will never be
needed or called again), it is called once more with
valid_flag == LIBNBD_CALLBACK_FREE.
(Note it is also possible for the library to call the callback with
valid_flag == LIBNBD_CALLBACK_VALID|LIBNBD_CALLBACK_FREE, meaning it's the
last valid call.)
As an example, nbd_set_debug_callback registers a debug closure which
is saved in the handle. The closure is freed either when
nbd_set_debug_callback is called again, or the handle is closed. The
sequence of events would look like this:
Caller Callback
nbd_set_debug_callback
# a new debug_fn is registered
calls any nbd function debug_fn (valid_flag == VALID)
debug_fn (valid_flag == VALID)
debug_fn (valid_flag == VALID)
...
nbd_set_debug_callback
# the old debug_fn is freed
debug_fn (valid_flag == FREE)
# the new debug_fn is called
calls any nbd function debug_fn (valid_flag == VALID)
debug_fn (valid_flag == VALID)
# the second debug_fn is freed
nbd_close debug_fn (valid_flag == FREE)
An example of a debug_fn written by a caller would be:
int my_debug_fn (int 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;
}
---
docs/libnbd.pod | 54 ++++++-
examples/glib-main-loop.c | 16 +-
examples/strict-structured-reads.c | 11 +-
generator/generator | 243 ++++++++++++----------------
generator/states-reply-simple.c | 3 +-
generator/states-reply-structured.c | 8 +-
generator/states-reply.c | 3 +-
generator/states.c | 3 +-
interop/dirty-bitmap.c | 5 +-
interop/structured-read.c | 6 +-
lib/aio.c | 11 ++
lib/debug.c | 7 +-
lib/internal.h | 10 +-
tests/meta-base-allocation.c | 11 +-
tests/oldstyle.c | 10 +-
15 files changed, 233 insertions(+), 168 deletions(-)
diff --git a/docs/libnbd.pod b/docs/libnbd.pod
index 631bb3b..e9a6030 100644
--- a/docs/libnbd.pod
+++ b/docs/libnbd.pod
@@ -487,7 +487,59 @@ C<nbd_set_debug_callback>, C<nbd_pread_callback>). Libnbd can call
these functions while processing.
Callbacks have an opaque C<void *user_data> pointer. This is passed
-as the first parameter to the callback.
+as the second parameter to the callback.
+
+=head2 Callback lifetimes
+
+All callbacks have an C<int valid_flag> parameter which is used to
+help with the lifetime of the callback. C<valid_flag> contains the
+I<logical or> of:
+
+=over 4
+
+=item C<LIBNBD_CALLBACK_VALID>
+
+The callback parameters are valid and this is a normal callback.
+
+=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 (int 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 (int valid_flag, void *user_data,
+ const char *context, const char *msg)
+ {
+ if (!(valid_flag & LIBNBD_CALLBACK_VALID)) return 0;
+
+ // Rest of callback as normal.
+ }
+
+=head2 Callbacks and locking
The callbacks are invoked at a point where the libnbd lock is held; as
such, it is unsafe for the callback to call any C<nbd_*> APIs on the
diff --git a/examples/glib-main-loop.c b/examples/glib-main-loop.c
index c633c1d..9d61923 100644
--- a/examples/glib-main-loop.c
+++ b/examples/glib-main-loop.c
@@ -272,9 +272,11 @@ static GMainLoop *loop;
static void connected (struct NBDSource *source);
static gboolean read_data (gpointer user_data);
-static int finished_read (void *vp, int64_t rcookie, int *error);
+static int finished_read (int valid_flag, void *vp,
+ int64_t rcookie, int *error);
static gboolean write_data (gpointer user_data);
-static int finished_write (void *vp, int64_t wcookie, int *error);
+static int finished_write (int valid_flag, void *vp,
+ int64_t wcookie, int *error);
int
main (int argc, char *argv[])
@@ -395,10 +397,13 @@ read_data (gpointer user_data)
/* This callback is called from libnbd when any read command finishes. */
static int
-finished_read (void *vp, int64_t rcookie, int *error)
+finished_read (int valid_flag, void *vp, int64_t rcookie, int *error)
{
size_t i;
+ if (!(valid_flag & LIBNBD_CALLBACK_VALID))
+ return 0;
+
if (gssrc == NULL)
return 0;
@@ -460,10 +465,13 @@ write_data (gpointer user_data)
/* This callback is called from libnbd when any write command finishes. */
static int
-finished_write (void *vp, int64_t wcookie, int *error)
+finished_write (int valid_flag, void *vp, int64_t wcookie, int *error)
{
size_t i;
+ 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 a50f662..c154b08 100644
--- a/examples/strict-structured-reads.c
+++ b/examples/strict-structured-reads.c
@@ -51,12 +51,16 @@ static int64_t total_bytes;
static int total_success;
static int
-read_chunk (void *opaque, const void *bufv, size_t count, uint64_t offset,
+read_chunk (int valid_flag, void *opaque,
+ const void *bufv, size_t count, uint64_t offset,
int 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);
@@ -127,11 +131,14 @@ read_chunk (void *opaque, const void *bufv, size_t count, uint64_t offset,
}
static int
-read_verify (void *opaque, int64_t cookie, int *error)
+read_verify (int valid_flag, void *opaque, int64_t cookie, int *error)
{
struct data *data = opaque;
int ret = -1;
+ if (!(valid_flag & LIBNBD_CALLBACK_VALID))
+ return 0;
+
total_reads++;
total_chunks += data->chunks;
if (*error)
diff --git a/generator/generator b/generator/generator
index 3b57713..5d5c267 100755
--- a/generator/generator
+++ b/generator/generator
@@ -849,10 +849,7 @@ and arg =
written by the function *)
| BytesPersistIn of string * string (* same as above, but buffer persists *)
| BytesPersistOut of string * string
-| Closure of bool * closure (* function pointer + void *opaque
- flag if true means callbacks persist
- in the handle, false means they only
- exist during the function call *)
+| Closure of closure (* function pointer + void *opaque *)
| Flags of string (* NBD_CMD_FLAG_* flags *)
| Int of string (* small int *)
| Int64 of string (* 64 bit signed int *)
@@ -920,9 +917,8 @@ Return the state of the debug flag on this handle.";
"set_debug_callback", {
default_call with
- args = [ Closure (true,
- { cbname="debug_fn";
- cbargs=[String "context"; String "msg"] }) ];
+ args = [ Closure { cbname="debug_fn";
+ cbargs=[String "context"; String "msg"] } ];
ret = RErr;
shortdesc = "set the debug callback";
longdesc = "\
@@ -1350,11 +1346,10 @@ protocol extensions).";
"pread_structured", {
default_call with
args = [ BytesOut ("buf", "count"); UInt64 "offset";
- Closure (false,
- { cbname="chunk";
- cbargs=[BytesIn ("subbuf", "count");
- UInt64 "offset"; Int "status";
- Mutable (Int "error")] });
+ Closure { cbname="chunk";
+ cbargs=[BytesIn ("subbuf", "count");
+ UInt64 "offset"; Int "status";
+ Mutable (Int "error")] };
Flags "flags" ];
ret = RErr;
permitted_states = [ Connected ];
@@ -1541,13 +1536,12 @@ punching a hole.";
"block_status", {
default_call with
args = [ UInt64 "count"; UInt64 "offset";
- Closure (false,
- { cbname="extent";
- cbargs=[String "metacontext";
- UInt64 "offset";
- ArrayAndLen (UInt32 "entries",
- "nr_entries");
- Mutable (Int "error")]} );
+ Closure { cbname="extent";
+ cbargs=[String "metacontext";
+ UInt64 "offset";
+ ArrayAndLen (UInt32 "entries",
+ "nr_entries");
+ Mutable (Int "error")]};
Flags "flags" ];
ret = RErr;
permitted_states = [ Connected ];
@@ -1725,9 +1719,8 @@ C<nbd_pread>.";
"aio_pread_callback", {
default_call with
args = [ BytesPersistOut ("buf", "count"); UInt64 "offset";
- Closure (true,
- { cbname="callback";
- cbargs=[Int64 "cookie"; Mutable (Int "error")] } );
+ Closure { cbname="callback";
+ cbargs=[Int64 "cookie"; Mutable (Int "error")] };
Flags "flags" ];
ret = RInt64;
permitted_states = [ Connected ];
@@ -1755,12 +1748,11 @@ cause a deadlock.";
"aio_pread_structured", {
default_call with
args = [ BytesPersistOut ("buf", "count"); UInt64 "offset";
- Closure (true,
- { cbname="chunk";
- cbargs=[BytesIn ("subbuf", "count");
- UInt64 "offset";
- Int "status";
- Mutable (Int "error");]} );
+ Closure { cbname="chunk";
+ cbargs=[BytesIn ("subbuf", "count");
+ UInt64 "offset";
+ Int "status";
+ Mutable (Int "error");]};
Flags "flags" ];
ret = RInt64;
permitted_states = [ Connected ];
@@ -1777,16 +1769,14 @@ documented in C<nbd_pread_structured>.";
"aio_pread_structured_callback", {
default_call with
args = [ BytesPersistOut ("buf", "count"); UInt64 "offset";
- Closure (true,
- { cbname="chunk";
- cbargs=[BytesIn ("subbuf", "count");
- UInt64 "offset";
- Int "status";
- Mutable (Int "error"); ]});
- Closure (true,
- { cbname="callback";
- cbargs=[Int64 "cookie";
- Mutable (Int "error"); ]} );
+ Closure { cbname="chunk";
+ cbargs=[BytesIn ("subbuf", "count");
+ UInt64 "offset";
+ Int "status";
+ Mutable (Int "error"); ]};
+ Closure { cbname="callback";
+ cbargs=[Int64 "cookie";
+ Mutable (Int "error"); ]};
Flags "flags" ];
ret = RInt64;
permitted_states = [ Connected ];
@@ -1829,9 +1819,8 @@ C<nbd_pwrite>.";
"aio_pwrite_callback", {
default_call with
args = [ BytesPersistIn ("buf", "count"); UInt64 "offset";
- Closure (true,
- { cbname="callback";
- cbargs=[Int64 "cookie"; Mutable (Int "error")]} );
+ Closure { cbname="callback";
+ cbargs=[Int64 "cookie"; Mutable (Int "error")]};
Flags "flags" ];
ret = RInt64;
permitted_states = [ Connected ];
@@ -1894,9 +1883,8 @@ Parameters behave as documented in C<nbd_flush>.";
"aio_flush_callback", {
default_call with
- args = [ Closure (true,
- { cbname="callback";
- cbargs=[Int64 "cookie"; Mutable (Int "error")]} );
+ args = [ Closure { cbname="callback";
+ cbargs=[Int64 "cookie"; Mutable (Int "error")]};
Flags "flags" ];
ret = RInt64;
permitted_states = [ Connected ];
@@ -1937,9 +1925,8 @@ Parameters behave as documented in C<nbd_trim>.";
"aio_trim_callback", {
default_call with
args = [ UInt64 "count"; UInt64 "offset";
- Closure (true,
- { cbname="callback";
- cbargs=[Int64 "cookie"; Mutable (Int "error")]} );
+ Closure { cbname="callback";
+ cbargs=[Int64 "cookie"; Mutable (Int "error")]};
Flags "flags" ];
ret = RInt64;
permitted_states = [ Connected ];
@@ -1980,9 +1967,8 @@ Parameters behave as documented in C<nbd_cache>.";
"aio_cache_callback", {
default_call with
args = [ UInt64 "count"; UInt64 "offset";
- Closure (true,
- { cbname="callback";
- cbargs=[Int64 "cookie"; Mutable (Int "error")]} );
+ Closure { cbname="callback";
+ cbargs=[Int64 "cookie"; Mutable (Int "error")]};
Flags "flags" ];
ret = RInt64;
permitted_states = [ Connected ];
@@ -2023,9 +2009,8 @@ Parameters behave as documented in C<nbd_zero>.";
"aio_zero_callback", {
default_call with
args = [ UInt64 "count"; UInt64 "offset";
- Closure (true,
- { cbname="callback";
- cbargs=[Int64 "cookie"; Mutable (Int "error")]} );
+ Closure { cbname="callback";
+ cbargs=[Int64 "cookie"; Mutable (Int "error")]};
Flags "flags" ];
ret = RInt64;
permitted_states = [ Connected ];
@@ -2052,12 +2037,11 @@ cause a deadlock.";
"aio_block_status", {
default_call with
args = [ UInt64 "count"; UInt64 "offset";
- Closure (true,
- { cbname="extent";
- cbargs=[String "metacontext"; UInt64 "offset";
- ArrayAndLen (UInt32 "entries",
- "nr_entries");
- Mutable (Int "error")] } );
+ Closure { cbname="extent";
+ cbargs=[String "metacontext"; UInt64 "offset";
+ ArrayAndLen (UInt32 "entries",
+ "nr_entries");
+ Mutable (Int "error")] };
Flags "flags" ];
ret = RInt64;
permitted_states = [ Connected ];
@@ -2073,15 +2057,13 @@ Parameters behave as documented in C<nbd_block_status>.";
"aio_block_status_callback", {
default_call with
args = [ UInt64 "count"; UInt64 "offset";
- Closure (true,
- { cbname="extent";
- cbargs=[String "metacontext"; UInt64 "offset";
- ArrayAndLen (UInt32 "entries",
- "nr_entries");
- Mutable (Int "error")]});
- Closure (true,
- { cbname="callback";
- cbargs=[Int64 "cookie"; Mutable (Int "error")]} );
+ Closure { cbname="extent";
+ cbargs=[String "metacontext"; UInt64 "offset";
+ ArrayAndLen (UInt32 "entries",
+ "nr_entries");
+ Mutable (Int "error")]};
+ Closure { cbname="callback";
+ cbargs=[Int64 "cookie"; Mutable (Int "error")]};
Flags "flags" ];
ret = RInt64;
permitted_states = [ Connected ];
@@ -3121,7 +3103,8 @@ module C : sig
val generate_lib_unlocked_h : unit -> unit
val generate_lib_api_c : unit -> unit
val generate_docs_libnbd_api_pod : unit -> unit
- val print_arg_list : ?handle:bool -> ?user_data:bool -> ?types:bool -> arg list -> unit
+ val print_arg_list : ?handle:bool -> ?valid_flag:bool -> ?user_data:bool ->
+ ?types:bool -> arg list -> unit
end = struct
(* Check the API definition. *)
@@ -3174,7 +3157,7 @@ let rec name_of_arg = function
| BytesOut (n, len) -> [n; len]
| BytesPersistIn (n, len) -> [n; len]
| BytesPersistOut (n, len) -> [n; len]
-| Closure (_, { cbname }) -> [cbname; sprintf "%s_user_data" cbname ]
+| Closure { cbname } -> [cbname; sprintf "%s_user_data" cbname ]
| Flags n -> [n]
| Int n -> [n]
| Int64 n -> [n]
@@ -3187,7 +3170,8 @@ let rec name_of_arg = function
| UInt32 n -> [n]
| UInt64 n -> [n]
-let rec print_arg_list ?(handle = false) ?(user_data = false)
+let rec print_arg_list ?(handle = false) ?(valid_flag = false)
+ ?(user_data = false)
?(types = true) args =
pr "(";
let comma = ref false in
@@ -3196,6 +3180,12 @@ let rec print_arg_list ?(handle = false) ?(user_data = false)
if types then pr "struct nbd_handle *";
pr "h"
);
+ if valid_flag then (
+ if !comma then pr ", ";
+ comma := true;
+ if types then pr "int ";
+ pr "valid_flag";
+ );
if user_data then (
if !comma then pr ", ";
comma := true;
@@ -3228,10 +3218,10 @@ let rec print_arg_list ?(handle = false) ?(user_data = false)
pr "%s, " n;
if types then pr "size_t ";
pr "%s" len
- | Closure (_, { cbname; cbargs }) ->
+ | Closure { cbname; cbargs } ->
if types then (
pr "int (*%s) " cbname;
- print_arg_list ~user_data:true cbargs;
+ print_arg_list ~valid_flag:true ~user_data:true cbargs;
)
else
pr "%s" cbname;
@@ -3334,6 +3324,9 @@ let generate_include_libnbd_h () =
pr "\n";
List.iter (fun (n, i) -> pr "#define LIBNBD_%-30s %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";
@@ -3795,27 +3788,14 @@ let print_python_binding name { args; ret } =
*)
List.iter (
function
- | Closure (persistent, { cbname; cbargs }) ->
- (* Persistent closures need an explicit function to decrement
- * the closure refcounts and free the user_data struct.
- *)
- if persistent then (
- pr "static void\n";
- pr "free_%s_%s_user_data (void *vp)\n" name cbname;
- pr "{\n";
- pr " PyObject *user_data = vp;\n";
- pr "\n";
- pr " Py_DECREF (user_data);\n";
- pr "}\n";
- pr "\n";
- );
-
+ | Closure { cbname; cbargs } ->
pr "/* Wrapper for %s callback of %s. */\n" cbname name;
pr "static int\n";
pr "%s_%s_wrapper " name cbname;
- C.print_arg_list ~user_data:true cbargs;
+ C.print_arg_list ~valid_flag:true ~user_data:true cbargs;
pr "\n";
pr "{\n";
+ pr " if (valid_flag & LIBNBD_CALLBACK_VALID) {\n";
pr " int ret;\n";
pr " PyGILState_STATE py_save = PyGILState_UNLOCKED;\n";
pr " PyObject *py_args, *py_ret;\n";
@@ -3927,6 +3907,12 @@ let print_python_binding name { args; ret } =
| UInt _ | UInt32 _ -> assert false
) cbargs;
pr " return ret;\n";
+ pr " }\n";
+ pr "\n";
+ pr " if (valid_flag & LIBNBD_CALLBACK_FREE)\n";
+ pr " Py_DECREF ((PyObject *)user_data);\n";
+ pr "\n";
+ pr " return 0;\n";
pr "}\n";
pr "\n"
| _ -> ()
@@ -3966,7 +3952,7 @@ let print_python_binding name { args; ret } =
pr " PyObject *%s; /* PyCapsule pointing to struct py_aio_buffer */\n"
n;
pr " struct py_aio_buffer *%s_buf;\n" n
- | Closure (_, { cbname }) ->
+ | Closure { cbname } ->
pr " PyObject *%s_user_data;\n" cbname
| Flags n ->
pr " uint32_t %s_u32;\n" n;
@@ -4032,7 +4018,7 @@ let print_python_binding name { args; ret } =
| 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
| Flags n -> pr ", &%s" n
| Int n -> pr ", &%s" n
| Int64 n -> pr ", &%s" n
@@ -4048,13 +4034,12 @@ let print_python_binding name { args; ret } =
pr "))\n";
pr " return NULL;\n";
- (* If the parameter has persistent closures then we need to
+ (* If the parameter has closures then we need to
* make sure the ref count remains positive.
*)
List.iter (
function
- | Closure (false, _) -> ()
- | Closure (true, { cbname }) ->
+ | Closure { cbname } ->
pr " Py_INCREF (%s_user_data);\n" cbname
| _ -> ()
) args;
@@ -4086,7 +4071,7 @@ let print_python_binding name { args; ret } =
pr " %s = malloc (%s);\n" n count
| BytesPersistIn (n, _) | BytesPersistOut (n, _) ->
pr " %s_buf = nbd_internal_py_get_aio_buffer (%s);\n" n n
- | Closure (_, { cbname }) ->
+ | Closure { 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;
@@ -4122,7 +4107,7 @@ let print_python_binding name { args; ret } =
| BytesOut (n, count) -> pr ", %s, %s" n count
| BytesPersistIn (n, _)
| BytesPersistOut (n, _) -> pr ", %s_buf->data, %s_buf->len" n n
- | Closure (_, { cbname }) ->
+ | Closure { cbname } ->
pr ", %s_%s_wrapper" name cbname;
pr ", %s_user_data" cbname
| Flags n -> pr ", %s_u32" n
@@ -4200,11 +4185,7 @@ let print_python_binding name { args; ret } =
| Bool _ -> ()
| BytesIn (n, _) -> pr " PyBuffer_Release (&%s);\n" n
| BytesPersistIn _ | BytesOut _ | BytesPersistOut _ -> ()
- | Closure (false, _) -> ()
- | Closure (true, { cbname }) ->
- pr " /* This ensures the callback data is freed eventually. */\n";
- pr " nbd_add_close_callback (h, free_%s_%s_user_data, %s_user_data);\n"
- name cbname cbname
+ | Closure _ -> ()
| Flags _ -> ()
| Int _ -> ()
| Int64 _ -> ()
@@ -4351,7 +4332,7 @@ class NBD (object):
| BytesIn (n, _) | BytesPersistIn (n, _) -> [n, None]
| BytesPersistOut (n, _) -> [n, None]
| BytesOut (_, count) -> [count, None]
- | Closure (_, { cbname }) -> [cbname, None]
+ | Closure { cbname } -> [cbname, None]
| Flags n -> [n, Some "0"]
| Int n -> [n, None]
| Int64 n -> [n, None]
@@ -4424,10 +4405,7 @@ let args_to_ocaml_args args =
| Flags n :: rest -> Some (OCamlFlags n), List.rev rest
| _ -> None, args in
let args =
- List.map (
- function
- | a -> [OCamlArg a]
- ) args in
+ List.map (fun a -> [OCamlArg a]) args in
let args = List.flatten args in
match flags with
| Some f -> f :: OCamlHandle :: args
@@ -4450,7 +4428,7 @@ and ocaml_arg_to_string = function
| OCamlArg (BytesPersistIn _) -> "Buffer.t"
| OCamlArg (BytesOut _) -> "bytes"
| OCamlArg (BytesPersistOut _) -> "Buffer.t"
- | OCamlArg (Closure (_, { cbargs })) ->
+ | OCamlArg (Closure { cbargs }) ->
sprintf "(%s)"
(ocaml_fundecl_to_string (List.map (fun a -> OCamlArg a) cbargs)
RErr)
@@ -4486,7 +4464,7 @@ let rec name_of_ocaml_arg = function
| BytesOut (n, len) -> n
| BytesPersistIn (n, len) -> n
| BytesPersistOut (n, len) -> n
- | Closure (_, { cbname }) -> cbname
+ | Closure { cbname } -> cbname
| Flags n -> n
| Int n -> n
| Int64 n -> n
@@ -4644,22 +4622,7 @@ let print_ocaml_binding (name, { args; ret }) =
(* Functions with a callback parameter require special handling. *)
List.iter (
function
- | Closure (persistent, { cbname; cbargs }) ->
- (* Persistent closures need an explicit function to remove
- * the global root and free the user_data struct.
- *)
- if persistent then (
- pr "static void\n";
- pr "free_%s_%s_user_data (void *vp)\n" name cbname;
- pr "{\n";
- pr " value *user_data = vp;\n";
- pr "\n";
- pr " caml_remove_generational_global_root (user_data);\n";
- pr " free (user_data);\n";
- pr "}\n";
- pr "\n"
- );
-
+ | Closure { cbname; cbargs } ->
let argnames =
List.map (
function
@@ -4762,16 +4725,24 @@ let print_ocaml_binding (name, { args; ret }) =
pr "\n";
pr "static int\n";
pr "%s_%s_wrapper " name cbname;
- C.print_arg_list ~user_data:true cbargs;
+ C.print_arg_list ~valid_flag:true ~user_data:true cbargs;
pr "\n";
pr "{\n";
- pr " int ret;\n";
+ pr " int ret = 0;\n";
pr "\n";
- pr " caml_leave_blocking_section ();\n";
- pr " ret = %s_%s_wrapper_locked " name cbname;
+ pr " if (valid_flag & LIBNBD_CALLBACK_VALID) {\n";
+ pr " caml_leave_blocking_section ();\n";
+ pr " ret = %s_%s_wrapper_locked " name cbname;
C.print_arg_list ~user_data:true ~types:false cbargs;
pr ";\n";
- pr " caml_enter_blocking_section ();\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 "\n"
@@ -4846,15 +4817,7 @@ let print_ocaml_binding (name, { args; ret }) =
pr " struct nbd_buffer %s_buf = NBD_buffer_val (%sv);\n" n n;
pr " void *%s = %s_buf.data;\n" n n;
pr " size_t %s = %s_buf.len;\n" count n
- | OCamlArg (Closure (false, { cbname })) ->
- pr " /* This is safe because we only call this while this stack\n";
- pr " * frame is live.\n";
- pr " */\n";
- pr " CAMLlocal1 (_%s_user_data);\n" cbname;
- pr " value *%s_user_data = &_%s_user_data;\n" cbname cbname;
- pr " _%s_user_data = %sv;\n" cbname cbname;
- pr " const void *%s = %s_%s_wrapper;\n" cbname name cbname
- | OCamlArg (Closure (true, { cbname })) ->
+ | OCamlArg (Closure { 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";
@@ -4863,9 +4826,7 @@ let print_ocaml_binding (name, { args; ret }) =
pr " if (%s_user_data == NULL) caml_raise_out_of_memory ();\n" cbname;
pr " caml_register_generational_global_root (%s_user_data);\n" cbname;
pr " *%s_user_data = %sv;\n" cbname cbname;
- pr " const void *%s = %s_%s_wrapper;\n" cbname name cbname;
- pr " nbd_add_close_callback (h, free_%s_%s_user_data, %s_user_data);\n"
- name cbname cbname
+ pr " const void *%s = %s_%s_wrapper;\n" cbname name cbname
| OCamlArg (Flags _) -> assert false (* see above *)
| OCamlArg (Int n) ->
pr " int %s = Int_val (%sv);\n" n n
diff --git a/generator/states-reply-simple.c b/generator/states-reply-simple.c
index 94875aa..d6b2e2c 100644
--- a/generator/states-reply-simple.c
+++ b/generator/states-reply-simple.c
@@ -64,7 +64,8 @@
int error = 0;
assert (cmd->error == 0);
- if (cmd->cb.fn.read (cmd->cb.fn_user_data, cmd->data, cmd->count,
+ if (cmd->cb.fn.read (LIBNBD_CALLBACK_VALID, cmd->cb.fn_user_data,
+ cmd->data, cmd->count,
cmd->offset, LIBNBD_READ_DATA, &error) == -1)
cmd->error = error ? error : EPROTO;
}
diff --git a/generator/states-reply-structured.c b/generator/states-reply-structured.c
index f60232e..2ef8d20 100644
--- a/generator/states-reply-structured.c
+++ b/generator/states-reply-structured.c
@@ -298,7 +298,7 @@
* 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.read (cmd->cb.fn_user_data,
+ if (cmd->cb.fn.read (LIBNBD_CALLBACK_VALID, cmd->cb.fn_user_data,
cmd->data + (offset - cmd->offset),
0, offset, LIBNBD_READ_ERROR, &scratch) == -1)
if (cmd->error == 0)
@@ -385,7 +385,7 @@
if (cmd->cb.fn.read) {
int error = cmd->error;
- if (cmd->cb.fn.read (cmd->cb.fn_user_data,
+ if (cmd->cb.fn.read (LIBNBD_CALLBACK_VALID, cmd->cb.fn_user_data,
cmd->data + (offset - cmd->offset),
length - sizeof offset, offset,
LIBNBD_READ_DATA, &error) == -1)
@@ -447,7 +447,7 @@
if (cmd->cb.fn.read) {
int error = cmd->error;
- if (cmd->cb.fn.read (cmd->cb.fn_user_data,
+ if (cmd->cb.fn.read (LIBNBD_CALLBACK_VALID, cmd->cb.fn_user_data,
cmd->data + offset, length,
cmd->offset + offset,
LIBNBD_READ_HOLE, &error) == -1)
@@ -499,7 +499,7 @@
/* Call the caller's extent function. */
int error = cmd->error;
- if (cmd->cb.fn.extent (cmd->cb.fn_user_data,
+ if (cmd->cb.fn.extent (LIBNBD_CALLBACK_VALID, cmd->cb.fn_user_data,
meta_context->name, cmd->offset,
&h->bs_entries[1], (length-4) / 4, &error) == -1)
if (cmd->error == 0)
diff --git a/generator/states-reply.c b/generator/states-reply.c
index 1a0c149..b11479c 100644
--- a/generator/states-reply.c
+++ b/generator/states-reply.c
@@ -184,7 +184,8 @@ save_reply_state (struct nbd_handle *h)
if (cmd->cb.callback) {
int error = cmd->error;
- if (cmd->cb.callback (cmd->cb.user_data, cookie, &error) == -1 && error)
+ if (cmd->cb.callback (LIBNBD_CALLBACK_VALID,
+ cmd->cb.user_data, cookie, &error) == -1 && error)
cmd->error = error;
}
diff --git a/generator/states.c b/generator/states.c
index 69aa431..374b8c5 100644
--- a/generator/states.c
+++ b/generator/states.c
@@ -124,7 +124,8 @@ void abort_commands (struct nbd_handle *h,
int error = cmd->error ? cmd->error : ENOTCONN;
assert (cmd->type != NBD_CMD_DISC);
- if (cmd->cb.callback (cmd->cb.user_data, cmd->cookie,
+ if (cmd->cb.callback (LIBNBD_CALLBACK_VALID,
+ cmd->cb.user_data, cmd->cookie,
&error) == -1 && error)
cmd->error = error;
}
diff --git a/interop/dirty-bitmap.c b/interop/dirty-bitmap.c
index 8f0087d..5973696 100644
--- a/interop/dirty-bitmap.c
+++ b/interop/dirty-bitmap.c
@@ -42,11 +42,14 @@ struct data {
};
static int
-cb (void *opaque, const char *metacontext, uint64_t offset,
+cb (int valid_flag, 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 d00524f..ab659da 100644
--- a/interop/structured-read.c
+++ b/interop/structured-read.c
@@ -48,12 +48,16 @@ struct data {
};
static int
-read_cb (void *opaque, const void *bufv, size_t count, uint64_t offset,
+read_cb (int valid_flag, void *opaque,
+ const void *bufv, size_t count, uint64_t offset,
int 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 8d7cb8d..de29b14 100644
--- a/lib/aio.c
+++ b/lib/aio.c
@@ -90,6 +90,17 @@ nbd_unlocked_aio_command_completed (struct nbd_handle *h,
else
h->cmds_done = cmd->next;
+ /* Free the callbacks. */
+ if (cmd->type != NBD_CMD_READ && 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.read)
+ cmd->cb.fn.read (LIBNBD_CALLBACK_FREE, cmd->cb.fn_user_data,
+ NULL, 0, 0, 0, NULL);
+ if (cmd->cb.callback)
+ cmd->cb.callback (LIBNBD_CALLBACK_FREE, cmd->cb.user_data,
+ 0, NULL);
+
free (cmd);
/* If the command was successful, return true. */
diff --git a/lib/debug.c b/lib/debug.c
index 12c10f7..56a9455 100644
--- a/lib/debug.c
+++ b/lib/debug.c
@@ -40,9 +40,12 @@ nbd_unlocked_get_debug (struct nbd_handle *h)
int
nbd_unlocked_set_debug_callback (struct nbd_handle *h,
- int (*debug_fn) (void *, const char *, const char *),
+ int (*debug_fn) (int, void *, const char *, const char *),
void *data)
{
+ if (h->debug_fn)
+ /* ignore return value */
+ h->debug_fn (LIBNBD_CALLBACK_FREE, h->debug_data, NULL, NULL);
h->debug_fn = debug_fn;
h->debug_data = data;
return 0;
@@ -76,7 +79,7 @@ nbd_internal_debug (struct nbd_handle *h, const char *fs, ...)
if (h->debug_fn)
/* ignore return value */
- h->debug_fn (h->debug_data, context, msg);
+ h->debug_fn (LIBNBD_CALLBACK_VALID, h->debug_data, context, msg);
else
fprintf (stderr, "libnbd: debug: %s: %s\n", context ? : "unknown", msg);
out:
diff --git a/lib/internal.h b/lib/internal.h
index b2a65bc..1790d01 100644
--- a/lib/internal.h
+++ b/lib/internal.h
@@ -80,7 +80,7 @@ struct nbd_handle {
/* For debugging. */
bool debug;
- int (*debug_fn) (void *, const char *, const char *);
+ int (*debug_fn) (int, void *, const char *, const char *);
void *debug_data;
/* Linked list of close callbacks. */
@@ -257,12 +257,14 @@ struct socket {
const struct socket_ops *ops;
};
-typedef int (*extent_fn) (void *user_data,
+typedef int (*extent_fn) (int valid_flag, void *user_data,
const char *metacontext, uint64_t offset,
uint32_t *entries, size_t nr_entries, int *error);
-typedef int (*read_fn) (void *user_data, const void *buf, size_t count,
+typedef int (*read_fn) (int valid_flag, void *user_data,
+ const void *buf, size_t count,
uint64_t offset, int status, int *error);
-typedef int (*callback_fn) (void *user_data, int64_t cookie, int *error);
+typedef int (*callback_fn) (int valid_flag, void *user_data,
+ int64_t cookie, int *error);
struct command_cb {
union {
diff --git a/tests/meta-base-allocation.c b/tests/meta-base-allocation.c
index 95e029b..b6a0e63 100644
--- a/tests/meta-base-allocation.c
+++ b/tests/meta-base-allocation.c
@@ -30,7 +30,7 @@
#include <libnbd.h>
-static int check_extent (void *data, const char *metacontext,
+static int check_extent (int valid_flag, void *data, const char *metacontext,
uint64_t offset,
uint32_t *entries, size_t nr_entries, int *error);
@@ -128,12 +128,17 @@ main (int argc, char *argv[])
}
static int
-check_extent (void *data, const char *metacontext,
+check_extent (int valid_flag, void *data, const char *metacontext,
uint64_t offset,
uint32_t *entries, size_t nr_entries, int *error)
{
size_t i;
- int id = * (int *)data;
+ int id;
+
+ if (!(valid_flag & LIBNBD_CALLBACK_VALID))
+ return 0;
+
+ id = * (int *)data;
printf ("extent: id=%d, metacontext=%s, offset=%" PRIu64 ", "
"nr_entries=%zu, error=%d\n",
diff --git a/tests/oldstyle.c b/tests/oldstyle.c
index f4397d3..210fd52 100644
--- a/tests/oldstyle.c
+++ b/tests/oldstyle.c
@@ -37,10 +37,16 @@ static char wbuf[512] = { 1, 2, 3, 4 }, rbuf[512];
static const char *progname;
static int
-pread_cb (void *data, const void *buf, size_t count, uint64_t offset,
+pread_cb (int valid_flag, void *data,
+ const void *buf, size_t count, uint64_t offset,
int status, int *error)
{
- int *calls = data;
+ int *calls;
+
+ if (!(valid_flag & LIBNBD_CALLBACK_VALID))
+ return 0;
+
+ calls = data;
++*calls;
if (buf != rbuf || count != sizeof rbuf) {
--
2.22.0
More information about the Libguestfs
mailing list