[PATCH libnbd 0/5] WIP: python: Add test for doing asynch copy.
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
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
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
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
On Sat, Aug 10, 2019 at 04:57:01PM -0500, Eric Blake wrote:
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?
Ah ha! Yes I did indeed observe this problem but couldn't understand what was happening - this explains it. Thanks, Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-df lists disk usage of guests without needing to install any software inside the virtual machine. Supports Linux and Windows. http://people.redhat.com/~rjones/virt-df/
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
participants (2)
-
Eric Blake -
Richard W.M. Jones