Re: [Libguestfs] [PATCH libnbd 2/3] lib: Implement closure lifetimes.

On 7/24/19 7:17 AM, Richard W.M. Jones wrote: > +=head2 Callback lifetimes > + > +All callbacks have an C<int valid_flag> parameter which is used to > +help with the lifetime of the callback. C<valid_flag> contains the > +I<logical or> of: Again, worth mentioning that this explicitly only for the C bindings?

> +=item other bits > + > +Other bits in C<valid_flag> should be ignored. 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.

> +++ b/examples/strict-structured-reads.c > @@ -127,11 +131,14 @@ read_chunk (void *opaque, const void *bufv, size_t count, uint64_t offset, > } > > static int > -read_verify (void *opaque, int64_t cookie, int *error) > +read_verify (int valid_flag, void *opaque, int64_t cookie, int *error) > { > struct data *data = opaque; > int ret = -1; > > + if (!(valid_flag & LIBNBD_CALLBACK_VALID)) > + return 0; ...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).

> @@ -3196,6 +3180,12 @@ let rec print_arg_list ?(handle = false) ?(user_data = false) > if types then pr "struct nbd_handle *"; > pr "h" > ); > + if valid_flag then ( > + if !comma then pr ", "; > + comma := true; > + if types then pr "int "; > + pr "valid_flag"; 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.

> @@ -3228,10 +3218,10 @@ let rec print_arg_list ?(handle = false) ?(user_data = false) > pr "%s, " n; > if types then pr "size_t "; > pr "%s" len > - | Closure (_, { cbname; cbargs }) -> > + | Closure { cbname; cbargs } -> > if types then ( > pr "int (*%s) " cbname; > - print_arg_list ~user_data:true cbargs; > + print_arg_list ~valid_flag:true ~user_data:true cbargs; Indentation.

> + | Closure { cbname; cbargs } -> > pr "/* Wrapper for %s callback of %s. */\n" cbname name; > pr "static int\n"; > pr "%s_%s_wrapper " name cbname; > - C.print_arg_list ~user_data:true cbargs; > + C.print_arg_list ~valid_flag:true ~user_data:true cbargs; > pr "\n"; > pr "{\n"; > + pr " if (valid_flag & LIBNBD_CALLBACK_VALID) {\n"; > pr " int ret;\n"; > pr " PyGILState_STATE py_save = PyGILState_UNLOCKED;\n"; Do we care about the generated indentation looking sane? (That can be a followup patch, to minimize the churn on this one...)

> pr " PyObject *py_args, *py_ret;\n"; > @@ -3927,6 +3907,12 @@ let print_python_binding name { args; ret } = > | UInt _ | UInt32 _ -> assert false > ) cbargs; > pr " return ret;\n"; > + pr " }\n"; Ouch. Calling 'return ret' before checking for LIBNBD_CALLBACK_FREE is liable to leak. I think you have to defer the return...

> + pr "\n"; > + pr " if (valid_flag & LIBNBD_CALLBACK_FREE)\n"; > + pr " Py_DECREF ((PyObject *)user_data);\n"; > + pr "\n"; > + pr " return 0;\n"; ...to here. > @@ -4048,13 +4034,12 @@ let print_python_binding name { args; ret } = > pr "))\n"; > pr " return NULL;\n"; > > - (* If the parameter has persistent closures then we need to > + (* If the parameter has closures then we need to > * make sure the ref count remains positive. > *) > List.iter ( > function > - | Closure (false, _) -> () > - | Closure (true, { cbname }) -> > + | Closure { cbname } -> Now that this match... > pr " Py_INCREF (%s_user_data);\n" cbname > | _ -> () > ) args; > @@ -4086,7 +4071,7 @@ let print_python_binding name { args; ret } = > pr " %s = malloc (%s);\n" n count > | BytesPersistIn (n, _) | BytesPersistOut (n, _) -> > pr " %s_buf = nbd_internal_py_get_aio_buffer (%s);\n" n n > - | Closure (_, { cbname }) -> > + | Closure { cbname } -> > pr " if (!PyCallable_Check (%s_user_data)) {\n" cbname; > pr " PyErr_SetString (PyExc_TypeError,\n"; > pr " \"callback parameter %s is not callable\");\n" cbname; ...and this match are identical, should we group the code?