Libguestfs - Aug 2019 - Re: [PATCH libnbd 2/5] python: Allow Python callbacks to auto-retire by returning an integer.

This doesn't yet work.  However it does make me more convinced than
ever that we really need to sort out persistent buffer lifetimes in
the library (similar to what we did for closures).

Rich.

Create a class for AIO buffers.  This is a mostly neutral code
refactoring, but we add a convenient function for getting the size of
the buffer (same as previous commit for OCaml).
---
 generator/generator                | 111 +++++++++++++++++------------
 python/handle.c                    |  26 +++++--
 python/t/500-aio-pread.py          |   4 +-
 python/t/505-aio-pread-callback.py |  10 +--
 python/t/510-aio-pwrite.py         |   6 +-
 5 files changed, 96 insertions(+), 61 deletions(-)

diff --git a/generator/generator b/generator/generator
index 26ab365..0107724 100755
--- a/generator/generator
+++ b/generator/generator
@@ -3981,8 +3981,10 @@ raise_exception ()
       pr "extern PyObject *nbd_internal_py_%s (PyObject *self, PyObject
*args);\n"
          name;
   ) ([ "create"; "close";
-       "alloc_aio_buffer"; "aio_buffer_from_bytearray";
-       "aio_buffer_to_bytearray" ] @ List.map fst handle_calls);
+       "alloc_aio_buffer";
+       "aio_buffer_from_bytearray";
+       "aio_buffer_to_bytearray";
+       "aio_buffer_size" ] @ List.map fst handle_calls);
 
   pr "\n";
   pr "#endif /* LIBNBD_METHODS_H */\n"
@@ -4009,8 +4011,10 @@ let generate_python_libnbdmod_c ()        pr "  {
(char *) \"%s\", nbd_internal_py_%s, METH_VARARGS, NULL },\n"
          name name;
   ) ([ "create"; "close";
-       "alloc_aio_buffer"; "aio_buffer_from_bytearray";
-       "aio_buffer_to_bytearray" ] @ List.map fst handle_calls);
+       "alloc_aio_buffer";
+       "aio_buffer_from_bytearray";
+       "aio_buffer_to_bytearray";
+       "aio_buffer_size" ] @ List.map fst handle_calls);
   pr "  { NULL, NULL, 0, NULL }\n";
   pr "};\n";
   pr "\n";
@@ -4502,17 +4506,28 @@ Error.__str__ = _str
 
   pr "\
 
-# AIO buffer functions.
-def aio_buffer (len):
-    '''allocate an AIO buffer used for nbd.aio_pread and
nbd.aio_pwrite'''
-    return libnbdmod.alloc_aio_buffer (len)
-
-def aio_buffer_from_bytearray (ba):
-    '''create an AIO buffer from a bytearray'''
-    return libnbdmod.aio_buffer_from_bytearray (ba)
-def aio_buffer_to_bytearray (buf):
-    '''copy an AIO buffer into a bytearray'''
-    return libnbdmod.aio_buffer_to_bytearray (buf)
+class Buffer (object):
+    '''Asynchronous I/O persistent buffer'''
+
+    def __init__ (self, len):
+        '''allocate an uninitialized AIO buffer used for
nbd.aio_pread'''
+        self._o = libnbdmod.alloc_aio_buffer (len)
+
+    @classmethod
+    def from_bytearray (cls, ba):
+        '''create an AIO buffer from a bytearray'''
+        o = libnbdmod.aio_buffer_from_bytearray (ba)
+        self = cls (0)
+        self._o = o
+        return self
+
+    def to_bytearray (self):
+        '''copy an AIO buffer into a bytearray'''
+        return libnbdmod.aio_buffer_to_bytearray (self._o)
+
+    def size (self):
+        '''return the size of an AIO buffer'''
+        return libnbdmod.aio_buffer_size (self._o)
 
 class NBD (object):
     '''NBD handle'''
