Libguestfs - Aug 2019 - Re: [PATCH libnbd 5/6] generator: Implement OClosure.

On 8/13/19 5:06 AM, Richard W.M. Jones wrote:
> An optional Closure parameter, but otherwise works the same way as
> Closure.
> @@ -3778,6 +3777,7 @@ let generate_lib_api_c () >      ) args;
>      List.iter (
>        function
> +      | OClosure { cbname } -> pr ", %s_callback ?
\"<fun>\" : \"NULL\"" cbname
Well, it also permits a NULL fn pointer.
> @@ -4383,6 +4387,16 @@ let print_python_binding name { args; optargs; ret;
may_set_error } >    ) args;
>    List.iter (
>      function
> +    | OClosure { cbname } ->
> +       pr "  if (%s_user_data) {\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;
I don't think PyNone is callable; this probably needs to gain a special
case for when the user omitted the optional argument and we thus...
> +       pr "      PyErr_SetString (PyExc_TypeError,\n";
> +       pr "                       \"callback parameter %s is not
callable\");\n" cbname;
> +       pr "      return NULL;\n";
> +       pr "    }\n";
> +       pr "  }\n"
>      | OFlags (n, _) -> pr "  %s_u32 = %s;\n" n n
>    ) optargs;
>  
> @@ -4412,6 +4426,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;
> +       pr ", %s_user_data" cbname
>      | OFlags (n, _) -> pr ", %s_u32" n
>    ) optargs;
>    pr ");\n";
> @@ -4668,6 +4685,7 @@ class NBD (object):
>        let optargs >          List.map (
>            function
> +          | OClosure { cbname } -> cbname, Some "None", None
...passed in the default Python 'None' in its place.

> @@ -5202,6 +5223,19 @@ let print_ocaml_binding (name, { args; optargs; ret
}) >
>    List.iter (
>      function
> +    | OClosure { cbname } ->
> +       pr "  const void *%s_callback = NULL;\n" cbname;
> +       pr "  value *%s_user_data = NULL;\n" cbname;
> +       pr "  if (%sv != Val_int (0)) { /* Some closure */\n"
cbname;
But for OCaml you got it right - you are handling 'None' vs. 'Some
closure'.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

On Tue, Aug 13, 2019 at 06:34:11AM -0500, Eric Blake
wrote:
> On 8/13/19 5:06 AM, Richard W.M. Jones wrote:
> > An optional Closure parameter, but otherwise works the same way as
> > Closure.
> 
> > @@ -3778,6 +3777,7 @@ let generate_lib_api_c () > >      ) args;
> >      List.iter (
> >        function
> > +      | OClosure { cbname } -> pr ", %s_callback ?
\"<fun>\" : \"NULL\"" cbname
> 
> Well, it also permits a NULL fn pointer.
This is right isn't it?
> > @@ -4383,6 +4387,16 @@ let print_python_binding name { args; optargs;
ret; may_set_error } > >    ) args;
> >    List.iter (
> >      function
> > +    | OClosure { cbname } ->
> > +       pr "  if (%s_user_data) {\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;
> 
> I don't think PyNone is callable; this probably needs to gain a special
> case for when the user omitted the optional argument and we thus...
Yeah I'm not sure about this, plus we don't have any test coverage of
it.  But it doesn't work ...

$ nbdkit null --run './run nbdsh --connect nbd://localhost -c "buf =
nbd.Buffer(512)" -c "h.aio_pread_callback (buf, 0)"'
Traceback (most recent call last):
  File "/usr/lib64/python3.7/runpy.py", line 193, in
_run_module_as_main
    "__main__", mod_spec)
  File "/usr/lib64/python3.7/runpy.py", line 85, in _run_code
    exec(code, run_globals)
  File "/home/rjones/d/libnbd/python/nbd.py", line 1417, in
<module>
    nbdsh.shell()
  File "/home/rjones/d/libnbd/python/nbdsh.py", line 62, in shell
    exec (c)
  File "<string>", line 1, in <module>
  File "/home/rjones/d/libnbd/python/nbd.py", line 923, in
aio_pread_callback
    return libnbdmod.aio_pread_callback (self._o, buf._o, offset, completion,
flags)
TypeError: callback parameter completion is not callable

It seems like the "if (%_user_data) {" test at the top is not
sufficient to tell me if the value is None and I've got to do
something else instead.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-p2v converts physical machines to virtual machines.  Boot with a
live CD or over the network (PXE) and turn machines into KVM guests.
http://libguestfs.org/virt-v2v

On 8/13/19 8:32 AM, Richard W.M. Jones wrote:
> On Tue, Aug 13, 2019 at 06:34:11AM -0500, Eric Blake wrote:
>> On 8/13/19 5:06 AM, Richard W.M. Jones wrote:
>>> An optional Closure parameter, but otherwise works the same way as
>>> Closure.
>>
>>> @@ -3778,6 +3777,7 @@ let generate_lib_api_c () >>>      )
args;
>>>      List.iter (
>>>        function
>>> +      | OClosure { cbname } -> pr ", %s_callback ?
\"<fun>\" : \"NULL\"" cbname
>>
>> Well, it also permits a NULL fn pointer.
> 
> This is right isn't it?
Yes. The commit message says it works the same way, but the fact that we
now explicitly permit NULL _is_ an intended side effect (so at most it's
worth a tweak to the commit message).
> 
>>> @@ -4383,6 +4387,16 @@ let print_python_binding name { args;
optargs; ret; may_set_error } >>>    ) args;
>>>    List.iter (
>>>      function
>>> +    | OClosure { cbname } ->
>>> +       pr "  if (%s_user_data) {\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;
>>
>> I don't think PyNone is callable; this probably needs to gain a
special
>> case for when the user omitted the optional argument and we thus...
> 
> Yeah I'm not sure about this, plus we don't have any test coverage
of
> it.  But it doesn't work ...
> 
> $ nbdkit null --run './run nbdsh --connect nbd://localhost -c "buf
= nbd.Buffer(512)" -c "h.aio_pread_callback (buf, 0)"'
> Traceback (most recent call last):
>   File "/usr/lib64/python3.7/runpy.py", line 193, in
_run_module_as_main
>     "__main__", mod_spec)
>   File "/usr/lib64/python3.7/runpy.py", line 85, in _run_code
>     exec(code, run_globals)
>   File "/home/rjones/d/libnbd/python/nbd.py", line 1417, in
<module>
>     nbdsh.shell()
>   File "/home/rjones/d/libnbd/python/nbdsh.py", line 62, in shell
>     exec (c)
>   File "<string>", line 1, in <module>
>   File "/home/rjones/d/libnbd/python/nbd.py", line 923, in
aio_pread_callback
>     return libnbdmod.aio_pread_callback (self._o, buf._o, offset,
completion, flags)
> TypeError: callback parameter completion is not callable
> 
> It seems like the "if (%_user_data) {" test at the top is not
> sufficient to tell me if the value is None and I've got to do
> something else instead.
> 
Correct - as I see it, %s_user_data will ALWAYS be non-null (because
otherwise the PyArg_ParseTupe() would have failed).  I think we either
just check for PyCallable_Check and silently ignore all other arguments
whatever their type (whether the default PyNone or otherwise), or we
explicitly check if %s_user_data is PyNone or satisfies PyCallable_Check
and throw an exception if not.  The latter is probably nicer (I'm not
sure off-hand how to exactly check for PyNone, but it should be easy
enough to research), but the former is less code.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org