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

[Richard W.M. Jones]

On Tue, Aug 13, 2019 at 10:36:23AM -0500, Eric Blake wrote:
> 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...)

Right the behaviour on failure of the Python bindings is
very difficult.

(It should be OK in OCaml because of the way the stack
unwinding in OCaml exceptions works).

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

Yes this is buggy - fixed in my copy.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-builder quickly builds VMs from scratch
http://libguestfs.org/virt-builder.1.html