@@ -4532,42 +4547,46 @@ class NBD (object):
       let args          List.map (
           function
-          | Bool n -> n
-          | BytesIn (n, _) | BytesPersistIn (n, _) -> n
-          | BytesPersistOut (n, _) -> n
-          | BytesOut (_, count) -> count
-          | Closure { cbname } -> cbname
-          | Int n -> n
-          | Int64 n -> n
-          | Path n -> n
-          | SockAddrAndLen (n, _) -> n
-          | String n -> n
-          | StringList n -> n
-          | UInt n -> n
-          | UInt32 n -> n
-          | UInt64 n -> n
+          | Bool n -> n, None, None
+          | BytesIn (n, _) -> n, None, None
+          | BytesOut (_, count) -> count, None, None
+          | BytesPersistIn (n, _) -> n, None, Some (sprintf
"%s._o" n)
+          | BytesPersistOut (n, _) -> n, None, Some (sprintf
"%s._o" n)
+          | Closure { cbname } -> cbname, None, None
+          | Int n -> n, None, None
+          | Int64 n -> n, None, None
+          | Path n -> n, None, None
+          | SockAddrAndLen (n, _) -> n, None, None
+          | String n -> n, None, None
+          | StringList n -> n, None, None
+          | UInt n -> n, None, None
+          | UInt32 n -> n, None, None
+          | UInt64 n -> n, None, None
         ) args in
       let optargs          List.map (
           function
-          | OFlags n -> n, "0"
+          | OFlags n -> n, Some "0", None
         ) optargs in
-      let () -        let params = args @ List.map (fun (n, def) -> n ^
"=" ^ def) optargs in
-        let params = List.map ((^) ", ") params in
-        let params = String.concat "" params in
-        pr "    def %s (self%s):\n" name params in
-      let () -        let longdesc = Str.global_replace py_fn_rex
"C<nbd." longdesc in
-        let longdesc = Str.global_replace py_const_rex "C<"
longdesc in
-        let longdesc = pod2text longdesc in
-        pr "        '''▶ %s\n\n%s'''\n"
-           shortdesc (String.concat "\n" longdesc) in
-      let () -        let vars = args @ List.map fst optargs in
-        let vars = List.map ((^) ", ") vars in
-        let vars = String.concat "" vars in
-        pr "        return libnbdmod.%s (self._o%s)\n" name vars in
+      let args = args @ optargs in
+      pr "    def %s (self" name;
+      List.iter (
+        function
+        | n, None, _ -> pr ", %s" n
+        | n, Some default, _ -> pr ", %s=%s" n default
+      ) args;
+      pr "):\n";
+      let longdesc = Str.global_replace py_fn_rex "C<nbd."
longdesc in
+      let longdesc = Str.global_replace py_const_rex "C<" longdesc
in
+      let longdesc = pod2text longdesc in
+      pr "        '''▶ %s\n\n%s'''\n"
shortdesc (String.concat "\n" longdesc);
+      pr "        return libnbdmod.%s (self._o" name;
+      List.iter (
+        function
+        | _, _, Some getter -> pr ", %s" getter
+        | n, _, None -> pr ", %s" n
+      ) args;
+      pr ")\n";
       pr "\n"
   ) handle_calls;
 
diff --git a/python/handle.c b/python/handle.c
index 7cff41a..4b510ad 100644
--- a/python/handle.c
+++ b/python/handle.c
@@ -72,7 +72,7 @@ nbd_internal_py_close (PyObject *self, PyObject *args)
   return Py_None;
 }
 
-static char aio_buffer_name[] = "struct py_aio_buffer";
+static const char aio_buffer_name[] = "nbd.Buffer";
 
 struct py_aio_buffer *
 nbd_internal_py_get_aio_buffer (PyObject *capsule)
@@ -89,9 +89,7 @@ free_aio_buffer (PyObject *capsule)
   free (buf);
 }
 
-/* Allocate a persistent buffer used for nbd_aio_pread and
- * nbd_aio_pwrite.
- */
+/* Allocate a persistent buffer used for nbd_aio_pread. */
 PyObject *
 nbd_internal_py_alloc_aio_buffer (PyObject *self, PyObject *args)
 {
@@ -115,7 +113,7 @@ nbd_internal_py_alloc_aio_buffer (PyObject *self, PyObject
*args)
     free (buf);
     return NULL;
   }
-  buf->data = calloc (1, buf->len);
+  buf->data = malloc (buf->len);
   if (buf->data == NULL) {
     PyErr_NoMemory ();
     free (buf);
@@ -193,3 +191,21 @@ nbd_internal_py_aio_buffer_to_bytearray (PyObject *self,
PyObject *args)
 
   return PyByteArray_FromStringAndSize (buf->data, buf->len);
 }
