A Closure is a list of (usually one, but can be more) closures. In C there is also a singe ‘void *user_data’ parameter which is passed by the caller into the function and through as the first parameter of each callback invocation. By grouping the previously separate Opaque and Callback* parameters together we can avoid the awkward situation where we have to scan through the argument list to try to match them up. Because this is a closure, in non-C languages we can simply map these to a closure and drop the opaque parameter entirely. It is not needed since languages with proper closures can capture local state in the closure. Unlike the previous code it is no longer possible to mix persistent and non-persistent callbacks in the same API call. This was not used before and is unlikely to be useful. For the C API there is no API or ABI change (the only change is the naming of the opaque pointer which is not part of the API). For the non-C languages the opaque parameter is no longer required as discussed above. Partly based on Eric Blake's earlier work here: https://www.redhat.com/archives/libguestfs/2019-July/thread.html#00160 --- Makefile.am | 6 +- generator/generator | 619 +++++++++++++++------------- generator/states-reply-simple.c | 2 +- generator/states-reply-structured.c | 12 +- generator/states-reply.c | 2 +- generator/states.c | 3 +- lib/handle.c | 7 +- lib/internal.h | 11 +- lib/rw.c | 56 +-- python/t/405-pread-structured.py | 12 +- python/t/460-block-status.py | 11 +- python/t/600-debug-callback.py | 2 +- 12 files changed, 391 insertions(+), 352 deletions(-) diff --git a/Makefile.am b/Makefile.am index 98a0284..d815a62 100644 --- a/Makefile.am +++ b/Makefile.am @@ -36,11 +36,13 @@ SUBDIRS = \ tests \ python \ sh \ - ocaml \ - ocaml/examples \ interop \ $(NULL) +# ocaml \ +# ocaml/examples \ +# + noinst_SCRIPTS = run # Check no files are missing from EXTRA_DIST rules, and that all diff --git a/generator/generator b/generator/generator index caa6353..d166c11 100755 --- a/generator/generator +++ b/generator/generator @@ -849,13 +849,14 @@ and arg = written by the function *) | BytesPersistIn of string * string (* same as above, but buffer persists *) | BytesPersistOut of string * string -| Callback of string * arg list (* callback function returning int *) -| CallbackPersist of string * arg list (* as above, but callback persists *) +| 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 *) | Flags of string (* NBD_CMD_FLAG_* flags *) | Int of string (* small int *) | Int64 of string (* 64 bit signed int *) | Mutable of arg (* mutable argument, eg. int* *) -| Opaque of string (* opaque object, void* in C *) | Path of string (* filename or path *) | SockAddrAndLen of string * string (* struct sockaddr * + socklen_t *) | String of string (* string *) @@ -863,6 +864,10 @@ and arg = | UInt of string (* small unsigned int *) | UInt32 of string (* 32 bit unsigned int *) | UInt64 of string (* 64 bit unsigned int *) +and closure = { + cbname : string; (* name of callback function *) + cbargs : arg list; (* all closures return int for now *) +} and ret = | RBool (* return a boolean, or error *) | RConstString (* return a const string, NULL for error *) @@ -915,9 +920,9 @@ Return the state of the debug flag on this handle."; "set_debug_callback", { default_call with - args = [ Opaque "data"; - CallbackPersist ("debug_fn", [Opaque "data"; - String "context"; String "msg"]) ]; + args = [ Closure (true, + [{ cbname="debug_fn"; + cbargs=[String "context"; String "msg"] }]) ]; ret = RErr; shortdesc = "set the debug callback"; longdesc = "\ @@ -1345,10 +1350,11 @@ protocol extensions)."; "pread_structured", { default_call with args = [ BytesOut ("buf", "count"); UInt64 "offset"; - Opaque "data"; - Callback ("chunk", [ Opaque "data"; BytesIn ("subbuf", "count"); + Closure (false, + [{ cbname="chunk"; + cbargs=[ BytesIn ("subbuf", "count"); UInt64 "offset"; Int "status"; - Mutable (Int "error"); ]); + Mutable (Int "error")] }]); Flags "flags" ]; ret = RErr; permitted_states = [ Connected ]; @@ -1534,12 +1540,13 @@ punching a hole."; "block_status", { default_call with args = [ UInt64 "count"; UInt64 "offset"; - Opaque "data"; - Callback ("extent", [Opaque "data"; String "metacontext"; - UInt64 "offset"; - ArrayAndLen (UInt32 "entries", - "nr_entries"); - Mutable (Int "error")]); + Closure (false, + [ {cbname="extent"; + cbargs=[String "metacontext"; + UInt64 "offset"; + ArrayAndLen (UInt32 "entries", + "nr_entries"); + Mutable (Int "error")]} ]); Flags "flags" ]; ret = RErr; permitted_states = [ Connected ]; @@ -1717,9 +1724,9 @@ C<nbd_pread>."; "aio_pread_callback", { default_call with args = [ BytesPersistOut ("buf", "count"); UInt64 "offset"; - Opaque "data"; - CallbackPersist ("callback", [ Opaque "data"; Int64 "cookie"; - Mutable (Int "error") ]); + Closure (true, + [ {cbname="callback"; + cbargs=[Int64 "cookie"; Mutable (Int "error")] } ]); Flags "flags" ]; ret = RInt64; permitted_states = [ Connected ]; @@ -1747,12 +1754,12 @@ cause a deadlock."; "aio_pread_structured", { default_call with args = [ BytesPersistOut ("buf", "count"); UInt64 "offset"; - Opaque "data"; - CallbackPersist ("chunk", [ Opaque "data"; - BytesIn ("subbuf", "count"); - UInt64 "offset"; - Int "status"; - Mutable (Int "error"); ]); + Closure (true, + [ {cbname="chunk"; + cbargs=[BytesIn ("subbuf", "count"); + UInt64 "offset"; + Int "status"; + Mutable (Int "error");]} ]); Flags "flags" ]; ret = RInt64; permitted_states = [ Connected ]; @@ -1769,14 +1776,15 @@ documented in C<nbd_pread_structured>."; "aio_pread_structured_callback", { default_call with args = [ BytesPersistOut ("buf", "count"); UInt64 "offset"; - Opaque "data"; - CallbackPersist ("chunk", [ Opaque "data"; - BytesIn ("subbuf", "count"); - UInt64 "offset"; - Int "status"; - Mutable (Int "error"); ]); - CallbackPersist ("callback", [ Opaque "data"; Int64 "cookie"; - Mutable (Int "error") ]); + Closure (true, + [ {cbname="chunk"; + cbargs=[BytesIn ("subbuf", "count"); + UInt64 "offset"; + Int "status"; + Mutable (Int "error"); ]}; + {cbname="callback"; + cbargs=[Int64 "cookie"; + Mutable (Int "error"); ]} ]); Flags "flags" ]; ret = RInt64; permitted_states = [ Connected ]; @@ -1819,9 +1827,9 @@ C<nbd_pwrite>."; "aio_pwrite_callback", { default_call with args = [ BytesPersistIn ("buf", "count"); UInt64 "offset"; - Opaque "data"; - CallbackPersist ("callback", [ Opaque "data"; Int64 "cookie"; - Mutable (Int "error") ]); + Closure (true, + [ {cbname="callback"; + cbargs=[Int64 "cookie"; Mutable (Int "error")]} ]); Flags "flags" ]; ret = RInt64; permitted_states = [ Connected ]; @@ -1884,9 +1892,9 @@ Parameters behave as documented in C<nbd_flush>."; "aio_flush_callback", { default_call with - args = [ Opaque "data"; - CallbackPersist ("callback", [ Opaque "data"; Int64 "cookie"; - Mutable (Int "error") ]); + args = [ Closure (true, + [ {cbname="callback"; + cbargs=[Int64 "cookie"; Mutable (Int "error")]} ]); Flags "flags" ]; ret = RInt64; permitted_states = [ Connected ]; @@ -1927,9 +1935,9 @@ Parameters behave as documented in C<nbd_trim>."; "aio_trim_callback", { default_call with args = [ UInt64 "count"; UInt64 "offset"; - Opaque "data"; - CallbackPersist ("callback", [ Opaque "data"; Int64 "cookie"; - Mutable (Int "error") ]); + Closure (true, + [ {cbname="callback"; + cbargs=[Int64 "cookie"; Mutable (Int "error")]} ]); Flags "flags" ]; ret = RInt64; permitted_states = [ Connected ]; @@ -1970,9 +1978,9 @@ Parameters behave as documented in C<nbd_cache>."; "aio_cache_callback", { default_call with args = [ UInt64 "count"; UInt64 "offset"; - Opaque "data"; - CallbackPersist ("callback", [ Opaque "data"; Int64 "cookie"; - Mutable (Int "error") ]); + Closure (true, + [ {cbname="callback"; + cbargs=[Int64 "cookie"; Mutable (Int "error")]} ]); Flags "flags" ]; ret = RInt64; permitted_states = [ Connected ]; @@ -2013,9 +2021,9 @@ Parameters behave as documented in C<nbd_zero>."; "aio_zero_callback", { default_call with args = [ UInt64 "count"; UInt64 "offset"; - Opaque "data"; - CallbackPersist ("callback", [ Opaque "data"; Int64 "cookie"; - Mutable (Int "error") ]); + Closure (true, + [ {cbname="callback"; + cbargs=[Int64 "cookie"; Mutable (Int "error")]} ]); Flags "flags" ]; ret = RInt64; permitted_states = [ Connected ]; @@ -2042,12 +2050,12 @@ cause a deadlock."; "aio_block_status", { default_call with args = [ UInt64 "count"; UInt64 "offset"; - Opaque "data"; - CallbackPersist ("extent", [Opaque "data"; String "metacontext"; - UInt64 "offset"; - ArrayAndLen (UInt32 "entries", - "nr_entries"); - Mutable (Int "error")]); + Closure (true, + [ {cbname="extent"; + cbargs=[String "metacontext"; UInt64 "offset"; + ArrayAndLen (UInt32 "entries", + "nr_entries"); + Mutable (Int "error")] } ]); Flags "flags" ]; ret = RInt64; permitted_states = [ Connected ]; @@ -2063,14 +2071,14 @@ Parameters behave as documented in C<nbd_block_status>."; "aio_block_status_callback", { default_call with args = [ UInt64 "count"; UInt64 "offset"; - Opaque "data"; - CallbackPersist ("extent", [Opaque "data"; String "metacontext"; - UInt64 "offset"; - ArrayAndLen (UInt32 "entries", - "nr_entries"); - Mutable (Int "error") ]); - CallbackPersist ("callback", [ Opaque "data"; Int64 "cookie"; - Mutable (Int "error") ]); + 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")]} ]); Flags "flags" ]; ret = RInt64; permitted_states = [ Connected ]; @@ -3143,19 +3151,19 @@ let generate_lib_libnbd_syms () = pr " local: *;\n"; pr "};\n" -let rec name_of_arg = function -| ArrayAndLen (arg, n) -> name_of_arg arg @ [n] +let rec c_name_of_arg = function +| ArrayAndLen (arg, n) -> c_name_of_arg arg @ [n] | Bool n -> [n] | BytesIn (n, len) -> [n; len] | BytesOut (n, len) -> [n; len] | BytesPersistIn (n, len) -> [n; len] | BytesPersistOut (n, len) -> [n; len] -| Callback (n, _) | CallbackPersist (n, _) -> [n] +| Closure (_, closures) -> + "user_data" :: List.map (fun { cbname } -> cbname) closures | Flags n -> [n] | Int n -> [n] | Int64 n -> [n] -| Opaque n -> [n] -| Mutable arg -> name_of_arg arg +| Mutable arg -> c_name_of_arg arg | Path n -> [n] | SockAddrAndLen (n, len) -> [n; len] | String n -> [n] @@ -3164,13 +3172,18 @@ let rec name_of_arg = function | UInt32 n -> [n] | UInt64 n -> [n] -let rec print_c_arg_list ?(handle = false) args = +let rec print_c_arg_list ?(handle = false) ?(user_data = false) args = pr "("; let comma = ref false in if handle then ( comma := true; pr "struct nbd_handle *h"; ); + if user_data then ( + if !comma then pr ", "; + comma := true; + pr "void *user_data"; + ); List.iter ( fun arg -> if !comma then pr ", "; @@ -3183,15 +3196,18 @@ let rec print_c_arg_list ?(handle = false) args = | BytesPersistIn (n, len) -> pr "const void *%s, size_t %s" n len | BytesOut (n, len) | BytesPersistOut (n, len) -> pr "void *%s, size_t %s" n len - | Callback (n, args) - | CallbackPersist (n, args) -> - pr "int (*%s) " n; print_c_arg_list args + | Closure (_, cls) -> + pr "void *user_data"; + List.iter ( + fun { cbname; cbargs } -> + pr ", int (*%s) " cbname; + print_c_arg_list ~user_data:true cbargs + ) cls | Flags n -> pr "uint32_t %s" n | Int n -> pr "int %s" n | Int64 n -> pr "int64_t %s" n | Mutable (Int n) -> pr "int *%s" n | Mutable arg -> assert false - | Opaque n -> pr "void *%s" n | Path n | String n -> pr "const char *%s" n | StringList n -> pr "char **%s" n @@ -3258,7 +3274,7 @@ let generate_include_libnbd_h () = pr "\n"; pr "struct nbd_handle;\n"; pr "\n"; - pr "typedef void (*nbd_close_callback) (void *data);\n"; + pr "typedef void (*nbd_close_callback) (void *user_data);\n"; pr "\n"; List.iter (fun (n, i) -> pr "#define LIBNBD_%-30s %d\n" n i) constants; pr "\n"; @@ -3275,7 +3291,8 @@ let generate_include_libnbd_h () = 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, void *data);\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 ( @@ -3386,7 +3403,7 @@ let generate_lib_api_c () = pr " }\n" ); pr " ret = nbd_unlocked_%s (h" name; - let argnames = List.flatten (List.map name_of_arg args) in + let argnames = List.flatten (List.map c_name_of_arg args) in List.iter (pr ", %s") argnames; pr ");\n"; if permitted_states <> [] then @@ -3560,9 +3577,9 @@ 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 *data); + typedef void (*nbd_close_callback) (void *user_data); int nbd_add_close_callback (struct nbd_handle *nbd, - nbd_close_callback cb, void *data); + nbd_close_callback cb, void *user_data); "; @@ -3709,159 +3726,160 @@ PyInit_libnbdmod (void) " let print_python_binding name { args; ret } = - (* Functions with a callback parameter are special because we - * have to generate a wrapper function which translates the - * callback parameters back to Python. + (* Functions with a Closure parameter are special because we + * have to generate wrapper functions which translate the + * callbacks back to Python. *) - let find_callback opaque_id = - let cb = - try - List.find ( - function - | Callback (_, args) | CallbackPersist (_, args) -> - List.mem (Opaque opaque_id) args - | _ -> false - ) args - with - Not_found -> - failwithf "%s: couldn't find callback associated with Opaque %s" - name opaque_id in - match cb with - | Callback (name, _) | CallbackPersist (name, _) -> name - | _ -> assert false - in - List.iter ( function - | Callback (cb_name, args) | CallbackPersist (cb_name, args) -> - pr "struct %s_%s_data {\n" name cb_name; - pr " PyObject *fn;\n"; - pr " PyObject *data;\n"; + | Closure (persistent, cls) -> + pr "struct %s_user_data {\n" name; + List.iter ( + fun { cbname } -> + pr " PyObject *%s;\n" cbname + ) cls; pr "};\n"; pr "\n"; - pr "static int\n"; - pr "%s_%s_wrapper " name cb_name; - print_c_arg_list args; - 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; - | Opaque n -> - pr " struct %s_%s_data *_data = %s;\n" name cb_name n - | String n - | UInt64 n -> () - (* The following not yet implemented for callbacks XXX *) - | ArrayAndLen _ | Bool _ | BytesOut _ - | BytesPersistIn _ | BytesPersistOut _ - | Callback _ | CallbackPersist _ - | Flags _ | Mutable _ - | Path _ | SockAddrAndLen _ | StringList _ - | UInt _ | UInt32 _ -> assert false - ) args; - 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\"" - | Opaque n -> pr " \"O\"" - | String n -> pr " \"s\"" - | UInt64 n -> pr " \"K\"" - (* The following not yet implemented for callbacks XXX *) - | ArrayAndLen _ | Bool _ | BytesOut _ - | BytesPersistIn _ | BytesPersistOut _ - | Callback _ | CallbackPersist _ - | Flags _ | Mutable _ - | Path _ | SockAddrAndLen _ | StringList _ - | UInt _ | UInt32 _ -> assert false - ) args; - 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 - | Opaque _ -> pr ", _data->data" - | Int n | Int64 n - | String n - | UInt64 n -> pr ", %s" n - (* The following not yet implemented for callbacks XXX *) - | ArrayAndLen _ | Bool _ | BytesOut _ - | BytesPersistIn _ | BytesPersistOut _ - | Callback _ | CallbackPersist _ - | Flags _ | Mutable _ - | Path _ | SockAddrAndLen _ | StringList _ - | UInt _ | UInt32 _ -> assert false - ) args; - 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 (_data->fn, 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"; + (* 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 "{\n"; + pr " struct %s_user_data *user_data = vp;\n" name; + pr "\n"; + List.iter ( + fun { cbname } -> + pr " Py_DECREF (user_data->%s);\n" cbname + ) cls; + pr " free (user_data);\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 _ - | Opaque _ - | String _ - | UInt64 _ -> () - (* The following not yet implemented for callbacks XXX *) - | ArrayAndLen _ | Bool _ | BytesOut _ - | BytesPersistIn _ | BytesPersistOut _ - | Callback _ | CallbackPersist _ - | Flags _ | Mutable _ - | Path _ | SockAddrAndLen _ | StringList _ - | UInt _ | UInt32 _ -> assert false - ) args; - pr " return ret;\n"; - pr "}\n"; - pr "\n" + fun { cbname; cbargs } -> + pr "static int\n"; + pr "%s_%s_wrapper " name cbname; + print_c_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"; + + 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 | _ -> () ) args; @@ -3899,10 +3917,11 @@ 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 - | Callback (n, _) -> - pr " struct %s_%s_data _%s_data, *%s_data = &_%s_data;\n" name n n n n - | CallbackPersist (n, _) -> - pr " struct %s_%s_data *%s_data;\n" name 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 | Flags n -> pr " uint32_t %s_u32;\n" n; pr " unsigned int %s; /* really uint32_t */\n" n @@ -3911,8 +3930,7 @@ let print_python_binding name { args; ret } = pr " int64_t %s_i64;\n" n; pr " long long %s; /* really int64_t */\n" n | Mutable arg -> - pr " PyObject *%s;\n" (List.hd (name_of_arg arg)) - | Opaque _ -> () + pr " PyObject *%s;\n" (List.hd (c_name_of_arg arg)) | Path n -> pr " PyObject *py_%s = NULL;\n" n; pr " char *%s = NULL;\n" n @@ -3935,34 +3953,17 @@ let print_python_binding name { args; ret } = ) args; pr "\n"; - (* Allocate the parameter. *) + (* Allocate the persistent closure user_data. *) List.iter ( function - | ArrayAndLen _ - | Bool _ - | BytesIn _ - | BytesPersistIn _ - | BytesOut _ - | BytesPersistOut _ - | Callback _ -> () - | CallbackPersist (n, _) -> - pr " %s_data = malloc (sizeof *%s_data);\n" n n; - pr " if (%s_data == NULL) {\n" n; + | 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" - | Flags _ - | Int _ - | Int64 _ - | Opaque _ - | Mutable _ - | Path _ - | SockAddrAndLen _ - | String _ - | StringList _ - | UInt _ - | UInt32 _ - | UInt64 _ -> () + | _ -> () ) args; (* Parse the Python parameters. *) @@ -3975,12 +3976,11 @@ let print_python_binding name { args; ret } = | BytesPersistIn (n, _) -> pr " \"O\"" | BytesOut (_, count) -> pr " \"n\"" | BytesPersistOut (_, count) -> pr " \"O\"" - | Callback (n, _) | CallbackPersist (n, _) -> pr " \"O\"" + | Closure (_, cls) -> List.iter (fun _ -> pr " \"O\"") cls | Flags n -> pr " \"I\"" | Int n -> pr " \"i\"" | Int64 n -> pr " \"L\"" | Mutable _ -> pr " \"O\"" - | Opaque _ -> pr " \"O\"" | Path n -> pr " \"O&\"" | SockAddrAndLen (n, _) -> pr " \"O\"" | String n -> pr " \"s\"" @@ -4000,12 +4000,12 @@ let print_python_binding name { args; ret } = | BytesIn (n, _) | BytesPersistIn (n, _) | BytesPersistOut (n, _) -> pr ", &%s" n | BytesOut (_, count) -> pr ", &%s" count - | Callback (n, _) | CallbackPersist (n, _) -> pr ", &%s_data->fn" n + | Closure (_, cls) -> + List.iter (fun { cbname } -> pr ", &user_data->%s" cbname) cls | Flags n -> pr ", &%s" n | Int n -> pr ", &%s" n | Int64 n -> pr ", &%s" n - | Mutable arg -> pr ", &%s" (List.hd (name_of_arg arg)) - | Opaque n -> pr ", &%s_data->data" (find_callback n) + | Mutable arg -> pr ", &%s" (List.hd (c_name_of_arg arg)) | Path n -> pr ", PyUnicode_FSConverter, &py_%s" n | SockAddrAndLen (n, _) -> pr ", &%s" n | String n -> pr ", &%s" n @@ -4017,6 +4017,20 @@ 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, cls) -> + List.iter ( + fun { cbname } -> + pr " Py_INCREF (user_data->%s);\n" cbname + ) cls + | _ -> () + ) args; + pr " h = get_handle (py_h);\n"; List.iter ( function @@ -4044,19 +4058,20 @@ 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 - | Callback (n, _) | CallbackPersist (n, _) -> - pr " if (!PyCallable_Check (%s_data->fn)) {\n" n; - pr " PyErr_SetString (PyExc_TypeError,\n"; - pr " \"callback parameter %s is not callable\");\n" - n; - pr " return NULL;\n"; - pr " }\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 | Flags n -> pr " %s_u32 = %s;\n" n n | Int _ -> () | Int64 n -> pr " %s_i64 = %s;\n" n n | Mutable _ -> pr " abort (); /* Mutable for normal Python parameters not impl */\n" - | Opaque n -> () | Path n -> pr " %s = PyBytes_AS_STRING (py_%s);\n" n n; pr " assert (%s != NULL);\n" n @@ -4082,12 +4097,16 @@ 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 - | Callback (n, _) | CallbackPersist (n, _) -> pr ", %s_%s_wrapper" name n + | Closure (_, cls) -> + pr ", user_data"; + List.iter ( + fun { cbname } -> + pr ", %s_%s_wrapper" name cbname + ) cls | Flags n -> pr ", %s_u32" n | Int n -> pr ", %s" n | Int64 n -> pr ", %s_i64" n - | Mutable arg -> pr ", /* XXX */ (void *) %s" (List.hd (name_of_arg arg)) - | Opaque n -> pr ", %s_data" (find_callback n) + | Mutable arg -> pr ", /* XXX */ (void *) %s" (List.hd (c_name_of_arg arg)) | Path n -> pr ", %s" n | SockAddrAndLen (n, _) -> pr ", /* XXX */ (void *) %s, 0" n | String n -> pr ", %s" n @@ -4117,12 +4136,11 @@ let print_python_binding name { args; ret } = | Bool _ | BytesIn _ | BytesPersistIn _ | BytesPersistOut _ - | Callback _ | CallbackPersist _ + | Closure _ | Flags _ | Int _ | Int64 _ | Mutable _ - | Opaque _ | Path _ | SockAddrAndLen _ | String _ @@ -4160,15 +4178,14 @@ let print_python_binding name { args; ret } = | Bool _ -> () | BytesIn (n, _) -> pr " PyBuffer_Release (&%s);\n" n | BytesPersistIn _ | BytesOut _ | BytesPersistOut _ -> () - | Callback _ -> () - | CallbackPersist (n, _) -> + | Closure (false, _) -> () + | Closure (true, _) -> pr " /* This ensures the callback data is freed eventually. */\n"; - pr " nbd_add_close_callback (h, free, %s_data);\n" n + pr " nbd_add_close_callback (h, free_%s_user_data, user_data);\n" name | Flags _ -> () | Int _ -> () | Int64 _ -> () | Mutable _ -> () - | Opaque _ -> () | Path n -> pr " Py_XDECREF (py_%s);\n" n | SockAddrAndLen _ -> () @@ -4302,28 +4319,30 @@ class NBD (object): List.iter ( fun (name, { args; shortdesc; longdesc }) -> - let args = List.map ( - function - | ArrayAndLen (UInt32 n, _) -> n, None - | ArrayAndLen _ -> assert false - | Bool n -> n, None - | BytesIn (n, _) | BytesPersistIn (n, _) -> n, None - | BytesPersistOut (n, _) -> n, None - | BytesOut (_, count) -> count, None - | Callback (n, _) | CallbackPersist (n, _) -> n, None - | Flags n -> n, Some "0" - | Int n -> n, None - | Int64 n -> n, None - | Mutable arg -> List.hd (name_of_arg arg), None - | Opaque n -> n, None - | Path n -> n, None - | SockAddrAndLen (n, _) -> n, None - | String n -> n, None - | StringList n -> n, None - | UInt n -> n, None - | UInt32 n -> n, None - | UInt64 n -> n, None - ) args in + let args = + List.map ( + function + | ArrayAndLen (UInt32 n, _) -> [n, None] + | ArrayAndLen _ -> assert false + | Bool n -> [n, None] + | BytesIn (n, _) | BytesPersistIn (n, _) -> [n, None] + | BytesPersistOut (n, _) -> [n, None] + | BytesOut (_, count) -> [count, None] + | Closure (_, cls) -> + List.map (fun { cbname } -> cbname, None) cls + | Flags n -> [n, Some "0"] + | Int n -> [n, None] + | Int64 n -> [n, None] + | Mutable arg -> [List.hd (c_name_of_arg arg), None] + | Path n -> [n, None] + | SockAddrAndLen (n, _) -> [n, None] + | String n -> [n, None] + | StringList n -> [n, None] + | UInt n -> [n, None] + | UInt32 n -> [n, None] + | UInt64 n -> [n, None] + ) args in + let args = List.flatten args in let () = let args = List.map ( function @@ -4357,6 +4376,7 @@ if __name__ == \"__main__\": nbdsh.shell() " +(* (*----------------------------------------------------------------------*) (* OCaml bindings. *) @@ -4427,7 +4447,7 @@ and ocaml_ret_to_string = function let name_of_ocaml_arg = function | OCamlHandle -> "h" | OCamlFlags n -> n - | OCamlArg a -> List.hd (name_of_arg a) + | OCamlArg a -> List.hd (c_name_of_arg a) let generate_ocaml_nbd_mli () = generate_header OCamlStyle; @@ -4686,7 +4706,7 @@ let print_ocaml_binding (name, { args; ret }) = pr " int ret;\n"; pr "\n"; pr " caml_leave_blocking_section ();\n"; - let c_argnames = List.flatten (List.map name_of_arg args) in + let c_argnames = List.flatten (List.map c_name_of_arg args) in pr " ret = %s_%s_wrapper_locked (%s);\n" name cb_name (String.concat ", " c_argnames); pr " caml_enter_blocking_section ();\n"; @@ -4845,7 +4865,7 @@ let print_ocaml_binding (name, { args; ret }) = pr " r = nbd_%s (h%s);\n" name (String.concat "" (List.map ((^) ", ") - (List.flatten (List.map name_of_arg args)))); + (List.flatten (List.map c_name_of_arg args)))); pr " caml_leave_blocking_section ();\n"; pr "\n"; pr " if (r == %s)\n" errcode; @@ -4938,6 +4958,7 @@ let generate_ocaml_nbd_c () = pr "\n"; List.iter print_ocaml_binding handle_calls +*) (*----------------------------------------------------------------------*) @@ -4954,6 +4975,8 @@ let () = output_to "python/libnbdmod.c" generate_python_libnbdmod_c; output_to "python/methods.c" generate_python_methods_c; output_to "python/nbd.py" generate_python_nbd_py; +(* output_to "ocaml/NBD.mli" generate_ocaml_nbd_mli; output_to "ocaml/NBD.ml" generate_ocaml_nbd_ml; output_to "ocaml/nbd-c.c" generate_ocaml_nbd_c; + *) diff --git a/generator/states-reply-simple.c b/generator/states-reply-simple.c index ba26f8c..72a401b 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.opaque, cmd->data, cmd->count, + if (cmd->cb.fn.read (cmd->cb.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 5c4d2f2..3168d1b 100644 --- a/generator/states-reply-structured.c +++ b/generator/states-reply-structured.c @@ -298,7 +298,8 @@ * 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.opaque, cmd->data + (offset - cmd->offset), + if (cmd->cb.fn.read (cmd->cb.user_data, + cmd->data + (offset - cmd->offset), 0, offset, LIBNBD_READ_ERROR, &scratch) == -1) if (cmd->error == 0) cmd->error = scratch; @@ -384,7 +385,8 @@ if (cmd->cb.fn.read) { int error = cmd->error; - if (cmd->cb.fn.read (cmd->cb.opaque, cmd->data + (offset - cmd->offset), + if (cmd->cb.fn.read (cmd->cb.user_data, + cmd->data + (offset - cmd->offset), length - sizeof offset, offset, LIBNBD_READ_DATA, &error) == -1) if (cmd->error == 0) @@ -445,7 +447,8 @@ if (cmd->cb.fn.read) { int error = cmd->error; - if (cmd->cb.fn.read (cmd->cb.opaque, cmd->data + offset, length, + if (cmd->cb.fn.read (cmd->cb.user_data, + cmd->data + offset, length, cmd->offset + offset, LIBNBD_READ_HOLE, &error) == -1) if (cmd->error == 0) @@ -496,7 +499,8 @@ /* Call the caller's extent function. */ int error = cmd->error; - if (cmd->cb.fn.extent (cmd->cb.opaque, meta_context->name, cmd->offset, + if (cmd->cb.fn.extent (cmd->cb.user_data, + meta_context->name, cmd->offset, &h->bs_entries[1], (length-4) / 4, &error) == -1) if (cmd->error == 0) cmd->error = error ? error : EPROTO; diff --git a/generator/states-reply.c b/generator/states-reply.c index 742fc1a..ebdbdc1 100644 --- a/generator/states-reply.c +++ b/generator/states-reply.c @@ -184,7 +184,7 @@ save_reply_state (struct nbd_handle *h) if (cmd->cb.callback) { int error = cmd->error; - if (cmd->cb.callback (cmd->cb.opaque, cookie, &error) == -1 && error) + if (cmd->cb.callback (cmd->cb.user_data, cookie, &error) == -1 && error) cmd->error = error; } diff --git a/generator/states.c b/generator/states.c index 992f833..93ad4e5 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.opaque, cmd->cookie, &error) == -1 && error) + if (cmd->cb.callback (cmd->cb.user_data, cmd->cookie, + &error) == -1 && error) cmd->error = error; } if (cmd->error == 0) diff --git a/lib/handle.c b/lib/handle.c index cbe7e8a..5003227 100644 --- a/lib/handle.c +++ b/lib/handle.c @@ -98,7 +98,7 @@ nbd_close (struct nbd_handle *h) for (cc = h->close_callbacks; cc != NULL; cc = cc_next) { cc_next = cc->next; - cc->cb (cc->data); + cc->cb (cc->user_data); free (cc); } @@ -202,7 +202,8 @@ nbd_unlocked_add_meta_context (struct nbd_handle *h, const char *name) * programming languages. */ int -nbd_add_close_callback (struct nbd_handle *h, nbd_close_callback cb, void *data) +nbd_add_close_callback (struct nbd_handle *h, + nbd_close_callback cb, void *user_data) { int ret; struct close_callback *cc; @@ -216,7 +217,7 @@ nbd_add_close_callback (struct nbd_handle *h, nbd_close_callback cb, void *data) } cc->next = h->close_callbacks; cc->cb = cb; - cc->data = data; + cc->user_data = user_data; h->close_callbacks = cc; ret = 0; diff --git a/lib/internal.h b/lib/internal.h index 44272b9..347bf69 100644 --- a/lib/internal.h +++ b/lib/internal.h @@ -212,7 +212,7 @@ struct meta_context { struct close_callback { struct close_callback *next; /* Linked list. */ nbd_close_callback cb; /* Function. */ - void *data; /* Data. */ + void *user_data; /* Data. */ }; struct socket_ops { @@ -241,14 +241,15 @@ struct socket { const struct socket_ops *ops; }; -typedef int (*extent_fn) (void *data, const char *metacontext, uint64_t offset, +typedef int (*extent_fn) (void *user_data, + const char *metacontext, uint64_t offset, uint32_t *entries, size_t nr_entries, int *error); -typedef int (*read_fn) (void *data, const void *buf, size_t count, +typedef int (*read_fn) (void *user_data, const void *buf, size_t count, uint64_t offset, int status, int *error); -typedef int (*callback_fn) (void *data, int64_t cookie, int *error); +typedef int (*callback_fn) (void *user_data, int64_t cookie, int *error); struct command_cb { - void *opaque; + void *user_data; union { extent_fn extent; read_fn read; diff --git a/lib/rw.c b/lib/rw.c index a878fea..f2fe4e0 100644 --- a/lib/rw.c +++ b/lib/rw.c @@ -60,12 +60,12 @@ nbd_unlocked_pread (struct nbd_handle *h, void *buf, int nbd_unlocked_pread_structured (struct nbd_handle *h, void *buf, size_t count, uint64_t offset, - void *opaque, read_fn read, uint32_t flags) + void *user_data, read_fn read, uint32_t flags) { int64_t ch; ch = nbd_unlocked_aio_pread_structured (h, buf, count, offset, - opaque, read, flags); + user_data, read, flags); if (ch == -1) return -1; @@ -145,11 +145,12 @@ nbd_unlocked_zero (struct nbd_handle *h, int nbd_unlocked_block_status (struct nbd_handle *h, uint64_t count, uint64_t offset, - void *data, extent_fn extent, uint32_t flags) + void *user_data, extent_fn extent, uint32_t flags) { int64_t ch; - ch = nbd_unlocked_aio_block_status (h, count, offset, data, extent, flags); + ch = nbd_unlocked_aio_block_status (h, count, offset, user_data, extent, + flags); if (ch == -1) return -1; @@ -159,8 +160,8 @@ nbd_unlocked_block_status (struct nbd_handle *h, int64_t nbd_internal_command_common (struct nbd_handle *h, uint16_t flags, uint16_t type, - uint64_t offset, uint64_t count, void *data, - struct command_cb *cb) + uint64_t offset, uint64_t count, + void *data, struct command_cb *cb) { struct command_in_flight *cmd, *prev_cmd; @@ -256,10 +257,10 @@ nbd_unlocked_aio_pread (struct nbd_handle *h, void *buf, int64_t nbd_unlocked_aio_pread_callback (struct nbd_handle *h, void *buf, size_t count, uint64_t offset, - void *opaque, callback_fn callback, + void *user_data, callback_fn callback, uint32_t flags) { - struct command_cb cb = { .opaque = opaque, .callback = callback, }; + struct command_cb cb = { .user_data = user_data, .callback = callback, }; /* We could silently accept flag DF, but it really only makes sense * with callbacks, because otherwise there is no observable change @@ -277,20 +278,22 @@ nbd_unlocked_aio_pread_callback (struct nbd_handle *h, void *buf, int64_t nbd_unlocked_aio_pread_structured (struct nbd_handle *h, void *buf, size_t count, uint64_t offset, - void *opaque, read_fn read, uint32_t flags) + void *user_data, read_fn read, + uint32_t flags) { return nbd_unlocked_aio_pread_structured_callback (h, buf, count, offset, - opaque, read, NULL, flags); + user_data, read, NULL, + flags); } int64_t nbd_unlocked_aio_pread_structured_callback (struct nbd_handle *h, void *buf, size_t count, uint64_t offset, - void *opaque, read_fn read, + void *user_data, read_fn read, callback_fn callback, uint32_t flags) { - struct command_cb cb = { .opaque = opaque, .fn.read = read, + struct command_cb cb = { .user_data = user_data, .fn.read = read, .callback = callback, }; if ((flags & ~LIBNBD_CMD_FLAG_DF) != 0) { @@ -320,10 +323,10 @@ nbd_unlocked_aio_pwrite (struct nbd_handle *h, const void *buf, int64_t nbd_unlocked_aio_pwrite_callback (struct nbd_handle *h, const void *buf, size_t count, uint64_t offset, - void *opaque, callback_fn callback, + void *user_data, callback_fn callback, uint32_t flags) { - struct command_cb cb = { .opaque = opaque, .callback = callback, }; + struct command_cb cb = { .user_data = user_data, .callback = callback, }; if (nbd_unlocked_read_only (h) == 1) { set_error (EINVAL, "server does not support write operations"); @@ -352,10 +355,10 @@ nbd_unlocked_aio_flush (struct nbd_handle *h, uint32_t flags) } int64_t -nbd_unlocked_aio_flush_callback (struct nbd_handle *h, void *opaque, +nbd_unlocked_aio_flush_callback (struct nbd_handle *h, void *user_data, callback_fn callback, uint32_t flags) { - struct command_cb cb = { .opaque = opaque, .callback = callback, }; + struct command_cb cb = { .user_data = user_data, .callback = callback, }; if (nbd_unlocked_can_flush (h) != 1) { set_error (EINVAL, "server does not support flush operations"); @@ -382,10 +385,10 @@ nbd_unlocked_aio_trim (struct nbd_handle *h, int64_t nbd_unlocked_aio_trim_callback (struct nbd_handle *h, uint64_t count, uint64_t offset, - void *opaque, callback_fn callback, + void *user_data, callback_fn callback, uint32_t flags) { - struct command_cb cb = { .opaque = opaque, .callback = callback, }; + struct command_cb cb = { .user_data = user_data, .callback = callback, }; if (nbd_unlocked_read_only (h) == 1) { set_error (EINVAL, "server does not support write operations"); @@ -422,10 +425,10 @@ nbd_unlocked_aio_cache (struct nbd_handle *h, int64_t nbd_unlocked_aio_cache_callback (struct nbd_handle *h, uint64_t count, uint64_t offset, - void *opaque, callback_fn callback, + void *user_data, callback_fn callback, uint32_t flags) { - struct command_cb cb = { .opaque = opaque, .callback = callback, }; + struct command_cb cb = { .user_data = user_data, .callback = callback, }; /* Actually according to the NBD protocol document, servers do exist * that support NBD_CMD_CACHE but don't advertise the @@ -456,10 +459,10 @@ nbd_unlocked_aio_zero (struct nbd_handle *h, int64_t nbd_unlocked_aio_zero_callback (struct nbd_handle *h, uint64_t count, uint64_t offset, - void *opaque, callback_fn callback, + void *user_data, callback_fn callback, uint32_t flags) { - struct command_cb cb = { .opaque = opaque, .callback = callback, }; + struct command_cb cb = { .user_data = user_data, .callback = callback, }; if (nbd_unlocked_read_only (h) == 1) { set_error (EINVAL, "server does not support write operations"); @@ -489,20 +492,21 @@ nbd_unlocked_aio_zero_callback (struct nbd_handle *h, int64_t nbd_unlocked_aio_block_status (struct nbd_handle *h, uint64_t count, uint64_t offset, - void *data, extent_fn extent, + void *user_data, extent_fn extent, uint32_t flags) { - return nbd_unlocked_aio_block_status_callback (h, count, offset, data, extent, + return nbd_unlocked_aio_block_status_callback (h, count, offset, + user_data, extent, NULL, flags); } int64_t nbd_unlocked_aio_block_status_callback (struct nbd_handle *h, uint64_t count, uint64_t offset, - void *data, extent_fn extent, + void *user_data, extent_fn extent, callback_fn callback, uint32_t flags) { - struct command_cb cb = { .opaque = data, .fn.extent = extent, + struct command_cb cb = { .user_data = user_data, .fn.extent = extent, .callback = callback, }; if (!h->structured_replies) { diff --git a/python/t/405-pread-structured.py b/python/t/405-pread-structured.py index 9068a9a..49499b8 100644 --- a/python/t/405-pread-structured.py +++ b/python/t/405-pread-structured.py @@ -24,28 +24,30 @@ h.connect_command (["nbdkit", "-s", "--exit-with-parent", "-v", expected = b'\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x08\x00\x00\x00\x00\x00\x00\x00\x10\x00\x00\x00\x00\x00\x00\x00\x18\x00\x00\x00\x00\x00\x00\x00 \x00\x00\x00\x00\x00\x00\x00(\x00\x00\x00\x00\x00\x00\x000\x00\x00\x00\x00\x00\x00\x008\x00\x00\x00\x00\x00\x00\x00@\x00\x00\x00\x00\x00\x00\x00H\x00\x00\x00\x00\x00\x00\x00P\x00\x00\x00\x00\x00\x00\x00X\x00\x00\x00\x00\x00\x00\x00`\x00\x00\x00\x00\x00\x00\x00h\x00\x00\x00\x00\x00\x00\x00p\x00\x00\x00\x00\x00\x00\x00x\x00\x00\x00\x00\x00\x00\x00\x80\x00\x00\x00\x00\x00\x00\x00\x88\x00\x00\x00\x00\x00\x00\x00\x90\x00\x00\x00\x00\x00\x00\x00\x98\x00\x00\x00\x00\x00\x00\x00\xa0\x00\x00\x00\x00\x00\x00\x00\xa8\x00\x00\x00\x00\x00\x00\x00\xb0\x00\x00\x00\x00\x00\x00\x00\xb8\x00\x00\x00\x00\x00\x00\x00\xc0\x00\x00\x00\x00\x00\x00\x00\xc8\x00\x00\x00\x00\x00\x00\x00\xd0\x00\x00\x00\x00\x00\x00\x00\xd8\x00\x00\x00\x00\x00\x00\x00\xe0\x00\x00\x00\x00\x00\x00\x00\xe8\x00\x00\x00\x00\x00\x00\x00\xf0\x00\x00\x00\x00\x00\x00\x00\xf8\x00\x00\x00\x00\x00\x00\x01\x00\x00\x00\x00\x00\x00\x00\x01\x08\x00\x00\x00\x00\x00\x00\x01\x10\x00\x00\x00\x00\x00\x00\x01\x18\x00\x00\x00\x00\x00\x00\x01 \x00\x00\x00\x00\x00\x00\x01(\x00\x00\x00\x00\x00\x00\x010\x00\x00\x00\x00\x00\x00\x018\x00\x00\x00\x00\x00\x00\x01@\x00\x00\x00\x00\x00\x00\x01H\x00\x00\x00\x00\x00\x00\x01P\x00\x00\x00\x00\x00\x00\x01X\x00\x00\x00\x00\x00\x00\x01`\x00\x00\x00\x00\x00\x00\x01h\x00\x00\x00\x00\x00\x00\x01p\x00\x00\x00\x00\x00\x00\x01x\x00\x00\x00\x00\x00\x00\x01\x80\x00\x00\x00\x00\x00\x00\x01\x88\x00\x00\x00\x00\x00\x00\x01\x90\x00\x00\x00\x00\x00\x00\x01\x98\x00\x00\x00\x00\x00\x00\x01\xa0\x00\x00\x00\x00\x00\x00\x01\xa8\x00\x00\x00\x00\x00\x00\x01\xb0\x00\x00\x00\x00\x00\x00\x01\xb8\x00\x00\x00\x00\x00\x00\x01\xc0\x00\x00\x00\x00\x00\x00\x01\xc8\x00\x00\x00\x00\x00\x00\x01\xd0\x00\x00\x00\x00\x00\x00\x01\xd8\x00\x00\x00\x00\x00\x00\x01\xe0\x00\x00\x00\x00\x00\x00\x01\xe8\x00\x00\x00\x00\x00\x00\x01\xf0\x00\x00\x00\x00\x00\x00\x01\xf8' -def f (data, buf2, offset, s, err): +def f (user_data, buf2, offset, s, err): assert err.value == 0 err.value = errno.EPROTO - assert data == 42 + assert user_data == 42 assert buf2 == expected assert offset == 0 assert s == nbd.READ_DATA -buf = h.pread_structured (512, 0, 42, f) +buf = h.pread_structured (512, 0, lambda *args: f (42, *args)) print ("%r" % buf) assert buf == expected -buf = h.pread_structured (512, 0, 42, f, nbd.CMD_FLAG_DF) +buf = h.pread_structured (512, 0, lambda *args: f (42, *args), + nbd.CMD_FLAG_DF) print ("%r" % buf) assert buf == expected try: - buf = h.pread_structured (512, 0, 43, f, nbd.CMD_FLAG_DF) + buf = h.pread_structured (512, 0, lambda *args: f (43, *args), + nbd.CMD_FLAG_DF) assert False except nbd.Error as ex: assert ex.errnum == errno.EPROTO diff --git a/python/t/460-block-status.py b/python/t/460-block-status.py index 7e4ba2c..78c64f2 100644 --- a/python/t/460-block-status.py +++ b/python/t/460-block-status.py @@ -27,26 +27,27 @@ h.connect_command (["nbdkit", "-s", "--exit-with-parent", "-v", "sh", script]) entries = [] -def f (data, metacontext, offset, e, err): +def f (user_data, metacontext, offset, e, err): global entries - assert data == 42 + assert user_data == 42 assert err.value == 0 if metacontext != "base:allocation": return entries = e -h.block_status (65536, 0, 42, f) +h.block_status (65536, 0, lambda *args: f (42, *args)) assert entries == [ 8192, 0, 8192, 1, 16384, 3, 16384, 2, 16384, 0] -h.block_status (1024, 32256, 42, f) +h.block_status (1024, 32256, lambda *args: f (42, *args)) print ("entries = %r" % entries) assert entries == [ 512, 3, 16384, 2] -h.block_status (1024, 32256, 42, f, nbd.CMD_FLAG_REQ_ONE) +h.block_status (1024, 32256, lambda *args: f (42, *args), + nbd.CMD_FLAG_REQ_ONE) print ("entries = %r" % entries) assert entries == [ 512, 3] diff --git a/python/t/600-debug-callback.py b/python/t/600-debug-callback.py index 40cbd8f..cc03346 100644 --- a/python/t/600-debug-callback.py +++ b/python/t/600-debug-callback.py @@ -26,7 +26,7 @@ def f(id, context, msg): assert id == 42 messages.append (msg) -h.set_debug_callback (42, f) +h.set_debug_callback (lambda *args: f (42, *args)) h.connect_command (["nbdkit", "-s", "--exit-with-parent", "null"]) h.shutdown () -- 2.22.0

On Tue, Jul 16, 2019 at 09:03:55AM -0500, Eric Blake wrote: Yeah I actually hit this in the revised test case that uses lambdas, because it turns out that lambdas in Python are refcounted (but staticly defined functions are not). Actually I don't think there's a memory leak here because Closure only occurs once in the argument list (I have added a check for this based on your comment). _However_ there is certainly a more general problem that the Python bindings don't carefully clean up allocations on error paths. attribute((cleanup)) doesn't help in this particular case (but would in others) since we need the malloc to persist after the call. Py_INCREF is only done for persistent closures, so the loops are slightly different (note ‘Closure (false, _) -> ()’ in the false case in the first loop). We could combine them into one loop with a test of the persistent boolean, but heh, loops are cheap in the generator :-) Yes, this is one of the many ways that the Python code will leak on error paths. Yes it _should_ be fixed by this patch. I have corrected all the stuff above (except for the general memory leaks on error) and have pushed a version which includes also OCaml changes. I will prepare a new patch soon for fixing nbd_add_close_callback. Thanks, Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com libguestfs lets you edit virtual machines. Supports shell scripting, bindings from many languages. http://libguestfs.org