Richard W.M. Jones
2019-Jul-24 16:54 UTC
[Libguestfs] [PATCH libnbd v2 0/5] lib: Implement closure lifetimes.
v1 was here: https://www.redhat.com/archives/libguestfs/2019-July/thread.html#00231 The changes address everything that Eric picked up in his review of the first two patches. I have also added two more patches (4 and 5) which respectively fix docs and change int status -> unsigned status, as discussed. Passes make, check, check-valgrind. Rich.
Richard W.M. Jones
2019-Jul-24 16:54 UTC
[Libguestfs] [PATCH libnbd v2 1/5] generator: Change Closure so it describes single callbacks.
In preparation for closure lifetimes, split up the Closure so it no
longer describes a list of closures, but a single callback.
This changes the API because functions which take 2 or more closures
now pass a separate user_data for each one.
---
docs/libnbd.pod | 3 +-
examples/strict-structured-reads.c | 2 +-
generator/generator | 760 ++++++++++++----------------
generator/states-reply-simple.c | 2 +-
generator/states-reply-structured.c | 8 +-
lib/internal.h | 3 +-
lib/rw.c | 32 +-
7 files changed, 364 insertions(+), 446 deletions(-)
diff --git a/docs/libnbd.pod b/docs/libnbd.pod
index 5608e63..631bb3b 100644
--- a/docs/libnbd.pod
+++ b/docs/libnbd.pod
@@ -487,8 +487,7 @@ 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. libnbd functions that take
-two callback pointers share the same opaque data for both calls.
+as the first parameter to the callback.
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/strict-structured-reads.c
b/examples/strict-structured-reads.c
index 92eb3e6..a50f662 100644
--- a/examples/strict-structured-reads.c
+++ b/examples/strict-structured-reads.c
@@ -225,7 +225,7 @@ main (int argc, char *argv[])
*d = (struct data) { .offset = offset, .count = maxsize, .flags = flags,
.remaining = r, };
if (nbd_aio_pread_structured_callback (nbd, buf, sizeof buf, offset,
- read_chunk, read_verify, d,
+ read_chunk, d, read_verify, d,
flags) == -1) {
fprintf (stderr, "%s\n", nbd_get_error ());
exit (EXIT_FAILURE);
diff --git a/generator/generator b/generator/generator
index fdacd71..3b57713 100755
--- a/generator/generator
+++ b/generator/generator
@@ -849,10 +849,10 @@ and arg written by the
function *)
| BytesPersistIn of string * string (* same as above, but buffer persists *)
| BytesPersistOut of string * string
-| Closure of bool * closure list (* void *opaque + one or more closures
- flag if true means callbacks persist
- in the handle, false means they only
- exist during the function call *)
+| 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 *)
| Flags of string (* NBD_CMD_FLAG_* flags *)
| Int of string (* small int *)
| Int64 of string (* 64 bit signed int *)
@@ -921,8 +921,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"] }]) ];
+ { cbname="debug_fn";
+ cbargs=[String "context"; String
"msg"] }) ];
ret = RErr;
shortdesc = "set the debug callback";
longdesc = "\
@@ -1351,10 +1351,10 @@ protocol extensions).";
default_call with
args = [ BytesOut ("buf", "count"); UInt64
"offset";
Closure (false,
- [{ cbname="chunk";
- cbargs=[BytesIn ("subbuf",
"count");
- UInt64 "offset"; Int
"status";
- Mutable (Int "error")] }]);
+ { cbname="chunk";
+ cbargs=[BytesIn ("subbuf",
"count");
+ UInt64 "offset"; Int
"status";
+ Mutable (Int "error")] });
Flags "flags" ];
ret = RErr;
permitted_states = [ Connected ];
@@ -1542,12 +1542,12 @@ punching a hole.";
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")]} ]);
+ { cbname="extent";
+ cbargs=[String "metacontext";
+ UInt64 "offset";
+ ArrayAndLen (UInt32 "entries",
+ "nr_entries");
+ Mutable (Int "error")]} );
Flags "flags" ];
ret = RErr;
permitted_states = [ Connected ];
@@ -1726,8 +1726,8 @@ C<nbd_pread>.";
default_call with
args = [ BytesPersistOut ("buf", "count"); UInt64
"offset";
Closure (true,
- [{ cbname="callback";
- cbargs=[Int64 "cookie"; Mutable (Int
"error")] } ]);
+ { cbname="callback";
+ cbargs=[Int64 "cookie"; Mutable (Int
"error")] } );
Flags "flags" ];
ret = RInt64;
permitted_states = [ Connected ];
@@ -1756,11 +1756,11 @@ cause a deadlock.";
default_call with
args = [ BytesPersistOut ("buf", "count"); UInt64
"offset";
Closure (true,
- [{ cbname="chunk";
- cbargs=[BytesIn ("subbuf",
"count");
- UInt64 "offset";
- Int "status";
- Mutable (Int "error");]} ]);
+ { cbname="chunk";
+ cbargs=[BytesIn ("subbuf",
"count");
+ UInt64 "offset";
+ Int "status";
+ Mutable (Int "error");]} );
Flags "flags" ];
ret = RInt64;
permitted_states = [ Connected ];
@@ -1778,14 +1778,15 @@ documented in C<nbd_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"); ]};
- { cbname="callback";
- cbargs=[Int64 "cookie";
- Mutable (Int "error"); ]} ]);
+ { cbname="chunk";
+ cbargs=[BytesIn ("subbuf",
"count");
+ UInt64 "offset";
+ Int "status";
+ Mutable (Int "error"); ]});
+ Closure (true,
+ { cbname="callback";
+ cbargs=[Int64 "cookie";
+ Mutable (Int "error"); ]} );
Flags "flags" ];
ret = RInt64;
permitted_states = [ Connected ];
@@ -1829,8 +1830,8 @@ C<nbd_pwrite>.";
default_call with
args = [ BytesPersistIn ("buf", "count"); UInt64
"offset";
Closure (true,
- [{ cbname="callback";
- cbargs=[Int64 "cookie"; Mutable (Int
"error")]} ]);
+ { cbname="callback";
+ cbargs=[Int64 "cookie"; Mutable (Int
"error")]} );
Flags "flags" ];
ret = RInt64;
permitted_states = [ Connected ];
@@ -1894,8 +1895,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")]} ]);
+ { cbname="callback";
+ cbargs=[Int64 "cookie"; Mutable (Int
"error")]} );
Flags "flags" ];
ret = RInt64;
permitted_states = [ Connected ];
@@ -1937,8 +1938,8 @@ Parameters behave as documented in
C<nbd_trim>.";
default_call with
args = [ UInt64 "count"; UInt64 "offset";
Closure (true,
- [{ cbname="callback";
- cbargs=[Int64 "cookie"; Mutable (Int
"error")]} ]);
+ { cbname="callback";
+ cbargs=[Int64 "cookie"; Mutable (Int
"error")]} );
Flags "flags" ];
ret = RInt64;
permitted_states = [ Connected ];
@@ -1980,8 +1981,8 @@ Parameters behave as documented in
C<nbd_cache>.";
default_call with
args = [ UInt64 "count"; UInt64 "offset";
Closure (true,
- [{ cbname="callback";
- cbargs=[Int64 "cookie"; Mutable (Int
"error")]} ]);
+ { cbname="callback";
+ cbargs=[Int64 "cookie"; Mutable (Int
"error")]} );
Flags "flags" ];
ret = RInt64;
permitted_states = [ Connected ];
@@ -2023,8 +2024,8 @@ Parameters behave as documented in
C<nbd_zero>.";
default_call with
args = [ UInt64 "count"; UInt64 "offset";
Closure (true,
- [{ cbname="callback";
- cbargs=[Int64 "cookie"; Mutable (Int
"error")]} ]);
+ { cbname="callback";
+ cbargs=[Int64 "cookie"; Mutable (Int
"error")]} );
Flags "flags" ];
ret = RInt64;
permitted_states = [ Connected ];
@@ -2052,11 +2053,11 @@ cause a deadlock.";
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")] } ]);
+ { cbname="extent";
+ cbargs=[String "metacontext"; UInt64
"offset";
+ ArrayAndLen (UInt32 "entries",
+ "nr_entries");
+ Mutable (Int "error")] } );
Flags "flags" ];
ret = RInt64;
permitted_states = [ Connected ];
@@ -2073,13 +2074,14 @@ Parameters behave as documented in
C<nbd_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")]};
- { cbname="callback";
- cbargs=[Int64 "cookie"; Mutable (Int
"error")]} ]);
+ { 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")]} );
Flags "flags" ];
ret = RInt64;
permitted_states = [ Connected ];
@@ -3137,20 +3139,6 @@ let () name
) handle_calls;
- (* Closure also must appear only once, but can be anywhere. *)
- List.iter (
- fun (name, { args }) ->
- let rec loop = function
- | [] -> ()
- | Closure _ :: xs ->
- if List.exists (function Closure _ -> true | _ -> false) xs
then
- failwithf "%s: Closure must appear only once" name
- | x :: xs ->
- loop xs
- in
- loop args
- ) handle_calls;
-
(* !may_set_error is incompatible with permitted_states != [] because
* an incorrect state will result in set_error being called by the
* generated wrapper.
@@ -3186,8 +3174,7 @@ let rec name_of_arg = function
| BytesOut (n, len) -> [n; len]
| BytesPersistIn (n, len) -> [n; len]
| BytesPersistOut (n, len) -> [n; len]
-| Closure (_, closures) ->
- List.map (fun { cbname } -> cbname) closures @ ["user_data"]
+| Closure (_, { cbname }) -> [cbname; sprintf "%s_user_data"
cbname ]
| Flags n -> [n]
| Int n -> [n]
| Int64 n -> [n]
@@ -3241,19 +3228,16 @@ let rec print_arg_list ?(handle = false) ?(user_data =
false)
pr "%s, " n;
if types then pr "size_t ";
pr "%s" len
- | Closure (_, cls) ->
- List.iter (
- fun { cbname; cbargs } ->
- if types then (
- pr "int (*%s) " cbname;
- print_arg_list ~user_data:true cbargs;
- )
- else
- pr "%s" cbname;
- pr ", "
- ) cls;
+ | Closure (_, { cbname; cbargs }) ->
+ if types then (
+ pr "int (*%s) " cbname;
+ print_arg_list ~user_data:true cbargs;
+ )
+ else
+ pr "%s" cbname;
+ pr ", ";
if types then pr "void *";
- pr "user_data"
+ pr "%s_user_data" cbname
| Flags n ->
if types then pr "uint32_t ";
pr "%s" n
@@ -3811,154 +3795,140 @@ let print_python_binding name { args; ret } *)
List.iter (
function
- | Closure (persistent, cls) ->
- pr "struct %s_user_data {\n" name;
- List.iter (
- fun { cbname } ->
- pr " PyObject *%s;\n" cbname
- ) cls;
- pr "};\n";
- pr "\n";
-
+ | 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_user_data (void *vp)\n" name;
+ pr "free_%s_%s_user_data (void *vp)\n" name cbname;
pr "{\n";
- pr " struct %s_user_data *user_data = vp;\n" name;
+ pr " PyObject *user_data = vp;\n";
pr "\n";
- List.iter (
- fun { cbname } ->
- pr " Py_DECREF (user_data->%s);\n" cbname
- ) cls;
- pr " free (user_data);\n";
+ pr " Py_DECREF (user_data);\n";
pr "}\n";
pr "\n";
);
+ 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;
+ pr "\n";
+ pr "{\n";
+ pr " int ret;\n";
+ pr " PyGILState_STATE py_save = PyGILState_UNLOCKED;\n";
+ pr " PyObject *py_args, *py_ret;\n";
List.iter (
- fun { cbname; cbargs } ->
- pr "static int\n";
- pr "%s_%s_wrapper " name cbname;
- C.print_arg_list ~user_data:true cbargs;
- pr "\n";
- pr "{\n";
- pr " int ret;\n";
- pr " PyGILState_STATE py_save = PyGILState_UNLOCKED;\n";
- pr " PyObject *py_args, *py_ret;\n";
- List.iter (
- function
- | ArrayAndLen (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
- | BytesIn _
- | Int _
- | Int64 _ -> ()
- | Mutable (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;
- | String n
- | UInt64 n -> ()
- (* The following not yet implemented for callbacks XXX *)
- | ArrayAndLen _ | Bool _ | BytesOut _
- | BytesPersistIn _ | BytesPersistOut _
- | Closure _
- | Flags _ | Mutable _
- | Path _ | SockAddrAndLen _ | StringList _
- | UInt _ | UInt32 _ -> assert false
- ) cbargs;
- pr "\n";
+ function
+ | ArrayAndLen (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
+ | BytesIn _
+ | Int _
+ | Int64 _ -> ()
+ | Mutable (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;
+ | String n
+ | UInt64 n -> ()
+ (* The following not yet implemented for callbacks XXX *)
+ | ArrayAndLen _ | Bool _ | BytesOut _
+ | BytesPersistIn _ | BytesPersistOut _
+ | Closure _
+ | Flags _ | Mutable _
+ | Path _ | SockAddrAndLen _ | StringList _
+ | UInt _ | UInt32 _ -> assert false
+ ) cbargs;
+ pr "\n";
- pr " py_args = Py_BuildValue (\"(\"";
- List.iter (
- function
- | ArrayAndLen (UInt32 n, len) -> pr "
\"O\""
- | BytesIn (n, len) -> pr " \"y#\""
- | Int n -> pr " \"i\""
- | Int64 n -> pr " \"L\""
- | Mutable (Int n) -> pr " \"O\""
- | String n -> pr " \"s\""
- | UInt64 n -> pr " \"K\""
- (* The following not yet implemented for callbacks XXX *)
- | ArrayAndLen _ | Bool _ | BytesOut _
- | BytesPersistIn _ | BytesPersistOut _
- | Closure _
- | Flags _ | Mutable _
- | Path _ | SockAddrAndLen _ | StringList _
- | UInt _ | UInt32 _ -> assert false
- ) cbargs;
- pr " \")\"";
- List.iter (
- function
- | ArrayAndLen (UInt32 n, _) -> pr ", py_%s" n
- | BytesIn (n, len) -> pr ", %s, (int) %s" n len
- | Mutable (Int n) -> pr ", py_%s" n
- | Int n | Int64 n
- | String n
- | UInt64 n -> pr ", %s" n
- (* The following not yet implemented for callbacks XXX *)
- | ArrayAndLen _ | Bool _ | BytesOut _
- | BytesPersistIn _ | BytesPersistOut _
- | Closure _
- | Flags _ | Mutable _
- | Path _ | SockAddrAndLen _ | StringList _
- | UInt _ | UInt32 _ -> assert false
- ) cbargs;
- pr ");\n";
- pr " Py_INCREF (py_args);\n";
- pr "\n";
- pr " if (PyEval_ThreadsInitialized ())\n";
- pr " py_save = PyGILState_Ensure ();\n";
- pr "\n";
- pr " py_ret = PyObject_CallObject (((struct %s_user_data
*)user_data)->%s, py_args);\n" name cbname;
- pr "\n";
- pr " if (PyEval_ThreadsInitialized ())\n";
- pr " PyGILState_Release (py_save);\n";
- pr "\n";
- pr " Py_DECREF (py_args);\n";
- pr "\n";
- pr " if (py_ret != NULL) {\n";
- pr " ret = 0;\n";
- pr " Py_DECREF (py_ret); /* return value is discarded
*/\n";
- pr " }\n";
- pr " else {\n";
- pr " ret = -1;\n";
- pr " PyErr_PrintEx (0); /* print exception */\n";
- pr " };\n";
- pr "\n";
- List.iter (
- function
- | ArrayAndLen (UInt32 n, _) ->
- pr " Py_DECREF (py_%s);\n" n
- | Mutable (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
- | BytesIn _
- | Int _ | Int64 _
- | String _
- | UInt64 _ -> ()
- (* The following not yet implemented for callbacks XXX *)
- | ArrayAndLen _ | Bool _ | BytesOut _
- | BytesPersistIn _ | BytesPersistOut _
- | Closure _
- | Flags _ | Mutable _
- | Path _ | SockAddrAndLen _ | StringList _
- | UInt _ | UInt32 _ -> assert false
- ) cbargs;
- pr " return ret;\n";
- pr "}\n";
- pr "\n"
- ) cls
+ pr " py_args = Py_BuildValue (\"(\"";
+ List.iter (
+ function
+ | ArrayAndLen (UInt32 n, len) -> pr " \"O\""
+ | BytesIn (n, len) -> pr " \"y#\""
+ | Int n -> pr " \"i\""
+ | Int64 n -> pr " \"L\""
+ | Mutable (Int n) -> pr " \"O\""
+ | String n -> pr " \"s\""
+ | UInt64 n -> pr " \"K\""
+ (* The following not yet implemented for callbacks XXX *)
+ | ArrayAndLen _ | Bool _ | BytesOut _
+ | BytesPersistIn _ | BytesPersistOut _
+ | Closure _
+ | Flags _ | Mutable _
+ | Path _ | SockAddrAndLen _ | StringList _
+ | UInt _ | UInt32 _ -> assert false
+ ) cbargs;
+ pr " \")\"";
+ List.iter (
+ function
+ | ArrayAndLen (UInt32 n, _) -> pr ", py_%s" n
+ | BytesIn (n, len) -> pr ", %s, (int) %s" n len
+ | Mutable (Int n) -> pr ", py_%s" n
+ | Int n | Int64 n
+ | String n
+ | UInt64 n -> pr ", %s" n
+ (* The following not yet implemented for callbacks XXX *)
+ | ArrayAndLen _ | Bool _ | BytesOut _
+ | BytesPersistIn _ | BytesPersistOut _
+ | Closure _
+ | Flags _ | Mutable _
+ | Path _ | SockAddrAndLen _ | StringList _
+ | UInt _ | UInt32 _ -> assert false
+ ) cbargs;
+ pr ");\n";
+ pr " Py_INCREF (py_args);\n";
+ pr "\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 "\n";
+ pr " if (PyEval_ThreadsInitialized ())\n";
+ pr " PyGILState_Release (py_save);\n";
+ pr "\n";
+ pr " Py_DECREF (py_args);\n";
+ pr "\n";
+ pr " if (py_ret != NULL) {\n";
+ pr " ret = 0;\n";
+ pr " Py_DECREF (py_ret); /* return value is discarded
*/\n";
+ pr " }\n";
+ pr " else {\n";
+ pr " ret = -1;\n";
+ pr " PyErr_PrintEx (0); /* print exception */\n";
+ pr " };\n";
+ pr "\n";
+ List.iter (
+ function
+ | ArrayAndLen (UInt32 n, _) ->
+ pr " Py_DECREF (py_%s);\n" n
+ | Mutable (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
+ | BytesIn _
+ | Int _ | Int64 _
+ | String _
+ | UInt64 _ -> ()
+ (* The following not yet implemented for callbacks XXX *)
+ | ArrayAndLen _ | Bool _ | BytesOut _
+ | BytesPersistIn _ | BytesPersistOut _
+ | Closure _
+ | Flags _ | Mutable _
+ | Path _ | SockAddrAndLen _ | StringList _
+ | UInt _ | UInt32 _ -> assert false
+ ) cbargs;
+ pr " return ret;\n";
+ pr "}\n";
+ pr "\n"
| _ -> ()
) args;
@@ -3996,11 +3966,8 @@ 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 (false, cls) ->
- pr " struct %s_user_data _user_data;\n" name;
- pr " struct %s_user_data *user_data = &_user_data;\n"
name
- | Closure (true, cls) ->
- pr " struct %s_user_data *user_data;\n" name
+ | Closure (_, { cbname }) ->
+ pr " PyObject *%s_user_data;\n" cbname
| Flags n ->
pr " uint32_t %s_u32;\n" n;
pr " unsigned int %s; /* really uint32_t */\n" n
@@ -4031,19 +3998,6 @@ let print_python_binding name { args; ret } ) args;
pr "\n";
- (* Allocate the persistent closure user_data. *)
- List.iter (
- function
- | Closure (false, _) -> ()
- | Closure (true, cls) ->
- pr " user_data = malloc (sizeof *user_data);\n";
- pr " if (user_data == NULL) {\n";
- pr " PyErr_NoMemory ();\n";
- pr " return NULL;\n";
- pr " }\n"
- | _ -> ()
- ) args;
-
(* Parse the Python parameters. *)
pr " if (!PyArg_ParseTuple (args, (char *) \"O\"";
List.iter (
@@ -4054,7 +4008,7 @@ let print_python_binding name { args; ret } |
BytesPersistIn (n, _) -> pr " \"O\""
| BytesOut (_, count) -> pr " \"n\""
| BytesPersistOut (_, count) -> pr " \"O\""
- | Closure (_, cls) -> List.iter (fun _ -> pr "
\"O\"") cls
+ | Closure _ -> pr " \"O\""
| Flags n -> pr " \"I\""
| Int n -> pr " \"i\""
| Int64 n -> pr " \"L\""
@@ -4078,8 +4032,7 @@ let print_python_binding name { args; ret } | BytesIn
(n, _) | BytesPersistIn (n, _)
| BytesPersistOut (n, _) -> pr ", &%s" n
| BytesOut (_, count) -> pr ", &%s" count
- | Closure (_, cls) ->
- List.iter (fun { cbname } -> pr ", &user_data->%s"
cbname) cls
+ | Closure (_, { cbname }) -> pr ", &%s_user_data" cbname
| Flags n -> pr ", &%s" n
| Int n -> pr ", &%s" n
| Int64 n -> pr ", &%s" n
@@ -4101,11 +4054,8 @@ let print_python_binding name { args; ret } List.iter
(
function
| Closure (false, _) -> ()
- | Closure (true, cls) ->
- List.iter (
- fun { cbname } ->
- pr " Py_INCREF (user_data->%s);\n" cbname
- ) cls
+ | Closure (true, { cbname }) ->
+ pr " Py_INCREF (%s_user_data);\n" cbname
| _ -> ()
) args;
@@ -4136,15 +4086,12 @@ 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 (_, cls) ->
- List.iter (
- fun { cbname } ->
- pr " if (!PyCallable_Check (user_data->%s)) {\n"
cbname;
- pr " PyErr_SetString (PyExc_TypeError,\n";
- pr " \"callback parameter %s is not
callable\");\n" cbname;
- pr " return NULL;\n";
- pr " }\n"
- ) cls
+ | 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;
+ pr " return NULL;\n";
+ pr " }\n"
| Flags n -> pr " %s_u32 = %s;\n" n n
| Int _ -> ()
| Int64 n -> pr " %s_i64 = %s;\n" n n
@@ -4175,12 +4122,9 @@ 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 (_, cls) ->
- List.iter (
- fun { cbname } ->
- pr ", %s_%s_wrapper" name cbname
- ) cls;
- pr ", user_data"
+ | Closure (_, { cbname }) ->
+ pr ", %s_%s_wrapper" name cbname;
+ pr ", %s_user_data" cbname
| Flags n -> pr ", %s_u32" n
| Int n -> pr ", %s" n
| Int64 n -> pr ", %s_i64" n
@@ -4257,9 +4201,10 @@ let print_python_binding name { args; ret } |
BytesIn (n, _) -> pr " PyBuffer_Release (&%s);\n" n
| BytesPersistIn _ | BytesOut _ | BytesPersistOut _ -> ()
| Closure (false, _) -> ()
- | Closure (true, _) ->
+ | Closure (true, { cbname }) ->
pr " /* This ensures the callback data is freed eventually.
*/\n";
- pr " nbd_add_close_callback (h, free_%s_user_data,
user_data);\n" name
+ pr " nbd_add_close_callback (h, free_%s_%s_user_data,
%s_user_data);\n"
+ name cbname cbname
| Flags _ -> ()
| Int _ -> ()
| Int64 _ -> ()
@@ -4406,8 +4351,7 @@ class NBD (object):
| BytesIn (n, _) | BytesPersistIn (n, _) -> [n, None]
| BytesPersistOut (n, _) -> [n, None]
| BytesOut (_, count) -> [count, None]
- | Closure (_, cls) ->
- List.map (fun { cbname } -> cbname, None) cls
+ | Closure (_, { cbname }) -> [cbname, None]
| Flags n -> [n, Some "0"]
| Int n -> [n, None]
| Int64 n -> [n, None]
@@ -4471,7 +4415,6 @@ end = struct
type ocaml_arg | OCamlHandle (* The NBD handle (NBD.t) *)
| OCamlFlags of string (* Optional ?flags parameter *)
- | OCamlClosure of bool * closure (* Single closure parameter *)
| OCamlArg of arg (* Other arg (string = name). *)
let args_to_ocaml_args args @@ -4483,8 +4426,6 @@ let args_to_ocaml_args args
let args List.map (
function
- | Closure (persistent, cls) ->
- List.map (fun cl -> OCamlClosure (persistent, cl)) cls
| a -> [OCamlArg a]
) args in
let args = List.flatten args in
@@ -4502,10 +4443,6 @@ let rec ocaml_fundecl_to_string args ret and
ocaml_arg_to_string = function
| OCamlHandle -> "t"
| OCamlFlags n -> sprintf "?%s:int32 list" n
- | OCamlClosure (_, { cbargs }) ->
- sprintf "(%s)"
- (ocaml_fundecl_to_string (List.map (fun a -> OCamlArg a)
cbargs)
- RErr)
| OCamlArg (ArrayAndLen (t, _)) ->
sprintf "%s array" (ocaml_arg_to_string (OCamlArg t))
| OCamlArg (Bool _) -> "bool"
@@ -4513,7 +4450,10 @@ and ocaml_arg_to_string = function
| OCamlArg (BytesPersistIn _) -> "Buffer.t"
| OCamlArg (BytesOut _) -> "bytes"
| OCamlArg (BytesPersistOut _) -> "Buffer.t"
- | OCamlArg (Closure _) -> assert false (* see above *)
+ | OCamlArg (Closure (_, { cbargs })) ->
+ sprintf "(%s)"
+ (ocaml_fundecl_to_string (List.map (fun a -> OCamlArg a)
cbargs)
+ RErr)
| OCamlArg (Flags _) -> assert false (* see above *)
| OCamlArg (Int _) -> "int"
| OCamlArg (Int64 _) -> "int64"
@@ -4538,7 +4478,6 @@ and ocaml_ret_to_string = function
let rec name_of_ocaml_arg = function
| OCamlHandle -> "h"
| OCamlFlags n -> n
- | OCamlClosure (_, { cbname }) -> cbname
| OCamlArg a ->
match a with
| ArrayAndLen (arg, n) -> name_of_ocaml_arg (OCamlArg arg)
@@ -4547,7 +4486,7 @@ let rec name_of_ocaml_arg = function
| BytesOut (n, len) -> n
| BytesPersistIn (n, len) -> n
| BytesPersistOut (n, len) -> n
- | Closure (_, closures) -> assert false
+ | Closure (_, { cbname }) -> cbname
| Flags n -> n
| Int n -> n
| Int64 n -> n
@@ -4705,42 +4644,31 @@ let print_ocaml_binding (name, { args; ret }) (*
Functions with a callback parameter require special handling. *)
List.iter (
function
- | Closure (persistent, cls) ->
- pr "struct %s_user_data {\n" name;
- List.iter (fun { cbname } -> pr " value %s;\n" cbname)
cls;
- pr "};\n";
- pr "\n";
-
+ | 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_user_data (void *vp)\n" name;
+ pr "free_%s_%s_user_data (void *vp)\n" name cbname;
pr "{\n";
- pr " struct %s_user_data *user_data = vp;\n" name;
+ pr " value *user_data = vp;\n";
pr "\n";
- List.iter (
- fun { cbname } ->
- pr " caml_remove_generational_global_root
(&user_data->%s);\n"
- cbname
- ) cls;
+ pr " caml_remove_generational_global_root (user_data);\n";
pr " free (user_data);\n";
pr "}\n";
pr "\n"
);
- List.iter (
- fun { cbname; cbargs } ->
- let argnames - List.map (
- function
- | ArrayAndLen (UInt32 n, _) | BytesIn (n, _)
+ let argnames + List.map (
+ function
+ | ArrayAndLen (UInt32 n, _) | BytesIn (n, _)
| Int n | Int64 n
| Mutable (Int n) | String n | UInt64 n ->
- n ^ "v"
- (* The following not yet implemented for callbacks XXX *)
- | ArrayAndLen _ | Bool _ | BytesOut _
+ n ^ "v"
+ (* The following not yet implemented for callbacks XXX *)
+ | ArrayAndLen _ | Bool _ | BytesOut _
| BytesPersistIn _ | BytesPersistOut _
| Closure _
| Flags _ | Path _ | Mutable _
@@ -4748,105 +4676,105 @@ let print_ocaml_binding (name, { args; ret })
| UInt _ | UInt32 _ -> assert false
) cbargs in
- pr "static int\n";
- pr "%s_%s_wrapper_locked " name cbname;
- C.print_arg_list ~user_data:true cbargs;
- pr "\n";
- pr "{\n";
- pr " CAMLparam0 ();\n";
- assert (List.length argnames <= 5);
- pr " CAMLlocal%d (%s);\n" (List.length argnames)
- (String.concat ", " argnames);
- pr " CAMLlocal2 (fnv, rv);\n";
- pr " value args[%d];\n" (List.length argnames);
- pr "\n";
+ pr "/* Wrapper for %s callback of %s. */\n" cbname name;
+ pr "static int\n";
+ pr "%s_%s_wrapper_locked " name cbname;
+ C.print_arg_list ~user_data:true cbargs;
+ pr "\n";
+ pr "{\n";
+ pr " CAMLparam0 ();\n";
+ assert (List.length argnames <= 5);
+ pr " CAMLlocal%d (%s);\n" (List.length argnames)
+ (String.concat ", " argnames);
+ pr " CAMLlocal2 (fnv, rv);\n";
+ pr " value args[%d];\n" (List.length argnames);
+ pr "\n";
- List.iter (
- function
- | ArrayAndLen (UInt32 n, count) ->
- pr " %sv = nbd_internal_ocaml_alloc_int32_array (%s,
%s);\n"
- n n count;
- | BytesIn (n, len) ->
- pr " %sv = caml_alloc_string (%s);\n" n len;
- pr " memcpy (String_val (%sv), %s, %s);\n" n n len
- | Int n ->
- pr " %sv = Val_int (%s);\n" n n
- | Int64 n ->
- pr " %sv = caml_copy_int64 (%s);\n" n n
- | String n ->
- pr " %sv = caml_copy_string (%s);\n" n n
- | UInt64 n ->
- pr " %sv = caml_copy_int64 (%s);\n" n n
- | Mutable (Int n) ->
- pr " %sv = caml_alloc_tuple (1);\n" n;
- pr " Store_field (%sv, 0, Val_int (*%s));\n" n n
- (* The following not yet implemented for callbacks XXX *)
- | ArrayAndLen _ | Bool _ | BytesOut _
- | BytesPersistIn _ | BytesPersistOut _
- | Closure _
- | Flags _ | Mutable _
- | Path _ | SockAddrAndLen _ | StringList _
- | UInt _ | UInt32 _ -> assert false
- ) cbargs;
+ List.iter (
+ function
+ | ArrayAndLen (UInt32 n, count) ->
+ pr " %sv = nbd_internal_ocaml_alloc_int32_array (%s,
%s);\n"
+ n n count;
+ | BytesIn (n, len) ->
+ pr " %sv = caml_alloc_string (%s);\n" n len;
+ pr " memcpy (String_val (%sv), %s, %s);\n" n n len
+ | Int n ->
+ pr " %sv = Val_int (%s);\n" n n
+ | Int64 n ->
+ pr " %sv = caml_copy_int64 (%s);\n" n n
+ | String n ->
+ pr " %sv = caml_copy_string (%s);\n" n n
+ | UInt64 n ->
+ pr " %sv = caml_copy_int64 (%s);\n" n n
+ | Mutable (Int n) ->
+ pr " %sv = caml_alloc_tuple (1);\n" n;
+ pr " Store_field (%sv, 0, Val_int (*%s));\n" n n
+ (* The following not yet implemented for callbacks XXX *)
+ | ArrayAndLen _ | Bool _ | BytesOut _
+ | BytesPersistIn _ | BytesPersistOut _
+ | Closure _
+ | Flags _ | Mutable _
+ | Path _ | SockAddrAndLen _ | StringList _
+ | UInt _ | UInt32 _ -> assert false
+ ) cbargs;
- List.iteri (fun i n -> pr " args[%d] = %s;\n" i n)
argnames;
+ List.iteri (fun i n -> pr " args[%d] = %s;\n" i n)
argnames;
- pr " fnv = ((struct %s_user_data *)user_data)->%s;\n"
name cbname;
+ pr " fnv = * (value *) user_data;\n";
- pr " rv = caml_callbackN_exn (fnv, %d, args);\n"
- (List.length argnames);
+ pr " rv = caml_callbackN_exn (fnv, %d, args);\n"
+ (List.length argnames);
- List.iter (
- function
- | ArrayAndLen (UInt32 n, count) -> ()
- | BytesIn (n, len) -> ()
- | Int n -> ()
- | Int64 n -> ()
- | String n -> ()
- | UInt64 n -> ()
- | Mutable (Int n) ->
- pr " *%s = Int_val (Field (%sv, 0));\n" n n
- (* The following not yet implemented for callbacks XXX *)
- | ArrayAndLen _ | Bool _ | BytesOut _
- | BytesPersistIn _ | BytesPersistOut _
- | Closure _
- | Flags _ | Mutable _
- | Path _ | SockAddrAndLen _ | StringList _
- | UInt _ | UInt32 _ -> assert false
- ) cbargs;
+ List.iter (
+ function
+ | ArrayAndLen (UInt32 n, count) -> ()
+ | BytesIn (n, len) -> ()
+ | Int n -> ()
+ | Int64 n -> ()
+ | String n -> ()
+ | UInt64 n -> ()
+ | Mutable (Int n) ->
+ pr " *%s = Int_val (Field (%sv, 0));\n" n n
+ (* The following not yet implemented for callbacks XXX *)
+ | ArrayAndLen _ | Bool _ | BytesOut _
+ | BytesPersistIn _ | BytesPersistOut _
+ | Closure _
+ | Flags _ | Mutable _
+ | Path _ | SockAddrAndLen _ | StringList _
+ | UInt _ | UInt32 _ -> assert false
+ ) cbargs;
- pr " if (Is_exception_result (rv)) {\n";
- pr " /* XXX This is not really an error as callbacks can
return\n";
- pr " * an error indication. But perhaps we should direct
this\n";
- pr " * to a more suitable place or formalize what
exception\n";
- pr " * means error versus unexpected failure.\n";
- pr " */\n";
- pr " fprintf (stderr,\n";
- pr " \"libnbd: uncaught OCaml exception:
%%s\\n\",\n";
- pr " caml_format_exception (Extract_exception
(rv)));\n";
- pr " CAMLreturnT (int, -1);\n";
- pr " }\n";
+ pr " if (Is_exception_result (rv)) {\n";
+ pr " /* XXX This is not really an error as callbacks can
return\n";
+ pr " * an error indication. But perhaps we should direct
this\n";
+ pr " * to a more suitable place or formalize what
exception\n";
+ pr " * means error versus unexpected failure.\n";
+ pr " */\n";
+ pr " fprintf (stderr,\n";
+ pr " \"libnbd: uncaught OCaml exception:
%%s\\n\",\n";
+ pr " caml_format_exception (Extract_exception
(rv)));\n";
+ pr " CAMLreturnT (int, -1);\n";
+ pr " }\n";
- pr "\n";
- pr " CAMLreturnT (int, 0);\n";
- pr "}\n";
- pr "\n";
- pr "static int\n";
- pr "%s_%s_wrapper " name cbname;
- C.print_arg_list ~user_data:true cbargs;
- pr "\n";
- pr "{\n";
- pr " int ret;\n";
- pr "\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 " return ret;\n";
- pr "}\n";
- pr "\n"
- ) cls
+ pr "\n";
+ pr " CAMLreturnT (int, 0);\n";
+ pr "}\n";
+ pr "\n";
+ pr "static int\n";
+ pr "%s_%s_wrapper " name cbname;
+ C.print_arg_list ~user_data:true cbargs;
+ pr "\n";
+ pr "{\n";
+ pr " int ret;\n";
+ pr "\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 " return ret;\n";
+ pr "}\n";
+ pr "\n"
| _ -> ()
) args;
@@ -4889,32 +4817,6 @@ let print_ocaml_binding (name, { args; ret }) pr
" CAMLlocal1 (rv);\n";
pr "\n";
- (* Set up user_data for closures. *)
- let has_closures - try
- let cl = List.find (function Closure _ -> true | _ -> false) args
in
- match cl with
- | Closure (b, _) -> Some b
- | _ -> assert false
- with
- Not_found -> None in
- (match has_closures with
- | None -> ()
- | Some false ->
- pr " /* This is safe because we only call this while this
stack\n";
- pr " * frame is live.\n";
- pr " */\n";
- pr " struct %s_user_data _user_data;\n" name;
- pr " struct %s_user_data *user_data = &_user_data;\n"
name
- | Some true ->
- 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 " struct %s_user_data *user_data;\n" name;
- pr " user_data = malloc (sizeof *user_data);\n";
- pr " if (user_data == NULL) caml_raise_out_of_memory ();\n"
- );
-
List.iter (
function
| OCamlHandle ->
@@ -4927,14 +4829,6 @@ let print_ocaml_binding (name, { args; ret }) pr
" %s = Flags_val (Field (%sv, 0));\n" n n;
pr " else /* None */\n";
pr " %s = 0;\n" n
- | OCamlClosure (false, { cbname }) ->
- pr " user_data->%s = %sv;\n" cbname cbname;
- pr " const void *%s = %s_%s_wrapper;\n" cbname name cbname
- | OCamlClosure (true, { cbname }) ->
- pr " user_data->%s = %sv;\n" cbname cbname;
- pr " caml_register_generational_global_root
(&user_data->%s);\n"
- cbname;
- pr " const void *%s = %s_%s_wrapper;\n" cbname name cbname
| OCamlArg (ArrayAndLen (t, _)) -> (* XXX *) ()
| OCamlArg (Bool n) ->
pr " bool %s = Bool_val (%sv);\n" n n
@@ -4952,7 +4846,26 @@ 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 _) -> assert false (* see above *)
+ | 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 })) ->
+ 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 " 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
| OCamlArg (Flags _) -> assert false (* see above *)
| OCamlArg (Int n) ->
pr " int %s = Int_val (%sv);\n" n n
@@ -4975,12 +4888,6 @@ let print_ocaml_binding (name, { args; ret }) pr
" uint64_t %s = Int64_val (%sv);\n" n n
) oargs;
- (match has_closures with
- | None | Some false -> ()
- | Some true ->
- pr " nbd_add_close_callback (h, free_%s_user_data,
user_data);\n" name
- );
-
let errcode match ret with
| RBool | RErr | RFd | RInt | RInt64 -> pr " int r;\n";
"-1"
@@ -5014,7 +4921,6 @@ let print_ocaml_binding (name, { args; ret }) |
OCamlArg (StringList n) -> pr " free (%s);\n" n
| OCamlHandle
| OCamlFlags _
- | OCamlClosure _
| OCamlArg (ArrayAndLen _)
| OCamlArg (Bool _)
| OCamlArg (BytesIn _)
diff --git a/generator/states-reply-simple.c b/generator/states-reply-simple.c
index f1770fc..94875aa 100644
--- a/generator/states-reply-simple.c
+++ b/generator/states-reply-simple.c
@@ -64,7 +64,7 @@
int error = 0;
assert (cmd->error == 0);
- if (cmd->cb.fn.read (cmd->cb.user_data, cmd->data,
cmd->count,
+ if (cmd->cb.fn.read (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 25987ed..f60232e 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.user_data,
+ if (cmd->cb.fn.read (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.user_data,
+ if (cmd->cb.fn.read (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.user_data,
+ if (cmd->cb.fn.read (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.user_data,
+ if (cmd->cb.fn.extent (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/lib/internal.h b/lib/internal.h
index 3f2d3f8..b2a65bc 100644
--- a/lib/internal.h
+++ b/lib/internal.h
@@ -269,8 +269,9 @@ struct command_cb {
extent_fn extent;
read_fn read;
} fn;
+ void *fn_user_data; /* associated with one of the fn callbacks above */
callback_fn callback;
- void *user_data;
+ void *user_data; /* associated with the callback function */
};
struct command {
diff --git a/lib/rw.c b/lib/rw.c
index 680b81a..7999a86 100644
--- a/lib/rw.c
+++ b/lib/rw.c
@@ -280,18 +280,24 @@ nbd_unlocked_aio_pread_structured (struct nbd_handle *h,
void *buf,
uint32_t flags)
{
return nbd_unlocked_aio_pread_structured_callback (h, buf, count, offset,
- read, NULL, user_data,
+ read, user_data,
+ NULL, NULL,
flags);
}
int64_t
nbd_unlocked_aio_pread_structured_callback (struct nbd_handle *h, void *buf,
size_t count, uint64_t offset,
- read_fn read, callback_fn callback,
- void *user_data, uint32_t flags)
+ read_fn read,
+ void *read_user_data,
+ callback_fn callback,
+ void *callback_user_data,
+ uint32_t flags)
{
- struct command_cb cb = { .fn.read = read, .callback = callback,
- .user_data = user_data, };
+ struct command_cb cb = { .fn.read = read,
+ .fn_user_data = read_user_data,
+ .callback = callback,
+ .user_data = callback_user_data, };
if ((flags & ~LIBNBD_CMD_FLAG_DF) != 0) {
set_error (EINVAL, "invalid flag: %" PRIu32, flags);
@@ -493,18 +499,24 @@ nbd_unlocked_aio_block_status (struct nbd_handle *h,
uint32_t flags)
{
return nbd_unlocked_aio_block_status_callback (h, count, offset,
- extent, NULL, user_data,
+ extent, user_data,
+ NULL, NULL,
flags);
}
int64_t
nbd_unlocked_aio_block_status_callback (struct nbd_handle *h,
uint64_t count, uint64_t offset,
- extent_fn extent, callback_fn callback,
- void *user_data, uint32_t flags)
+ extent_fn extent,
+ void *extent_user_data,
+ callback_fn callback,
+ void *callback_user_data,
+ uint32_t flags)
{
- struct command_cb cb = { .fn.extent = extent, .callback = callback,
- .user_data = user_data, };
+ struct command_cb cb = { .fn.extent = extent,
+ .fn_user_data = extent_user_data,
+ .callback = callback,
+ .user_data = callback_user_data };
if (!h->structured_replies) {
set_error (ENOTSUP, "server does not support structured
replies");
--
2.22.0
Richard W.M. Jones
2019-Jul-24 16:54 UTC
[Libguestfs] [PATCH libnbd v2 2/5] lib: Implement closure lifetimes.
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 | 67 +++---
generator/generator | 316 ++++++++++++----------------
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 | 8 +-
lib/internal.h | 11 +-
tests/meta-base-allocation.c | 13 +-
tests/oldstyle.c | 10 +-
15 files changed, 298 insertions(+), 236 deletions(-)
diff --git a/docs/libnbd.pod b/docs/libnbd.pod
index 631bb3b..0ce4d32 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<unsigned 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 (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.
+ }
+
+=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..b7c496f 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 (unsigned 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 (unsigned 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 (unsigned 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 (unsigned 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..a996a67 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 (unsigned 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,40 +131,47 @@ 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 (unsigned valid_flag, void *opaque, int64_t cookie, int *error)
{
- struct data *data = opaque;
- int ret = -1;
+ int ret = 0;
- 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");
+ 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");
*error = EPROTO;
goto cleanup;
}
- }
- 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;
+ total_bytes += data->count;
+ total_success++;
+ ret = 0;
- cleanup:
- while (data->remaining) {
- struct range *r = data->remaining;
- data->remaining = r->next;
- free (r);
+ cleanup:
+ while (data->remaining) {
+ struct range *r = data->remaining;
+ data->remaining = r->next;
+ free (r);
+ }
}
- free (data);
+
+ if (valid_flag & LIBNBD_CALLBACK_FREE)
+ free (opaque);
+
return ret;
}
diff --git a/generator/generator b/generator/generator
index 3b57713..8c7ff16 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 "unsigned ";
+ 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,47 +3788,35 @@ 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 " int ret;\n";
- pr " PyGILState_STATE py_save = PyGILState_UNLOCKED;\n";
- pr " PyObject *py_args, *py_ret;\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";
List.iter (
function
| ArrayAndLen (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
| BytesIn _
| Int _
| Int64 _ -> ()
| Mutable (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;
| String n
| UInt64 n -> ()
(* The following not yet implemented for callbacks XXX *)
@@ -3884,36 +3865,35 @@ let print_python_binding name { args; ret }
| UInt _ | UInt32 _ -> 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 " ret = 0;\n";
- pr " Py_DECREF (py_ret); /* return value is discarded
*/\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 " Py_DECREF (py_ret); /* return value is discarded
*/\n";
+ pr " }\n";
+ pr " else {\n";
+ pr " ret = -1;\n";
+ pr " PyErr_PrintEx (0); /* print exception */\n";
+ pr " };\n";
pr "\n";
List.iter (
function
| ArrayAndLen (UInt32 n, _) ->
- pr " Py_DECREF (py_%s);\n" n
+ pr " Py_DECREF (py_%s);\n" n
| Mutable (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
| BytesIn _
| Int _ | Int64 _
| String _
@@ -3926,6 +3906,11 @@ let print_python_binding name { args; ret } |
Path _ | SockAddrAndLen _ | StringList _
| UInt _ | UInt32 _ -> 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"
@@ -3966,7 +3951,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 +4017,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,17 +4033,6 @@ let print_python_binding name { args; ret } pr
"))\n";
pr " return NULL;\n";
- (* If the parameter has persistent closures then we need to
- * make sure the ref count remains positive.
- *)
- List.iter (
- function
- | Closure (false, _) -> ()
- | Closure (true, { cbname }) ->
- pr " Py_INCREF (%s_user_data);\n" cbname
- | _ -> ()
- ) args;
-
pr " h = get_handle (py_h);\n";
List.iter (
function
@@ -4086,7 +4060,9 @@ 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 " /* 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 " PyErr_SetString (PyExc_TypeError,\n";
pr " \"callback parameter %s is not
callable\");\n" cbname;
@@ -4122,7 +4098,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 +4176,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 +4323,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 +4396,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 +4419,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 +4455,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 +4613,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 +4716,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 +4808,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 +4817,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..1a45c2b 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 (unsigned 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..e0e511b 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 (unsigned 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..7784bd9 100644
--- a/lib/debug.c
+++ b/lib/debug.c
@@ -40,9 +40,11 @@ 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 *),
- void *data)
+ debug_fn debug_fn, 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 +78,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..7f5998c 100644
--- a/lib/internal.h
+++ b/lib/internal.h
@@ -48,6 +48,7 @@ struct meta_context;
struct close_callback;
struct socket;
struct command;
+typedef int (*debug_fn) (unsigned, void *, const char *, const char *);
struct nbd_handle {
/* Lock protecting concurrent access to the handle. */
@@ -80,7 +81,7 @@ struct nbd_handle {
/* For debugging. */
bool debug;
- int (*debug_fn) (void *, const char *, const char *);
+ debug_fn debug_fn;
void *debug_data;
/* Linked list of close callbacks. */
@@ -257,12 +258,14 @@ struct socket {
const struct socket_ops *ops;
};
-typedef int (*extent_fn) (void *user_data,
+typedef int (*extent_fn) (unsigned 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) (unsigned 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) (unsigned 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..9e88d6b 100644
--- a/tests/meta-base-allocation.c
+++ b/tests/meta-base-allocation.c
@@ -30,7 +30,8 @@
#include <libnbd.h>
-static int check_extent (void *data, const char *metacontext,
+static int check_extent (unsigned valid_flag, void *data,
+ const char *metacontext,
uint64_t offset,
uint32_t *entries, size_t nr_entries, int *error);
@@ -128,12 +129,18 @@ main (int argc, char *argv[])
}
static int
-check_extent (void *data, const char *metacontext,
+check_extent (unsigned 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..cb0d435 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 (unsigned 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
Richard W.M. Jones
2019-Jul-24 16:54 UTC
[Libguestfs] [PATCH libnbd v2 3/5] lib: Remove nbd_add_close_callback.
We previously needed nbd_add_close_callback to do cleanup from
language bindings. However now we have closure lifetimes this is no
longer needed and can be removed.
See also:
https://www.redhat.com/archives/libguestfs/2019-July/msg00213.html
---
generator/generator | 18 ------------------
lib/handle.c | 35 -----------------------------------
lib/internal.h | 10 ----------
3 files changed, 63 deletions(-)
diff --git a/generator/generator b/generator/generator
index 8c7ff16..34ca3f1 100755
--- a/generator/generator
+++ b/generator/generator
@@ -3139,7 +3139,6 @@ let generate_lib_libnbd_syms ()
pr "{\n";
pr " global:\n";
- pr " nbd_add_close_callback;\n";
pr " nbd_create;\n";
pr " nbd_close;\n";
pr " nbd_get_errno;\n";
@@ -3339,11 +3338,6 @@ let generate_include_libnbd_h () pr "extern int
nbd_get_errno (void);\n";
pr "#define LIBNBD_HAVE_NBD_GET_ERRNO 1\n";
pr "\n";
- pr "extern int nbd_add_close_callback (struct nbd_handle *h,\n";
- pr " nbd_close_callback cb,\n";
- pr " void *user_data);\n";
- pr "#define LIBNBD_HAVE_NBD_ADD_CLOSE_CALLBACK 1\n";
- pr "\n";
List.iter (
fun (name, { args; ret }) -> print_extern_and_define name args ret
) handle_calls;
@@ -3617,18 +3611,6 @@ errors have corresponding errnos, so even if there has
been an error
this may return C<0>. Error codes are the standard ones from
C<E<lt>errno.hE<gt>>.
-=head1 CLOSE CALLBACKS
-
-You can register close callbacks with the handle. These are
-called when C<nbd_close> is called. The order in which they
-are called is not defined. This API is only available
-from C and is designed to help when writing bindings to libnbd
-from other programming languages.
-
- typedef void (*nbd_close_callback) (void *user_data);
- int nbd_add_close_callback (struct nbd_handle *nbd,
- nbd_close_callback cb, void *user_data);
-
";
pr "=head1 API CALLS\n";
diff --git a/lib/handle.c b/lib/handle.c
index 9c643d8..78e72c5 100644
--- a/lib/handle.c
+++ b/lib/handle.c
@@ -90,18 +90,11 @@ nbd_create (void)
void
nbd_close (struct nbd_handle *h)
{
- struct close_callback *cc, *cc_next;
struct meta_context *m, *m_next;
if (h == NULL)
return;
- for (cc = h->close_callbacks; cc != NULL; cc = cc_next) {
- cc_next = cc->next;
- cc->cb (cc->user_data);
- free (cc);
- }
-
free (h->bs_entries);
for (m = h->meta_contexts; m != NULL; m = m_next) {
m_next = m->next;
@@ -198,34 +191,6 @@ nbd_unlocked_add_meta_context (struct nbd_handle *h, const
char *name)
return 0;
}
-/* This is not generated because we don't want to offer it to other
- * programming languages.
- */
-int
-nbd_add_close_callback (struct nbd_handle *h,
- nbd_close_callback cb, void *user_data)
-{
- int ret;
- struct close_callback *cc;
-
- pthread_mutex_lock (&h->lock);
- cc = malloc (sizeof *cc);
- if (cc == NULL) {
- set_error (errno, "malloc");
- ret = -1;
- goto out;
- }
- cc->next = h->close_callbacks;
- cc->cb = cb;
- cc->user_data = user_data;
- h->close_callbacks = cc;
-
- ret = 0;
- out:
- pthread_mutex_unlock (&h->lock);
- return ret;
-}
-
const char *
nbd_unlocked_get_package_name (struct nbd_handle *h)
{
diff --git a/lib/internal.h b/lib/internal.h
index 7f5998c..9d88f08 100644
--- a/lib/internal.h
+++ b/lib/internal.h
@@ -45,7 +45,6 @@
#define MAX_REQUEST_SIZE (64 * 1024 * 1024)
struct meta_context;
-struct close_callback;
struct socket;
struct command;
typedef int (*debug_fn) (unsigned, void *, const char *, const char *);
@@ -84,9 +83,6 @@ struct nbd_handle {
debug_fn debug_fn;
void *debug_data;
- /* Linked list of close callbacks. */
- struct close_callback *close_callbacks;
-
/* State machine.
*
* The actual current state is ‘state’. ‘public_state’ is updated
@@ -226,12 +222,6 @@ struct meta_context {
uint32_t context_id; /* Context ID negotiated with the server. */
};
-struct close_callback {
- struct close_callback *next; /* Linked list. */
- nbd_close_callback cb; /* Function. */
- void *user_data; /* Data. */
-};
-
struct socket_ops {
ssize_t (*recv) (struct nbd_handle *h,
struct socket *sock, void *buf, size_t len);
--
2.22.0
Richard W.M. Jones
2019-Jul-24 16:54 UTC
[Libguestfs] [PATCH libnbd v2 4/5] docs: callbacks: Document that user_data, valid_flag only used by the C API.
Thanks: Eric Blake.
---
docs/libnbd.pod | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/docs/libnbd.pod b/docs/libnbd.pod
index 0ce4d32..6c8d962 100644
--- a/docs/libnbd.pod
+++ b/docs/libnbd.pod
@@ -487,7 +487,9 @@ 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 second parameter to the callback.
+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.
=head2 Callback lifetimes
@@ -539,6 +541,9 @@ write this:
// 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.
+
=head2 Callbacks and locking
The callbacks are invoked at a point where the libnbd lock is held; as
--
2.22.0
Richard W.M. Jones
2019-Jul-24 16:54 UTC
[Libguestfs] [PATCH libnbd v2 5/5] lib: Use unsigned for pread_structured status parameter.
This is a bitmask so using an unsigned type is slightly safer. This
is not an ABI change since the types are compatible.
Thanks: Eric Blake.
---
examples/strict-structured-reads.c | 2 +-
generator/generator | 67 ++++++++++++++++--------------
interop/structured-read.c | 2 +-
lib/internal.h | 2 +-
tests/oldstyle.c | 2 +-
5 files changed, 39 insertions(+), 36 deletions(-)
diff --git a/examples/strict-structured-reads.c
b/examples/strict-structured-reads.c
index a996a67..2279301 100644
--- a/examples/strict-structured-reads.c
+++ b/examples/strict-structured-reads.c
@@ -53,7 +53,7 @@ static int total_success;
static int
read_chunk (unsigned valid_flag, void *opaque,
const void *bufv, size_t count, uint64_t offset,
- int status, int *error)
+ unsigned status, int *error)
{
struct data *data = opaque;
struct range *r, **prev;
diff --git a/generator/generator b/generator/generator
index 34ca3f1..d46e8d6 100755
--- a/generator/generator
+++ b/generator/generator
@@ -1348,7 +1348,7 @@ protocol extensions).";
args = [ BytesOut ("buf", "count"); UInt64
"offset";
Closure { cbname="chunk";
cbargs=[BytesIn ("subbuf", "count");
- UInt64 "offset"; Int
"status";
+ UInt64 "offset"; UInt
"status";
Mutable (Int "error")] };
Flags "flags" ];
ret = RErr;
@@ -1751,7 +1751,7 @@ cause a deadlock.";
Closure { cbname="chunk";
cbargs=[BytesIn ("subbuf", "count");
UInt64 "offset";
- Int "status";
+ UInt "status";
Mutable (Int "error");]};
Flags "flags" ];
ret = RInt64;
@@ -1772,7 +1772,7 @@ documented in C<nbd_pread_structured>.";
Closure { cbname="chunk";
cbargs=[BytesIn ("subbuf", "count");
UInt64 "offset";
- Int "status";
+ UInt "status";
Mutable (Int "error"); ]};
Closure { cbname="callback";
cbargs=[Int64 "cookie";
@@ -3789,8 +3789,8 @@ let print_python_binding name { args; ret }
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
| BytesIn _
- | Int _
- | Int64 _ -> ()
+ | Int _
+ | Int64 _ -> ()
| Mutable (Int n) ->
pr " PyObject *py_%s_modname = PyUnicode_FromString
(\"ctypes\");\n" n;
pr " if (!py_%s_modname) { PyErr_PrintEx (0); return -1;
}\n" n;
@@ -3799,15 +3799,16 @@ let print_python_binding name { args; ret }
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;
- | String n
- | UInt64 n -> ()
+ | String _
+ | UInt _
+ | UInt64 _ -> ()
(* The following not yet implemented for callbacks XXX *)
| ArrayAndLen _ | Bool _ | BytesOut _
| BytesPersistIn _ | BytesPersistOut _
| Closure _
| Flags _ | Mutable _
| Path _ | SockAddrAndLen _ | StringList _
- | UInt _ | UInt32 _ -> assert false
+ | UInt32 _ -> assert false
) cbargs;
pr "\n";
@@ -3820,6 +3821,7 @@ let print_python_binding name { args; ret } |
Int64 n -> pr " \"L\""
| Mutable (Int n) -> pr " \"O\""
| String n -> pr " \"s\""
+ | UInt n -> pr " \"I\""
| UInt64 n -> pr " \"K\""
(* The following not yet implemented for callbacks XXX *)
| ArrayAndLen _ | Bool _ | BytesOut _
@@ -3827,7 +3829,7 @@ let print_python_binding name { args; ret } |
Closure _
| Flags _ | Mutable _
| Path _ | SockAddrAndLen _ | StringList _
- | UInt _ | UInt32 _ -> assert false
+ | UInt32 _ -> assert false
) cbargs;
pr " \")\"";
List.iter (
@@ -3836,15 +3838,15 @@ let print_python_binding name { args; ret } |
BytesIn (n, len) -> pr ", %s, (int) %s" n len
| Mutable (Int n) -> pr ", py_%s" n
| Int n | Int64 n
- | String n
- | UInt64 n -> pr ", %s" n
+ | String n
+ | UInt n | UInt64 n -> pr ", %s" n
(* The following not yet implemented for callbacks XXX *)
| ArrayAndLen _ | Bool _ | BytesOut _
| BytesPersistIn _ | BytesPersistOut _
| Closure _
| Flags _ | Mutable _
| Path _ | SockAddrAndLen _ | StringList _
- | UInt _ | UInt32 _ -> assert false
+ | UInt32 _ -> assert false
) cbargs;
pr ");\n";
pr " Py_INCREF (py_args);\n";
@@ -3877,16 +3879,16 @@ let print_python_binding name { args; ret }
pr " Py_DECREF (py_%s_ret);\n" n;
pr " Py_DECREF (py_%s);\n" n
| BytesIn _
- | Int _ | Int64 _
- | String _
- | UInt64 _ -> ()
+ | Int _ | Int64 _
+ | String _
+ | UInt _ | UInt64 _ -> ()
(* The following not yet implemented for callbacks XXX *)
| ArrayAndLen _ | Bool _ | BytesOut _
| BytesPersistIn _ | BytesPersistOut _
| Closure _
| Flags _ | Mutable _
| Path _ | SockAddrAndLen _ | StringList _
- | UInt _ | UInt32 _ -> assert false
+ | UInt32 _ -> assert false
) cbargs;
pr " }\n";
pr "\n";
@@ -4600,8 +4602,8 @@ let print_ocaml_binding (name, { args; ret })
List.map (
function
| ArrayAndLen (UInt32 n, _) | BytesIn (n, _)
- | Int n | Int64 n
- | Mutable (Int n) | String n | UInt64 n ->
+ | Int n | Int64 n
+ | Mutable (Int n) | String n | UInt n | UInt64 n ->
n ^ "v"
(* The following not yet implemented for callbacks XXX *)
| ArrayAndLen _ | Bool _ | BytesOut _
@@ -4609,7 +4611,7 @@ let print_ocaml_binding (name, { args; ret })
| Closure _
| Flags _ | Path _ | Mutable _
| SockAddrAndLen _ | StringList _
- | UInt _ | UInt32 _ -> assert false
+ | UInt32 _ -> assert false
) cbargs in
pr "/* Wrapper for %s callback of %s. */\n" cbname name;
@@ -4634,7 +4636,7 @@ let print_ocaml_binding (name, { args; ret }) |
BytesIn (n, len) ->
pr " %sv = caml_alloc_string (%s);\n" n len;
pr " memcpy (String_val (%sv), %s, %s);\n" n n len
- | Int n ->
+ | Int n | UInt n ->
pr " %sv = Val_int (%s);\n" n n
| Int64 n ->
pr " %sv = caml_copy_int64 (%s);\n" n n
@@ -4651,7 +4653,7 @@ let print_ocaml_binding (name, { args; ret })
| Closure _
| Flags _ | Mutable _
| Path _ | SockAddrAndLen _ | StringList _
- | UInt _ | UInt32 _ -> assert false
+ | UInt32 _ -> assert false
) cbargs;
List.iteri (fun i n -> pr " args[%d] = %s;\n" i n)
argnames;
@@ -4663,21 +4665,22 @@ let print_ocaml_binding (name, { args; ret })
List.iter (
function
- | ArrayAndLen (UInt32 n, count) -> ()
- | BytesIn (n, len) -> ()
- | Int n -> ()
- | Int64 n -> ()
- | String n -> ()
- | UInt64 n -> ()
+ | ArrayAndLen (UInt32 _, _)
+ | BytesIn _
+ | Int _
+ | Int64 _
+ | String _
+ | UInt _
+ | UInt64 _ -> ()
| Mutable (Int n) ->
pr " *%s = Int_val (Field (%sv, 0));\n" n n
(* The following not yet implemented for callbacks XXX *)
| ArrayAndLen _ | Bool _ | BytesOut _
- | BytesPersistIn _ | BytesPersistOut _
- | Closure _
- | Flags _ | Mutable _
- | Path _ | SockAddrAndLen _ | StringList _
- | UInt _ | UInt32 _ -> assert false
+ | BytesPersistIn _ | BytesPersistOut _
+ | Closure _
+ | Flags _ | Mutable _
+ | Path _ | SockAddrAndLen _ | StringList _
+ | UInt32 _ -> assert false
) cbargs;
pr " if (Is_exception_result (rv)) {\n";
diff --git a/interop/structured-read.c b/interop/structured-read.c
index e0e511b..4c6e619 100644
--- a/interop/structured-read.c
+++ b/interop/structured-read.c
@@ -50,7 +50,7 @@ struct data {
static int
read_cb (unsigned valid_flag, void *opaque,
const void *bufv, size_t count, uint64_t offset,
- int status, int *error)
+ unsigned status, int *error)
{
struct data *data = opaque;
const char *buf = bufv;
diff --git a/lib/internal.h b/lib/internal.h
index 9d88f08..dd417ea 100644
--- a/lib/internal.h
+++ b/lib/internal.h
@@ -253,7 +253,7 @@ typedef int (*extent_fn) (unsigned valid_flag, void
*user_data,
uint32_t *entries, size_t nr_entries, int *error);
typedef int (*read_fn) (unsigned valid_flag, void *user_data,
const void *buf, size_t count,
- uint64_t offset, int status, int *error);
+ uint64_t offset, unsigned status, int *error);
typedef int (*callback_fn) (unsigned valid_flag, void *user_data,
int64_t cookie, int *error);
diff --git a/tests/oldstyle.c b/tests/oldstyle.c
index cb0d435..95e1c97 100644
--- a/tests/oldstyle.c
+++ b/tests/oldstyle.c
@@ -39,7 +39,7 @@ static const char *progname;
static int
pread_cb (unsigned valid_flag, void *data,
const void *buf, size_t count, uint64_t offset,
- int status, int *error)
+ unsigned status, int *error)
{
int *calls;
--
2.22.0
Eric Blake
2019-Jul-24 18:39 UTC
Re: [Libguestfs] [PATCH libnbd v2 2/5] lib: Implement closure lifetimes.
On 7/24/19 11:54 AM, Richard W.M. Jones wrote:> 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.) >Our mails crossed, and I had questions about the implementation of v1 that aren't addressed here. But I'm also okay if you check this one in as-is, and I can write a followup patch that shuffles things around per my v1 comments as part of rebasing my auto-retire patches on top of this. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
Eric Blake
2019-Jul-24 18:43 UTC
Re: [Libguestfs] [PATCH libnbd v2 5/5] lib: Use unsigned for pread_structured status parameter.
On 7/24/19 11:54 AM, Richard W.M. Jones wrote:> This is a bitmask so using an unsigned type is slightly safer. This > is not an ABI change since the types are compatible. > > Thanks: Eric Blake. > --- > examples/strict-structured-reads.c | 2 +- > generator/generator | 67 ++++++++++++++++-------------- > interop/structured-read.c | 2 +- > lib/internal.h | 2 +- > tests/oldstyle.c | 2 +- > 5 files changed, 39 insertions(+), 36 deletions(-) >> @@ -3789,8 +3789,8 @@ let print_python_binding name { args; ret } > 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 > | BytesIn _ > - | Int _ > - | Int64 _ -> () > + | Int _ > + | Int64 _ -> ()A bit of churn here that could have been squashed into patch 2. But at the end of the day, whether to spend more time polishing the patch when the end result is fine is up to you. ACK series. I'll rebase my auto-retire patches on top of this once you've pushed. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
Eric Blake
2019-Jul-24 20:30 UTC
Re: [Libguestfs] [PATCH libnbd v2 2/5] lib: Implement closure lifetimes.
On 7/24/19 11:54 AM, Richard W.M. Jones wrote:> 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. >> +++ 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);nbd_aio_command_completed is skipped when a user calls nbd_close. While we've documented that nbd_close loses the exit status of any unretired command (different from the fact that completion callbacks run on transition to DEAD but the handle is still around), it is still probably worth tweaking nbd_close's use of free_cmd_list() to still call FREE on any pending callbacks on commands stranded by abrupt close to allow the user to still avoid memory leaks.> +++ b/lib/debug.c > @@ -40,9 +40,11 @@ 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 *), > - void *data) > + debug_fn debug_fn, void *data) > { > + if (h->debug_fn) > + /* ignore return value */ > + h->debug_fn (LIBNBD_CALLBACK_FREE, h->debug_data, NULL, NULL);Is it worth outputting a debug message just before freeing things? Or put differently, should this be something like h->debug_fn (LIBNBD_CALLBACK_VALID | LIBNBD_CALLBACK_FREE, h->debug_data, context, "changing debug callback"); Also, should nbd_close() output a debug message about the handle disappearing (and if so, obviously prior to calling callback(FREE)). -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
Possibly Parallel Threads
- [PATCH libnbd 1/2] generator: Handle closure args (cbargs) specially.
- [libnbd PATCH 2/2] RFC: generator: Handle shared callbacks in Python
- [PATCH libnbd 1/3] generator: Change Closure so it describes single callbacks.
- [PATCH libnbd v2 2/5] lib: Implement closure lifetimes.
- [PATCH libnbd v2] generator: Define new Closure type instead of callbacks.