+
+PyObject *
+nbd_internal_py_aio_buffer_size (PyObject *self, PyObject *args)
+{
+  PyObject *obj;
+  struct py_aio_buffer *buf;
+
+  if (!PyArg_ParseTuple (args,
+                         (char *)
"O:nbd_internal_py_aio_buffer_size",
+                         &obj))
+    return NULL;
+
+  buf = nbd_internal_py_get_aio_buffer (obj);
+  if (buf == NULL)
+    return NULL;
+
+  return PyLong_FromSsize_t (buf->len);
+}
diff --git a/python/t/500-aio-pread.py b/python/t/500-aio-pread.py
index 6ff06fd..0c5a07a 100644
--- a/python/t/500-aio-pread.py
+++ b/python/t/500-aio-pread.py
@@ -20,12 +20,12 @@ import nbd
 h = nbd.NBD ()
 h.connect_command (["nbdkit", "-s",
"--exit-with-parent", "-v",
                     "pattern", "size=512"])
-buf = nbd.aio_buffer (512)
+buf = nbd.Buffer (512)
 cookie = h.aio_pread (buf, 0)
 while not (h.aio_command_completed (cookie)):
     h.poll (-1)
 
-buf = nbd.aio_buffer_to_bytearray (buf)
+buf = buf.to_bytearray ()
 
 print ("%r" % buf)
 
diff --git a/python/t/505-aio-pread-callback.py
b/python/t/505-aio-pread-callback.py
index 9246616..e552db8 100644
--- a/python/t/505-aio-pread-callback.py
+++ b/python/t/505-aio-pread-callback.py
@@ -43,21 +43,21 @@ def callback (user_data, err):
     assert user_data == 42
 
 # First try: succeed in both callbacks
-buf = nbd.aio_buffer (512)
+buf = nbd.Buffer (512)
 cookie = h.aio_pread_structured_callback (buf, 0,
                                           lambda *args: chunk (42, *args),
                                           lambda *args: callback (42, *args))
 while not (h.aio_command_completed (cookie)):
     h.poll (-1)
 
-buf = nbd.aio_buffer_to_bytearray (buf)
+buf = buf.to_bytearray ()
 
 print ("%r" % buf)
 
 assert buf == expected
 
 # Second try: fail only during callback
-buf = nbd.aio_buffer (512)
+buf = nbd.Buffer (512)
 cookie = h.aio_pread_structured_callback (buf, 0,
                                           lambda *args: chunk (42, *args),
                                           lambda *args: callback (43, *args))
@@ -69,7 +69,7 @@ except nbd.Error as ex:
     assert ex.errnum == errno.ENOMEM
 
 # Third try: fail during both
-buf = nbd.aio_buffer (512)
+buf = nbd.Buffer (512)
 cookie = h.aio_pread_structured_callback (buf, 0,
                                           lambda *args: chunk (43, *args),
                                           lambda *args: callback (44, *args))
@@ -81,7 +81,7 @@ except nbd.Error as ex:
     assert ex.errnum == errno.ENOMEM
 
 # Fourth try: fail only during chunk
-buf = nbd.aio_buffer (512)
+buf = nbd.Buffer (512)
 cookie = h.aio_pread_structured_callback (buf, 0,
                                           lambda *args: chunk (43, *args),
                                           lambda *args: callback (42, *args))
diff --git a/python/t/510-aio-pwrite.py b/python/t/510-aio-pwrite.py
index 4bbc494..71aa9ba 100644
--- a/python/t/510-aio-pwrite.py
+++ b/python/t/510-aio-pwrite.py
@@ -33,17 +33,17 @@ h = nbd.NBD ()
 h.connect_command (["nbdkit", "-s",
"--exit-with-parent", "-v",
                     "file", datafile])
 
-buf1 = nbd.aio_buffer_from_bytearray (buf)
+buf1 = nbd.Buffer.from_bytearray (buf)
 cookie = h.aio_pwrite (buf1, 0, nbd.CMD_FLAG_FUA)
 while not (h.aio_command_completed (cookie)):
     h.poll (-1)
 
-buf2 = nbd.aio_buffer (512)
+buf2 = nbd.Buffer (512)
 cookie = h.aio_pread (buf2, 0)
 while not (h.aio_command_completed (cookie)):
     h.poll (-1)
 
-assert buf == nbd.aio_buffer_to_bytearray (buf2)
+assert buf == buf2.to_bytearray ()
 
 with open (datafile, "rb") as f:
     content = f.read ()
-- 
2.22.0

See equivalent change for OCaml in
commit d881d160e1cd9c9964782300a7652ffb4e506c27.
---
 generator/generator | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/generator/generator b/generator/generator
index 0107724..0523f0a 100755
--- a/generator/generator
+++ b/generator/generator
@@ -4135,7 +4135,8 @@ let print_python_binding name { args; optargs; ret;
may_set_error }         pr "    Py_DECREF (py_args);\n";
        pr "\n";
        pr "    if (py_ret != NULL) {\n";
