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

On Wed, Jul 24, 2019 at 10:02:04AM -0500, Eric Blake wrote: Yes, the documentation is rather C-specific. I'll add it (but as another patch on top). There's actually no difference at the machine code level - both tuple and struct have the same representation (same as it would be in C in fact). So it comes down to which is nicer, and I think it's slightly nicer to have the named fields. Indeed. This comment goes away in the following commit :-) I think at some point I gave up fighting tuareg mode :-( I need to ask Martin for the right settings to make it indent in the preferred way. Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-top is 'top' for virtual machines. Tiny program with many powerful monitoring features, net stats, disk stats, logging, etc. http://people.redhat.com/~rjones/virt-top

On 7/24/19 7:17 AM, Richard W.M. Jones wrote: Again, worth mentioning that this explicitly only for the C bindings? Are we guaranteeing never to set other bits, or that other bits will be 0 for now and only set later if the client opts in to whatever the new bits are useful for? But I'm okay with leaving this sentence as worded. Both examples are good. This one is okay (since we pass the same opaque to both this function and the later callback function, so have nothing to free at the last use of this one)... ...but this one is wrong; it calls free(data) which should be deferred to the valid_flag & LIBNBD_CALLBACK_FREE portion. (It looks like later on in the patch, I see that the nbd_aio_FOO_callback callback is always called exactly once with VALID|FREE, so your bug here is masked because of that calling convention - but we should really fix this one to be a better example). Re-echoing my comment in 1/3, writing this as 'Closure of string * arg list' may seem cleaner (certainly less typing below). Should this be 'unsigned int valid_flag', as bitmasks over signed values have awkward semantics if you touch the sign bit? But I'm okay with int for now, as it matches 'int nbd_aio_get_direction(...)' and nbd_pread_structured using 'int status' in its callback. Indentation. Do we care about the generated indentation looking sane? (That can be a followup patch, to minimize the churn on this one...) Ouch. Calling 'return ret' before checking for LIBNBD_CALLBACK_FREE is liable to leak. I think you have to defer the return... ...to here. Now that this match... ...and this match are identical, should we group the code? I got this far in my review; I'll resume later... -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org

On Wed, Jul 24, 2019 at 12:21:37PM -0500, Eric Blake wrote: This is fixed in v2, but ... That was actually my first version of the patch. However it runs into the problem that we need to call the FREE version when a command is aborted. It's complicated to know if we called cb.fn.read already. But thinking about this a bit more, maybe we should call VALID|FREE here and set cmd->cb.fn.read = NULL afterwards. [...] Yes nbd_close should check and call the callback again with FREE. Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com Fedora Windows cross-compiler. Compile Windows programs, test, and build Windows installers. Over 100 libraries supported. http://fedoraproject.org/wiki/MinGW

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 5d5c267..9a15916 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 1790d01..d534243 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; @@ -83,9 +82,6 @@ struct nbd_handle { int (*debug_fn) (int, void *, const char *, const char *); 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 @@ -225,12 +221,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