Re: [Libguestfs] [PATCH libnbd v2] generator: Define new Closure type instead of callbacks.

> + (* 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 Oh, good fix. We were not previously incrementing the ref-count of the Python Callable, and could have ended up deferencing a garbage-collected object.

> + (* 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" Memory leak. If Closure contains two callbacks, and the first malloc() succeeds while the second fails, we are not reclaiming the first. Should our generated python code take advantage of __attribute__((cleanup)) to make it easier to write?

> @@ -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; > + Any reason this loop is a separate pass, rather than... > 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 ...done here?

Also, you have a reference leaks: if the python code passes a non-Callable, you fail to DECREF it (made trickier when the Closure has two callbacks, and only the second callback is not Callable).

We still need to add coverage of h.aio_pread_structured_callback, to prove that this actually did what we wanted when two callbacks are present (the test will fail without this patch).