-       pr "      Py_DECREF (py_ret); /* return value is discarded
*/\n";
+       pr "      ret = PyLong_AsLong (py_ret);\n";
+       pr "      Py_DECREF (py_ret);\n";
        pr "    }\n";
        pr "    else {\n";
        pr "      ret = -1;\n";
-- 
2.22.0

When functions are called which take a persistent buffer as a
parameter, we must increment the reference count to avoid a segfault.
Unfortunately (and unavoidably right now) this leaks the buffer.

To fix this we will need to really implement buffer lifetimes,
controlled by libnbd.  (That would also fix the equivalent problem in
the OCaml bindings.)

Until that is done, leaking memory is slightly better than
segfaulting.
---
 generator/generator | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/generator/generator b/generator/generator
index 0523f0a..02afb3a 100755
--- a/generator/generator
+++ b/generator/generator
@@ -4283,6 +4283,11 @@ let print_python_binding name { args; optargs; ret;
may_set_error }      | BytesOut (n, count) ->
        pr "  %s = malloc (%s);\n" n count
     | BytesPersistIn (n, _) | BytesPersistOut (n, _) ->
+       pr "  /* Increment refcount since buf will be saved by
libnbd.\n";
+       pr "   * XXX This memory is leaked since we don't have a way
to\n";
+       pr "   * know the lifetime of this buffer.\n";
+       pr "   */\n";
+       pr "  Py_INCREF (%s);\n" n;
        pr "  %s_buf = nbd_internal_py_get_aio_buffer (%s);\n" n n
     | Closure { cbname } ->
        pr "  /* Increment refcount since pointer may be saved by libnbd.
*/\n";
-- 
2.22.0

Although it's documented as returning a new reference, decrementing
the refcount causes a segfault.  Unknown why.
---
 generator/generator | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/generator/generator b/generator/generator
index 02afb3a..a9f97d7 100755
--- a/generator/generator
+++ b/generator/generator
@@ -4150,7 +4150,7 @@ let print_python_binding name { args; optargs; ret;
may_set_error }           | CBMutable (Int n) ->
             pr "    PyObject *py_%s_ret = PyObject_GetAttrString (py_%s,
\"value\");\n" n n;
             pr "    *%s = PyLong_AsLong (py_%s_ret);\n" n n;
-            pr "    Py_DECREF (py_%s_ret);\n" n;
+            pr "    // Py_DECREF (py_%s_ret); - segfaults, why?\n" n;
             pr "    Py_DECREF (py_%s);\n" n
          | CBBytesIn _
          | CBInt _ | CBInt64 _
-- 
2.22.0

---
 python/t/590-aio-copy.py | 117 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 117 insertions(+)

