Re: [Libguestfs] [PATCH libnbd 1/3] generator: Change Closure so it describes single callbacks.

On 7/24/19 7:17 AM, Richard W.M. Jones wrote: > 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 Should we tweak this to explicitly call out "Callbacks in the C language", to differentiate it from bindings to other languages that do not do this?

> -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. > > +++ 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 *) Do we still need the closure record type, or can/should we simplify further to: | Closure of bool * string * arg list (and later in the series, even further by dropping 'bool *')

> | 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"] }) ]; Keeping the record type means that you still have to provide names, vs. directly using tuples where we'd write: args = [ Closure (true, "debug_fn", [String "context"; String "msg"]) ];

> @@ -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. Comment needs a touchup now that there is no user_data struct.

> + 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 _ Not your usual indentation style.