similar to: [libnbd PATCH] hack for testing python closure leaks: NOT FOR COMMIT

h.nbd_connect_command was leaking a python object per parameter, and also leaks memory on failure. In fact, it was possible to segfault on something as trivial as: nbdsh -c 'h.connect_command(["true",1])' h.nbd_pread was leaking a read buffer on every single synchronous read. My previous patch to address closure callbacks had a bug in h.nbd_pread_structured and similar: if the

Inspection of the generated code showed several places where we did not release references on all error paths. In particular, switching things to always jump to an out: label, even when the underlying C function cannot fail, makes it easier to clean up when the user passes wrong types to a function call. One of the easiest triggers without this patch was this one-liner: $ nbdsh -c

Same as the previous commit, but for Python. --- generator/generator | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/generator/generator b/generator/generator index 109fad6..a0322ee 100755 --- a/generator/generator +++ b/generator/generator @@ -4315,9 +4315,6 @@ let print_python_binding name { args; optargs; ret; may_set_error } = ) cbargs; pr

By using the completion free callback function we don't need to manually free the buffer. Note that after this patch OClosures are always called from OCaml, because we might need the side effect of freeing the buffer. This reverts part of commit fef1c281a65d061127bf178e5f8cfca0a2475c56. --- generator/generator | 59 +++++++++++++++++++++----------- ocaml/buffer.c

--- generator/generator | 31 ++++++++++++++++++++++++++++--- 1 file changed, 28 insertions(+), 3 deletions(-) diff --git a/generator/generator b/generator/generator index f6a4856..9bcb08d 100755 --- a/generator/generator +++ b/generator/generator @@ -3982,6 +3982,13 @@ let print_python_closure_wrapper { cbname; cbargs } = pr " const struct user_data *data = user_data;\n"; pr

We were previously generating one instance of the Python closure wrapper per (function * Closure arg). However these wrappers didn't actually differ across functions. We can therefore save a lot of code by only generating one wrapper per closure globally. This reduces the amount of generated code by nearly 25%. Before and after: $ wc -l python/methods.c 3275 python/methods.c $ wc -l

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 +- >

On 7/16/19 6:04 AM, Richard W.M. Jones wrote: > 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

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

Basically the same as this patch series, but for Python: https://www.redhat.com/archives/libguestfs/2019-August/msg00235.html plus adding the 590 asynch test at the end. Rich.

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

If we accept that callbacks will never handle the full range of parameters then we can simplify the generator quite a bit by using a special type for closure args vs normal method args. This removes many asserts and quite a bit of unreachable code (eg. Python code for handling ArrayAndLen in normal methods that was never used). The output of the generator after this commit should be identical.

Mechanical change: Wherever we call any closure with the LIBNBD_CALLBACK_FREE function, we also call nbd_internal_free_callback with the closure's user_data. This allows calls to associate a free callback with any closure via its user_data pointer. --- generator/states-reply-simple.c | 1 + generator/states-reply-structured.c | 24 ++++++++++++++++++------ generator/states-reply.c

On Wed, Jul 24, 2019 at 10:18:20AM -0500, Eric Blake wrote: > 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

Existing practice tends to prefer the void* closure argument for C callbacks to occur after the function pointer (POSIX: pthread_create; glibc: qsort_r; glib: g_thread_new; libvirt: virConnectDomainEventRegisterAny; etc.). It's also handy to think that calling 'myfunc(cb, arg)' will eventually result in a call to 'cb(arg)'. While I couldn't quickly find any style guide

On Wed, Jul 24, 2019 at 10:02:04AM -0500, Eric Blake wrote: > 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

For example nbd_set_debug takes a callback function. Previously this was defined explicitly inside the function parameters. This commit defines a new public typedef: typedef int (*nbd_debug_callback) (unsigned valid_flag, void *user_data, const char *context, const char *msg); and then uses the typedef like this: extern int nbd_set_debug_callback

On 7/24/19 7:17 AM, Richard W.M. Jones wrote: > Previously closures had a crude flag which tells if they are > persistent or transient. Transient closures (flag = false) last for > the lifetime of the currently called libnbd function. Persistent > closures had an indefinite lifetime which could last for as long as > the handle. In language bindings handling persistent closures

This is a combination of these two earlier series: https://www.redhat.com/archives/libguestfs/2019-August/msg00235.html https://www.redhat.com/archives/libguestfs/2019-August/msg00240.html plus changes to allow .callback = NULL / .free != NULL, and to reduce the complexity of freeing callbacks. Although it's rather long there's nothing complex here. We might consider squashing some

Previously closures had a crude flag which tells if they are persistent or transient. Transient closures (flag = false) last for the lifetime of the currently called libnbd function. Persistent closures had an indefinite lifetime which could last for as long as the handle. In language bindings handling persistent closures was wasteful as we needed to register a "close callback" to