diff --git a/python/t/590-aio-copy.py b/python/t/590-aio-copy.py
new file mode 100644
index 0000000..4e814ee
--- /dev/null
+++ b/python/t/590-aio-copy.py
@@ -0,0 +1,117 @@
+# libnbd Python bindings
+# Copyright (C) 2010-2019 Red Hat Inc.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program; if not, write to the Free Software
+# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
+
+import select
+import nbd
+
+disk_size = 512 * 1024 * 1024
+bs = 65536
+max_reads_in_flight = 16
+bytes_read = 0
+bytes_written = 0
+
+def asynch_copy (src, dst):
+    size = src.get_size ()
+
+    # This is our reading position in the source.
+    soff = 0
+
+    # This callback is called when any pread from the source
+    # has completed.
+    writes = []
+    def read_completed (buf, offset, error):
+        global bytes_read
+        bytes_read += buf.size ()
+        writes.append ((buf, offset))
+        # By returning 1 here we auto-retire the pread command.
+        return 1
+
+    # This callback is called when any pwrite to the destination
+    # has completed.
+    def write_completed (buf, error):
+        global bytes_written
+        bytes_written += buf.size ()
+        # By returning 1 here we auto-retire the pwrite command.
+        return 1
+
+    # The main loop which runs until we have finished reading and
+    # there are no more commands in flight.
+    while soff < size or dst.aio_in_flight () > 0:
+        # If we're able to submit more reads from the source
+        # then do so now.
+        if soff < size and src.aio_in_flight () < max_reads_in_flight:
+            bufsize = min (bs, size - soff)
+            buf = nbd.Buffer (bufsize)
+            src.aio_pread_callback (buf, soff,
+                                    lambda *args: read_completed (buf, soff,
+                                                                  *args))
+            soff += bufsize
+
+        # If there are any write commands waiting to be issued
+        # to the destination, send them now.
+        for (buf, offset) in writes:
+            dst.aio_pwrite_callback (buf, offset,
+                                     lambda *args: write_completed (buf,
*args))
+        writes = []
+
+        poll = select.poll ()
+
+        sfd = src.aio_get_fd ()
+        dfd = dst.aio_get_fd ()
+
+        sevents = 0
+        devents = 0
+        if src.aio_get_direction () & nbd.AIO_DIRECTION_READ:
+            sevents += select.POLLIN
+        if src.aio_get_direction () & nbd.AIO_DIRECTION_WRITE:
+            sevents += select.POLLOUT
+        if dst.aio_get_direction () & nbd.AIO_DIRECTION_READ:
+            devents += select.POLLIN
+        if dst.aio_get_direction () & nbd.AIO_DIRECTION_WRITE:
+            devents += select.POLLOUT
+        poll.register (sfd, sevents)
+        poll.register (dfd, devents)
+        for (fd, revents) in poll.poll ():
+            # The direction of each handle can change since we
+            # slept in the select.
+            if fd == sfd and revents & select.POLLIN and \
+               src.aio_get_direction () & nbd.AIO_DIRECTION_READ:
+                src.aio_notify_read ()
+            elif fd == sfd and revents & select.POLLOUT and \
+                 src.aio_get_direction () & nbd.AIO_DIRECTION_WRITE:
+                src.aio_notify_write ()
+            elif fd == dfd and revents & select.POLLIN and \
+                 dst.aio_get_direction () & nbd.AIO_DIRECTION_READ:
+                dst.aio_notify_read ()
+            elif fd == dfd and revents & select.POLLOUT and \
+                 dst.aio_get_direction () & nbd.AIO_DIRECTION_WRITE:
+                dst.aio_notify_write ()
+
+src = nbd.NBD ()
+src.set_handle_name ("src")
+dst = nbd.NBD ()
+dst.set_handle_name ("dst")
+src.connect_command (["nbdkit", "-s",
"--exit-with-parent", "-r",
+                      "pattern", "size=%d" % disk_size])
+dst.connect_command (["nbdkit", "-s",
"--exit-with-parent",
+                      "memory", "size=%d" % disk_size])
+asynch_copy (src, dst)
+
+print ("bytes read: %d written: %d size: %d" %
+       (bytes_read, bytes_written, disk_size))
+assert bytes_read == disk_size
+assert bytes_written == disk_size
-- 
2.22.0

On reflection I have pushed this patch because it's an improvement to
the current Python bindings and pretty much a change we would always
want to make no matter how we deal with the buffer lifetimes issue.

The rest of the series however, same as I wrote in the cover letter.

