Re: [Libguestfs] [PATCH libnbd v2 1/3] generator: Implement OClosure.

On 8/13/19 10:12 AM, Richard W.M. Jones wrote: > An optional Closure parameter, but otherwise works the same way as > Closure. > --- > generator/generator | 57 ++++++++++++++++++++++++++++++++++++--------- > 1 file changed, 46 insertions(+), 11 deletions(-) > > @@ -4394,6 +4399,16 @@ let print_python_binding name { args; optargs; ret; may_set_error } = > ) args; > List.iter ( > function > + | OClosure { cbname } -> > + pr " if (%s_user_data != Py_None) {\n" cbname; > + pr " /* Increment refcount since pointer may be saved by libnbd. */\n"; > + pr " Py_INCREF (%s_user_data);\n" 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; > + pr " return NULL;\n"; Leaks %s_user_data, because we fail to do a matching Py_DECREF on failure paths. It would be easier to defer the Py_INCREF to after we verified it is Callable. However, on looking further, I see this is also a pre-existing bug affecting Closure. For that matter, should we be using 'py_ret = NULL; goto out;' similar to StringList, to avoid any other leaks on any other failure paths? (The generated python code still probably has a number of poor handling of failures, which needs a proper audit...)

> + pr " }\n"; > + pr " }\n" > | OFlags (n, _) -> pr " %s_u32 = %s;\n" n n > ) optargs; > > @@ -4423,6 +4438,9 @@ let print_python_binding name { args; optargs; ret; may_set_error } = > ) args; > List.iter ( > function > + | OClosure { cbname } -> > + pr ", %s_user_data ? %s_wrapper : NULL" cbname cbname; Still looks wrong. %s_user_data is non-NULL; the check here should be %s_user_data != Py_None.