On Sat, Aug 10, 2019 at 06:02:56PM +0100, Richard W.M. Jones
wrote:
> Create a class for AIO buffers.  This is a mostly neutral code
> refactoring, but we add a convenient function for getting the size of
> the buffer (same as previous commit for OCaml).
> ---
>  generator/generator                | 111 +++++++++++++++++------------
>  python/handle.c                    |  26 +++++--
>  python/t/500-aio-pread.py          |   4 +-
>  python/t/505-aio-pread-callback.py |  10 +--
>  python/t/510-aio-pwrite.py         |   6 +-
>  5 files changed, 96 insertions(+), 61 deletions(-)
> 
> diff --git a/generator/generator b/generator/generator
> index 26ab365..0107724 100755
> --- a/generator/generator
> +++ b/generator/generator
> @@ -3981,8 +3981,10 @@ raise_exception ()
>        pr "extern PyObject *nbd_internal_py_%s (PyObject *self,
PyObject *args);\n"
>           name;
>    ) ([ "create"; "close";
> -       "alloc_aio_buffer";
"aio_buffer_from_bytearray";
> -       "aio_buffer_to_bytearray" ] @ List.map fst handle_calls);
> +       "alloc_aio_buffer";
> +       "aio_buffer_from_bytearray";
> +       "aio_buffer_to_bytearray";
> +       "aio_buffer_size" ] @ List.map fst handle_calls);
>  
>    pr "\n";
>    pr "#endif /* LIBNBD_METHODS_H */\n"
> @@ -4009,8 +4011,10 @@ let generate_python_libnbdmod_c () >        pr
"  { (char *) \"%s\", nbd_internal_py_%s, METH_VARARGS, NULL
},\n"
>           name name;
>    ) ([ "create"; "close";
> -       "alloc_aio_buffer";
"aio_buffer_from_bytearray";
> -       "aio_buffer_to_bytearray" ] @ List.map fst handle_calls);
> +       "alloc_aio_buffer";
> +       "aio_buffer_from_bytearray";
> +       "aio_buffer_to_bytearray";
> +       "aio_buffer_size" ] @ List.map fst handle_calls);
>    pr "  { NULL, NULL, 0, NULL }\n";
>    pr "};\n";
>    pr "\n";
> @@ -4502,17 +4506,28 @@ Error.__str__ = _str
>  
>    pr "\
>  
> -# AIO buffer functions.
> -def aio_buffer (len):
> -    '''allocate an AIO buffer used for nbd.aio_pread and
nbd.aio_pwrite'''
> -    return libnbdmod.alloc_aio_buffer (len)
> -
> -def aio_buffer_from_bytearray (ba):
> -    '''create an AIO buffer from a bytearray'''
> -    return libnbdmod.aio_buffer_from_bytearray (ba)
> -def aio_buffer_to_bytearray (buf):
> -    '''copy an AIO buffer into a bytearray'''
> -    return libnbdmod.aio_buffer_to_bytearray (buf)
> +class Buffer (object):
> +    '''Asynchronous I/O persistent buffer'''
> +
> +    def __init__ (self, len):
> +        '''allocate an uninitialized AIO buffer used for
nbd.aio_pread'''
> +        self._o = libnbdmod.alloc_aio_buffer (len)
> +
> +    @classmethod
> +    def from_bytearray (cls, ba):
> +        '''create an AIO buffer from a
bytearray'''
> +        o = libnbdmod.aio_buffer_from_bytearray (ba)
> +        self = cls (0)
> +        self._o = o
> +        return self
> +
> +    def to_bytearray (self):
> +        '''copy an AIO buffer into a bytearray'''
> +        return libnbdmod.aio_buffer_to_bytearray (self._o)
> +
> +    def size (self):
> +        '''return the size of an AIO buffer'''
> +        return libnbdmod.aio_buffer_size (self._o)
>  
>  class NBD (object):
>      '''NBD handle'''
> @@ -4532,42 +4547,46 @@ class NBD (object):
>        let args >          List.map (
>            function
> -          | Bool n -> n
> -          | BytesIn (n, _) | BytesPersistIn (n, _) -> n
> -          | BytesPersistOut (n, _) -> n
> -          | BytesOut (_, count) -> count
> -          | Closure { cbname } -> cbname
> -          | Int n -> n
> -          | Int64 n -> n
> -          | Path n -> n
> -          | SockAddrAndLen (n, _) -> n
> -          | String n -> n
> -          | StringList n -> n
> -          | UInt n -> n
> -          | UInt32 n -> n
> -          | UInt64 n -> n
> +          | Bool n -> n, None, None
> +          | BytesIn (n, _) -> n, None, None
> +          | BytesOut (_, count) -> count, None, None
> +          | BytesPersistIn (n, _) -> n, None, Some (sprintf
"%s._o" n)
> +          | BytesPersistOut (n, _) -> n, None, Some (sprintf
"%s._o" n)
> +          | Closure { cbname } -> cbname, None, None
> +          | Int n -> n, None, None
> +          | Int64 n -> n, None, None
> +          | Path n -> n, None, None
> +          | SockAddrAndLen (n, _) -> n, None, None
> +          | String n -> n, None, None
> +          | StringList n -> n, None, None
> +          | UInt n -> n, None, None
> +          | UInt32 n -> n, None, None
> +          | UInt64 n -> n, None, None
>          ) args in
>        let optargs >          List.map (
>            function
> -          | OFlags n -> n, "0"
> +          | OFlags n -> n, Some "0", None
>          ) optargs in
> -      let () > -        let params = args @ List.map (fun (n, def)
-> n ^ "=" ^ def) optargs in
> -        let params = List.map ((^) ", ") params in
> -        let params = String.concat "" params in
> -        pr "    def %s (self%s):\n" name params in
> -      let () > -        let longdesc = Str.global_replace py_fn_rex
"C<nbd." longdesc in
> -        let longdesc = Str.global_replace py_const_rex "C<"
longdesc in
> -        let longdesc = pod2text longdesc in
> -        pr "        '''▶ %s\n\n%s'''\n"
> -           shortdesc (String.concat "\n" longdesc) in
> -      let () > -        let vars = args @ List.map fst optargs in
> -        let vars = List.map ((^) ", ") vars in
> -        let vars = String.concat "" vars in
> -        pr "        return libnbdmod.%s (self._o%s)\n" name vars
in
> +      let args = args @ optargs in
> +      pr "    def %s (self" name;
> +      List.iter (
> +        function
> +        | n, None, _ -> pr ", %s" n
> +        | n, Some default, _ -> pr ", %s=%s" n default
> +      ) args;
> +      pr "):\n";
> +      let longdesc = Str.global_replace py_fn_rex "C<nbd."
longdesc in
> +      let longdesc = Str.global_replace py_const_rex "C<"
longdesc in
> +      let longdesc = pod2text longdesc in
> +      pr "        '''▶ %s\n\n%s'''\n"
shortdesc (String.concat "\n" longdesc);
> +      pr "        return libnbdmod.%s (self._o" name;
> +      List.iter (
> +        function
> +        | _, _, Some getter -> pr ", %s" getter
> +        | n, _, None -> pr ", %s" n
> +      ) args;
> +      pr ")\n";
>        pr "\n"
>    ) handle_calls;
>  
> diff --git a/python/handle.c b/python/handle.c
> index 7cff41a..4b510ad 100644
> --- a/python/handle.c
> +++ b/python/handle.c
> @@ -72,7 +72,7 @@ nbd_internal_py_close (PyObject *self, PyObject *args)
>    return Py_None;
>  }
>  
> -static char aio_buffer_name[] = "struct py_aio_buffer";
> +static const char aio_buffer_name[] = "nbd.Buffer";
>  
>  struct py_aio_buffer *
>  nbd_internal_py_get_aio_buffer (PyObject *capsule)
> @@ -89,9 +89,7 @@ free_aio_buffer (PyObject *capsule)
>    free (buf);
>  }
>  
> -/* Allocate a persistent buffer used for nbd_aio_pread and
> - * nbd_aio_pwrite.
> - */
> +/* Allocate a persistent buffer used for nbd_aio_pread. */
>  PyObject *
>  nbd_internal_py_alloc_aio_buffer (PyObject *self, PyObject *args)
>  {
> @@ -115,7 +113,7 @@ nbd_internal_py_alloc_aio_buffer (PyObject *self,
PyObject *args)
>      free (buf);
>      return NULL;
>    }
> -  buf->data = calloc (1, buf->len);
> +  buf->data = malloc (buf->len);
>    if (buf->data == NULL) {
>      PyErr_NoMemory ();
>      free (buf);
> @@ -193,3 +191,21 @@ nbd_internal_py_aio_buffer_to_bytearray (PyObject
*self, PyObject *args)
>  
>    return PyByteArray_FromStringAndSize (buf->data, buf->len);
>  }
> +
> +PyObject *
> +nbd_internal_py_aio_buffer_size (PyObject *self, PyObject *args)
> +{
> +  PyObject *obj;
> +  struct py_aio_buffer *buf;
> +
> +  if (!PyArg_ParseTuple (args,
> +                         (char *)
"O:nbd_internal_py_aio_buffer_size",
> +                         &obj))
> +    return NULL;
> +
> +  buf = nbd_internal_py_get_aio_buffer (obj);
> +  if (buf == NULL)
> +    return NULL;
> +
> +  return PyLong_FromSsize_t (buf->len);
> +}
> diff --git a/python/t/500-aio-pread.py b/python/t/500-aio-pread.py
> index 6ff06fd..0c5a07a 100644
> --- a/python/t/500-aio-pread.py
> +++ b/python/t/500-aio-pread.py
> @@ -20,12 +20,12 @@ import nbd
>  h = nbd.NBD ()
>  h.connect_command (["nbdkit", "-s",
"--exit-with-parent", "-v",
>                      "pattern", "size=512"])
> -buf = nbd.aio_buffer (512)
> +buf = nbd.Buffer (512)
>  cookie = h.aio_pread (buf, 0)
>  while not (h.aio_command_completed (cookie)):
>      h.poll (-1)
>  
> -buf = nbd.aio_buffer_to_bytearray (buf)
> +buf = buf.to_bytearray ()
>  
>  print ("%r" % buf)
>  
> diff --git a/python/t/505-aio-pread-callback.py
b/python/t/505-aio-pread-callback.py
> index 9246616..e552db8 100644
> --- a/python/t/505-aio-pread-callback.py
> +++ b/python/t/505-aio-pread-callback.py
> @@ -43,21 +43,21 @@ def callback (user_data, err):
>      assert user_data == 42
>  
>  # First try: succeed in both callbacks
> -buf = nbd.aio_buffer (512)
> +buf = nbd.Buffer (512)
>  cookie = h.aio_pread_structured_callback (buf, 0,
>                                            lambda *args: chunk (42, *args),
>                                            lambda *args: callback (42,
*args))
>  while not (h.aio_command_completed (cookie)):
>      h.poll (-1)
>  
> -buf = nbd.aio_buffer_to_bytearray (buf)
> +buf = buf.to_bytearray ()
>  
>  print ("%r" % buf)
>  
>  assert buf == expected
>  
>  # Second try: fail only during callback
> -buf = nbd.aio_buffer (512)
> +buf = nbd.Buffer (512)
>  cookie = h.aio_pread_structured_callback (buf, 0,
>                                            lambda *args: chunk (42, *args),
>                                            lambda *args: callback (43,
*args))
> @@ -69,7 +69,7 @@ except nbd.Error as ex:
>      assert ex.errnum == errno.ENOMEM
>  
>  # Third try: fail during both
> -buf = nbd.aio_buffer (512)
> +buf = nbd.Buffer (512)
>  cookie = h.aio_pread_structured_callback (buf, 0,
>                                            lambda *args: chunk (43, *args),
>                                            lambda *args: callback (44,
*args))
> @@ -81,7 +81,7 @@ except nbd.Error as ex:
>      assert ex.errnum == errno.ENOMEM
>  
>  # Fourth try: fail only during chunk
> -buf = nbd.aio_buffer (512)
> +buf = nbd.Buffer (512)
>  cookie = h.aio_pread_structured_callback (buf, 0,
>                                            lambda *args: chunk (43, *args),
>                                            lambda *args: callback (42,
*args))
> diff --git a/python/t/510-aio-pwrite.py b/python/t/510-aio-pwrite.py
> index 4bbc494..71aa9ba 100644
> --- a/python/t/510-aio-pwrite.py
> +++ b/python/t/510-aio-pwrite.py
> @@ -33,17 +33,17 @@ h = nbd.NBD ()
>  h.connect_command (["nbdkit", "-s",
"--exit-with-parent", "-v",
>                      "file", datafile])
>  
> -buf1 = nbd.aio_buffer_from_bytearray (buf)
> +buf1 = nbd.Buffer.from_bytearray (buf)
>  cookie = h.aio_pwrite (buf1, 0, nbd.CMD_FLAG_FUA)
>  while not (h.aio_command_completed (cookie)):
>      h.poll (-1)
>  
> -buf2 = nbd.aio_buffer (512)
> +buf2 = nbd.Buffer (512)
>  cookie = h.aio_pread (buf2, 0)
>  while not (h.aio_command_completed (cookie)):
>      h.poll (-1)
>  
> -assert buf == nbd.aio_buffer_to_bytearray (buf2)
> +assert buf == buf2.to_bytearray ()
>  
>  with open (datafile, "rb") as f:
>      content = f.read ()
Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-top is 'top' for virtual machines.  Tiny program with many
powerful monitoring features, net stats, disk stats, logging, etc.
http://people.redhat.com/~rjones/virt-top

On 8/10/19 12:02 PM, Richard W.M. Jones wrote:
> See equivalent change for OCaml in
> commit d881d160e1cd9c9964782300a7652ffb4e506c27.
> ---
>  generator/generator | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
The idea makes sense, but I'm not sure if the code is correct:
> diff --git a/generator/generator b/generator/generator
> index 0107724..0523f0a 100755
> --- a/generator/generator
> +++ b/generator/generator
> @@ -4135,7 +4135,8 @@ let print_python_binding name { args; optargs; ret;
may_set_error } >         pr "    Py_DECREF (py_args);\n";
>         pr "\n";
>         pr "    if (py_ret != NULL) {\n";
> -       pr "      Py_DECREF (py_ret); /* return value is discarded
*/\n";
> +       pr "      ret = PyLong_AsLong (py_ret);\n";
> +       pr "      Py_DECREF (py_ret);\n";
This doesn't detect if the user returned a non-integer type (in which
case ret will be -1) - are we okay blindly returning -1 regardless of
whether the user returned actual -1 vs. if they returned some other
non-integer Python object that has no __int__ conversion?  Or do we need
to use PyErr_Occurred() to distinguish between the two cases?  This is
particularly interesting since we document that the C callback must
return -1 before any update to *err will take effect; do we want Python
to have to return -1 for that effect, or is it okay if python raises an
exception and we safely catch that as the way to translate to a C return
of -1?

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