[libnbd PATCH v2 0/8] Another attempt at working towards zero-copy python aio_pread
Still more patches to come, but this fixes the heap leak I pointed out this morning, and rebases my previous patches on top of that cleanup. Eric Blake (8): python: Improve doc comments for nbd.py python: Plug uninit leak in nbd.Buffer.to_bytearray python: Enhance tests of nbd.Buffer python: Reformat generated methods.c in a few places python: Make py_aio_buffer a private struct python: Don't unwrap nbd.Buffer in nbd.py python: Simplify python generator python: Make nbd.Buffer lighter-weight generator/Python.ml | 336 +++++++++++++++++------------------- python/handle.c | 214 ++++++----------------- python/t/500-aio-pread.py | 83 ++++++++- python/t/510-aio-pwrite.py | 19 +- python/t/580-aio-is-zero.py | 7 + python/utils.c | 20 ++- 6 files changed, 336 insertions(+), 343 deletions(-) -- 2.36.1
PEP257 recommends that doc comments start with a one line summary and be a complete sentence. Tweak our existing comments to comply. It also recommends the use of """ strings, and for u""" when a string contains Unicode; however, for since it is easier to write ' than " in OCaml pr statements, we stick with ''' and u''' strings rather than follow the PEP completely. Also, enhance the documentation for nbd.Buffer.from_bytearray to match the semantic changes from commit d477f7c7, before we change those semantics yet again in an upcoming patch (copying is inefficient, we are moving towards reusing the underlying buffer directly). --- generator/Python.ml | 23 ++++++++++++++--------- 1 file changed, 14 insertions(+), 9 deletions(-) diff --git a/generator/Python.ml b/generator/Python.ml index 3f672ba..392be87 100644 --- a/generator/Python.ml +++ b/generator/Python.ml @@ -763,28 +763,33 @@ let '''Asynchronous I/O persistent buffer''' def __init__(self, len): - '''allocate an uninitialized AIO buffer used for nbd.aio_pread''' + '''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''' + '''Create an AIO buffer from a bytearray or other buffer-like object. + + If ba is not a buffer, it is tried as the parameter to the + bytearray constructor. Otherwise, ba is copied. Either way, the + resulting AIO buffer is independent from the original. + ''' 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''' + '''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 the size of an AIO buffer.''' return libnbdmod.aio_buffer_size(self._o) def is_zero(self, offset=0, size=-1): - ''' - Returns true if and only if all bytes in the buffer are zeroes. + '''Returns true if and only if all bytes in the buffer are zeroes. + Note that a freshly allocated buffer is uninitialized, not zero. By default this tests the whole buffer, but you can restrict @@ -801,11 +806,11 @@ let '''NBD handle''' def __init__(self): - '''create a new NBD handle''' + '''Create a new NBD handle.''' self._o = libnbdmod.create() def __del__(self): - '''close the NBD handle and underlying connection''' + '''Close the NBD handle and underlying connection.''' if hasattr(self, '_o'): libnbdmod.close(self._o) @@ -855,7 +860,7 @@ let let longdesc = Str.global_replace py_fn_rex "C<nbd.\\1>" 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 " u'''▶ %s\n\n%s'''\n" shortdesc (String.concat "\n" longdesc); pr " return libnbdmod.%s(" name; pr_wrap ',' (fun () -> pr "self._o"; -- 2.36.1
On Mon, Jun 06, 2022 at 09:08:26PM -0500, Eric Blake wrote:
PEP257 recommends that doc comments start with a one line summary and be a complete sentence. Tweak our existing comments to comply. It also recommends the use of """ strings, and for u""" when a string contains Unicode; however, for since it is easier to write ' than " in OCaml pr statements, we stick with ''' and u''' strings rather than follow the PEP completely.
Also, enhance the documentation for nbd.Buffer.from_bytearray to match the semantic changes from commit d477f7c7, before we change those semantics yet again in an upcoming patch (copying is inefficient, we are moving towards reusing the underlying buffer directly). --- generator/Python.ml | 23 ++++++++++++++--------- 1 file changed, 14 insertions(+), 9 deletions(-)
diff --git a/generator/Python.ml b/generator/Python.ml index 3f672ba..392be87 100644 --- a/generator/Python.ml +++ b/generator/Python.ml @@ -763,28 +763,33 @@ let '''Asynchronous I/O persistent buffer'''
def __init__(self, len): - '''allocate an uninitialized AIO buffer used for nbd.aio_pread''' + '''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''' + '''Create an AIO buffer from a bytearray or other buffer-like object. + + If ba is not a buffer, it is tried as the parameter to the + bytearray constructor. Otherwise, ba is copied. Either way, the + resulting AIO buffer is independent from the original. + ''' 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''' + '''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 the size of an AIO buffer.''' return libnbdmod.aio_buffer_size(self._o)
def is_zero(self, offset=0, size=-1): - ''' - Returns true if and only if all bytes in the buffer are zeroes. + '''Returns true if and only if all bytes in the buffer are zeroes. + Note that a freshly allocated buffer is uninitialized, not zero.
By default this tests the whole buffer, but you can restrict @@ -801,11 +806,11 @@ let '''NBD handle'''
def __init__(self): - '''create a new NBD handle''' + '''Create a new NBD handle.''' self._o = libnbdmod.create()
def __del__(self): - '''close the NBD handle and underlying connection''' + '''Close the NBD handle and underlying connection.''' if hasattr(self, '_o'): libnbdmod.close(self._o)
@@ -855,7 +860,7 @@ let let longdesc = Str.global_replace py_fn_rex "C<nbd.\\1>" 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 " u'''▶ %s\n\n%s'''\n" shortdesc (String.concat "\n" longdesc); pr " return libnbdmod.%s(" name; pr_wrap ',' (fun () -> pr "self._o";
Reviewed-by: Richard W.M. Jones <rjones@redhat.com> Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com nbdkit - Flexible, fast NBD server with plugins https://gitlab.com/nbdkit/nbdkit
When we created nbd.Buffer to handle both aio_pread and aio_pwrite, we provided two different approaches to construct an instance. The constructor nbd.Buffer(int) allocates uninitialized data, to be filled in by aio_pread (rather than wasting time pre-zeroing the data just to be changed again - although that speedup is only possible when opting in to h.set_pread_initialized(false)). Meanwhile, the classmethod nbd.Buffer.from_bytearray(...) allocates initialized data, to be consumed by aio_pwrite. However, this scheme makes it trivially easy to use the wrong instance creation approach for a given handle operation, and thereby leak uninitialized heap contents into the destination with something as simple-looking as h.aio_pwrite(nbd.Buffer(512), 0). It doesn't help that while bytearray(10) is all zeroes (a conscientious choice of the Python language), and therefore nbd.Buffer.from_bytearray(10).is_zero() is deterministically True, nbd.Buffer(10).is_zero() is non-deterministic; nor that nbd.Buffer(10).to_bytearray() can allow Python code to play with uninitialized data. It is always a dangerous practice to leak heap contents; however, as this leak is triggered only by poor client Python code, and not under control of anything the server sends, we don't think it rises to the level of the heap leak that was assigned CVE-2022-0485 as plugged back in commit 8d444b41. The solution employed here is to mark when a buffer has been initialized, in nbd.Buffer.from_bytearray() and h.aio_pread[_structured], as well as force-initialize an uninitialized buffer before b.to_bytearray() or h.aio_pwrite. Furthermore, we can make b.is_zero() pretend an uninitialized buffer is all zeroes (since the user can no longer easily get at any other contents). The approach is not bulletproof - if h.aio_pread triggers a subsequent read error in the C code (such as client-side bounds check or server error response) AND the user has disabled h.set_pread_initialized(), we've already marked the buffer initialized at that point even though the buffer may not be fully written. But that's okay - we've already documented that a user opting in to that setting is responsible for safe buffer practices, and should not dereference a read buffer when an error is reported while retiring the command. Generated code changes as follows: | --- python/methods.h.bak 2022-06-06 07:43:33.167181594 -0500 | +++ python/methods.h 2022-06-06 07:43:40.494190038 -0500 | @@ -31,6 +31,7 @@ | struct py_aio_buffer { | Py_ssize_t len; | void *data; | + bool initialized; | }; | | extern char **nbd_internal_py_get_string_list (PyObject *); | --- python/methods.c.bak 2022-06-06 07:43:33.165181592 -0500 | +++ python/methods.c 2022-06-06 07:56:25.109094074 -0500 | @@ -3201,6 +3201,7 @@ nbd_internal_py_aio_pread (PyObject *sel | completion_user_data->buf = buf; | offset_u64 = offset; | | + buf_buf->initialized = true; | ret = nbd_aio_pread (h, buf_buf->data, buf_buf->len, offset_u64, completion, flags_u32); | completion_user_data = NULL; | if (ret == -1) { | @@ -3274,6 +3275,7 @@ nbd_internal_py_aio_pread_structured (Py | Py_INCREF (py_chunk_fn); | chunk_user_data->fn = py_chunk_fn; | | + buf_buf->initialized = true; | ret = nbd_aio_pread_structured (h, buf_buf->data, buf_buf->len, offset_u64, chunk, completion, flags_u32); | chunk_user_data = NULL; | completion_user_data = NULL; | @@ -3335,6 +3337,10 @@ nbd_internal_py_aio_pwrite (PyObject *se | completion_user_data->buf = buf; | offset_u64 = offset; | | + if (!buf_buf->initialized) { | + memset (buf_buf->data, 0, buf_buf->len); | + buf_buf->initialized = true; | + } | ret = nbd_aio_pwrite (h, buf_buf->data, buf_buf->len, offset_u64, completion, flags_u32); | completion_user_data = NULL; | if (ret == -1) { | --- python/nbd.py.bak 2022-06-06 07:43:33.169181596 -0500 | +++ python/nbd.py 2022-06-06 07:43:40.516190063 -0500 | @@ -150,7 +150,9 @@ class Buffer(object): | def is_zero(self, offset=0, size=-1): | '''Returns true if and only if all bytes in the buffer are zeroes. | | - Note that a freshly allocated buffer is uninitialized, not zero. | + Note that although a freshly allocated buffer is uninitialized, | + this will report it as all zeroes, as it will be force-initialized | + to zero before any code that can access the buffer's contents. | | By default this tests the whole buffer, but you can restrict | the test to a sub-range of the buffer using the optional Fixes: f05b8ec5 ("python: Change aio_buffer into nbd.Buffer class.", v0.9.8) --- generator/Python.ml | 23 +++++++++++++++++++---- python/handle.c | 9 ++++++++- python/t/510-aio-pwrite.py | 13 ++++++++++++- python/t/580-aio-is-zero.py | 7 +++++++ 4 files changed, 46 insertions(+), 6 deletions(-) diff --git a/generator/Python.ml b/generator/Python.ml index 392be87..8239803 100644 --- a/generator/Python.ml +++ b/generator/Python.ml @@ -37,6 +37,7 @@ let struct py_aio_buffer { Py_ssize_t len; void *data; + bool initialized; }; extern char **nbd_internal_py_get_string_list (PyObject *); @@ -79,7 +80,7 @@ let "aio_buffer_from_bytearray"; "aio_buffer_to_bytearray"; "aio_buffer_size"; - "aio_buffer_is_zero" ] @ List.map fst handle_calls); + "aio_buffer_is_zero"] @ List.map fst handle_calls); pr "\n"; pr "#endif /* LIBNBD_METHODS_H */\n" @@ -111,7 +112,7 @@ let "aio_buffer_from_bytearray"; "aio_buffer_to_bytearray"; "aio_buffer_size"; - "aio_buffer_is_zero" ] @ List.map fst handle_calls); + "aio_buffer_is_zero"] @ List.map fst handle_calls); pr " { NULL, NULL, 0, NULL }\n"; pr "};\n"; pr "\n"; @@ -408,6 +409,7 @@ let pr "))\n"; pr " goto out;\n"; + (* Two passes over parameters. Any 'goto err' must be in first pass. *) pr " h = get_handle (py_h);\n"; pr " if (!h) goto out;\n"; List.iter ( @@ -475,7 +477,18 @@ let ) args; pr "\n"; - (* Call the underlying C function. *) + (* Second pass, and call the underlying C function. *) + List.iter ( + function + | BytesPersistIn (n, _) -> + pr " if (!%s_buf->initialized) {\n" n; + pr " memset (%s_buf->data, 0, %s_buf->len);\n" n n; + pr " %s_buf->initialized = true;\n" n; + pr " }\n" + | BytesPersistOut (n, _) -> + pr " %s_buf->initialized = true;\n" n + | _ -> () + ) args; pr " ret = nbd_%s (h" name; List.iter ( function @@ -790,7 +803,9 @@ let def is_zero(self, offset=0, size=-1): '''Returns true if and only if all bytes in the buffer are zeroes. - Note that a freshly allocated buffer is uninitialized, not zero. + Note that although a freshly allocated buffer is uninitialized, + this will report it as all zeroes, as it will be force-initialized + to zero before any code that can access the buffer's contents. By default this tests the whole buffer, but you can restrict the test to a sub-range of the buffer using the optional diff --git a/python/handle.c b/python/handle.c index ed04b76..d42a563 100644 --- a/python/handle.c +++ b/python/handle.c @@ -127,6 +127,7 @@ nbd_internal_py_alloc_aio_buffer (PyObject *self, PyObject *args) return NULL; } + buf->initialized = false; if (!PyArg_ParseTuple (args, "n:nbd_internal_py_alloc_aio_buffer", &buf->len)) { free (buf); @@ -202,6 +203,7 @@ nbd_internal_py_aio_buffer_from_bytearray (PyObject *self, PyObject *args) } memcpy (buf->data, data, len); Py_XDECREF (arr); + buf->initialized = true; ret = PyCapsule_New (buf, aio_buffer_name, free_aio_buffer); if (ret == NULL) { @@ -228,6 +230,11 @@ nbd_internal_py_aio_buffer_to_bytearray (PyObject *self, PyObject *args) if (buf == NULL) return NULL; + if (!buf->initialized) { + memset (buf->data, 0, buf->len); + buf->initialized = true; + } + return PyByteArray_FromStringAndSize (buf->data, buf->len); } @@ -288,7 +295,7 @@ nbd_internal_py_aio_buffer_is_zero (PyObject *self, PyObject *args) return NULL; } - if (is_zero (buf->data + offset, size)) + if (!buf->initialized || is_zero (buf->data + offset, size)) Py_RETURN_TRUE; else Py_RETURN_FALSE; diff --git a/python/t/510-aio-pwrite.py b/python/t/510-aio-pwrite.py index 438cc54..89599fc 100644 --- a/python/t/510-aio-pwrite.py +++ b/python/t/510-aio-pwrite.py @@ -1,5 +1,5 @@ # libnbd Python bindings -# Copyright (C) 2010-2019 Red Hat Inc. +# Copyright (C) 2010-2022 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 @@ -50,4 +50,15 @@ with open(datafile, "rb") as f: assert buf == content +# Also check that an uninitialized buffer doesn't leak heap contents +buf3 = nbd.Buffer(512) +cookie = h.aio_pwrite(buf3, 0, flags=nbd.CMD_FLAG_FUA) +while not h.aio_command_completed(cookie): + h.poll(-1) + +with open(datafile, "rb") as f: + content = f.read() + +assert nbd.Buffer.from_bytearray(content).is_zero() + os.unlink(datafile) diff --git a/python/t/580-aio-is-zero.py b/python/t/580-aio-is-zero.py index 49dea5e..1a9a7d3 100644 --- a/python/t/580-aio-is-zero.py +++ b/python/t/580-aio-is-zero.py @@ -69,3 +69,10 @@ assert not buf.is_zero(2**20-1, 1) assert buf.is_zero(2**20, 1) assert not buf.is_zero(0, 1) assert buf.is_zero(2**21-1, 1) + +# A buffer created with only a size is generally uninitialized until +# used with aio_pread; but it will be zeroed if accessed prematurely +buf = nbd.Buffer(1024) +assert buf.size() == 1024 +assert buf.is_zero() +assert nbd.Buffer.from_bytearray(buf.to_bytearray()).is_zero() -- 2.36.1
On Mon, Jun 06, 2022 at 09:08:27PM -0500, Eric Blake wrote:
@@ -79,7 +80,7 @@ let "aio_buffer_from_bytearray"; "aio_buffer_to_bytearray"; "aio_buffer_size"; - "aio_buffer_is_zero" ] @ List.map fst handle_calls); + "aio_buffer_is_zero"] @ List.map fst handle_calls);
pr "\n"; pr "#endif /* LIBNBD_METHODS_H */\n" @@ -111,7 +112,7 @@ let "aio_buffer_from_bytearray"; "aio_buffer_to_bytearray"; "aio_buffer_size"; - "aio_buffer_is_zero" ] @ List.map fst handle_calls); + "aio_buffer_is_zero"] @ List.map fst handle_calls); pr " { NULL, NULL, 0, NULL }\n"; pr "};\n"; pr "\n";
Unintended whitespace changes? Without those two hunks: Reviewed-by: Richard W.M. Jones <rjones@redhat.com> Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com libguestfs lets you edit virtual machines. Supports shell scripting, bindings from many languages. http://libguestfs.org
On Tue, Jun 07, 2022 at 02:13:23PM +0100, Richard W.M. Jones wrote:
On Mon, Jun 06, 2022 at 09:08:27PM -0500, Eric Blake wrote:
@@ -79,7 +80,7 @@ let "aio_buffer_from_bytearray"; "aio_buffer_to_bytearray"; "aio_buffer_size"; - "aio_buffer_is_zero" ] @ List.map fst handle_calls); + "aio_buffer_is_zero"] @ List.map fst handle_calls);
pr "\n"; pr "#endif /* LIBNBD_METHODS_H */\n" @@ -111,7 +112,7 @@ let "aio_buffer_from_bytearray"; "aio_buffer_to_bytearray"; "aio_buffer_size"; - "aio_buffer_is_zero" ] @ List.map fst handle_calls); + "aio_buffer_is_zero"] @ List.map fst handle_calls); pr " { NULL, NULL, 0, NULL }\n"; pr "};\n"; pr "\n";
Unintended whitespace changes? Without those two hunks:
Yeah, I was playing with adding aio_buffer_is_init to let the user probe the status of an nbd.Buffer, then changed my mind, and it shows. Will fix. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
On Mon, Jun 06, 2022 at 09:08:27PM -0500, Eric Blake wrote:
The solution employed here is to mark when a buffer has been initialized, in nbd.Buffer.from_bytearray() and h.aio_pread[_structured], as well as force-initialize an uninitialized buffer before b.to_bytearray() or h.aio_pwrite. Furthermore, we can make b.is_zero() pretend an uninitialized buffer is all zeroes (since the user can no longer easily get at any other contents).
@@ -288,7 +295,7 @@ nbd_internal_py_aio_buffer_is_zero (PyObject *self, PyObject *args) return NULL; }
- if (is_zero (buf->data + offset, size)) + if (!buf->initialized || is_zero (buf->data + offset, size)) Py_RETURN_TRUE; else Py_RETURN_FALSE;
I just noticed this function has an odd order of execution - if size is 0, it returns true even if offset is out-of-bounds. It would also be more Pythonic if we changed offset/size to instead be more like Python range start/stop parameters (particularly since future patches can then compute memoryview(buffer)[start:end] as the bytes to check for being zero), but that's an API change. I'll have to think about that one more... But that doesn't change the content of this patch. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
Add some more coverage of existing behavior, so we can better track that we are not introducing unintended changes in upcoming patches. --- python/t/500-aio-pread.py | 83 +++++++++++++++++++++++++++++++++++--- python/t/510-aio-pwrite.py | 6 +++ 2 files changed, 84 insertions(+), 5 deletions(-) diff --git a/python/t/500-aio-pread.py b/python/t/500-aio-pread.py index fc1abad..c236163 100644 --- a/python/t/500-aio-pread.py +++ b/python/t/500-aio-pread.py @@ -1,5 +1,5 @@ # libnbd Python bindings -# Copyright (C) 2010-2019 Red Hat Inc. +# Copyright (C) 2010-2022 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 @@ -19,17 +19,25 @@ import nbd h = nbd.NBD() h.connect_command(["nbdkit", "-s", "--exit-with-parent", "-v", - "pattern", "size=512"]) + "pattern", "size=1024"]) buf = nbd.Buffer(512) cookie = h.aio_pread(buf, 0) while not h.aio_command_completed(cookie): h.poll(-1) -buf = buf.to_bytearray() +buf1 = buf.to_bytearray() -print("%r" % buf) +# Prove that .to_bytearray() defaults to copying, even if buf is reused -assert buf == (b'\x00\x00\x00\x00\x00\x00\x00\x00' +cookie = h.aio_pread(buf, 512) +while not h.aio_command_completed(cookie): + h.poll(-1) + +buf2 = buf.to_bytearray() + +print("%r" % buf1) + +assert buf1 == (b'\x00\x00\x00\x00\x00\x00\x00\x00' + b'\x00\x00\x00\x00\x00\x00\x00\x08' + b'\x00\x00\x00\x00\x00\x00\x00\x10' + b'\x00\x00\x00\x00\x00\x00\x00\x18' @@ -93,3 +101,68 @@ assert buf == (b'\x00\x00\x00\x00\x00\x00\x00\x00' + b'\x00\x00\x00\x00\x00\x00\x01\xe8' + b'\x00\x00\x00\x00\x00\x00\x01\xf0' + b'\x00\x00\x00\x00\x00\x00\x01\xf8') + +assert buf2 == (b'\x00\x00\x00\x00\x00\x00\x02\x00' + + b'\x00\x00\x00\x00\x00\x00\x02\x08' + + b'\x00\x00\x00\x00\x00\x00\x02\x10' + + b'\x00\x00\x00\x00\x00\x00\x02\x18' + + b'\x00\x00\x00\x00\x00\x00\x02 ' + + b'\x00\x00\x00\x00\x00\x00\x02(' + + b'\x00\x00\x00\x00\x00\x00\x020' + + b'\x00\x00\x00\x00\x00\x00\x028' + + b'\x00\x00\x00\x00\x00\x00\x02@' + + b'\x00\x00\x00\x00\x00\x00\x02H' + + b'\x00\x00\x00\x00\x00\x00\x02P' + + b'\x00\x00\x00\x00\x00\x00\x02X' + + b'\x00\x00\x00\x00\x00\x00\x02`' + + b'\x00\x00\x00\x00\x00\x00\x02h' + + b'\x00\x00\x00\x00\x00\x00\x02p' + + b'\x00\x00\x00\x00\x00\x00\x02x' + + b'\x00\x00\x00\x00\x00\x00\x02\x80' + + b'\x00\x00\x00\x00\x00\x00\x02\x88' + + b'\x00\x00\x00\x00\x00\x00\x02\x90' + + b'\x00\x00\x00\x00\x00\x00\x02\x98' + + b'\x00\x00\x00\x00\x00\x00\x02\xa0' + + b'\x00\x00\x00\x00\x00\x00\x02\xa8' + + b'\x00\x00\x00\x00\x00\x00\x02\xb0' + + b'\x00\x00\x00\x00\x00\x00\x02\xb8' + + b'\x00\x00\x00\x00\x00\x00\x02\xc0' + + b'\x00\x00\x00\x00\x00\x00\x02\xc8' + + b'\x00\x00\x00\x00\x00\x00\x02\xd0' + + b'\x00\x00\x00\x00\x00\x00\x02\xd8' + + b'\x00\x00\x00\x00\x00\x00\x02\xe0' + + b'\x00\x00\x00\x00\x00\x00\x02\xe8' + + b'\x00\x00\x00\x00\x00\x00\x02\xf0' + + b'\x00\x00\x00\x00\x00\x00\x02\xf8' + + b'\x00\x00\x00\x00\x00\x00\x03\x00' + + b'\x00\x00\x00\x00\x00\x00\x03\x08' + + b'\x00\x00\x00\x00\x00\x00\x03\x10' + + b'\x00\x00\x00\x00\x00\x00\x03\x18' + + b'\x00\x00\x00\x00\x00\x00\x03 ' + + b'\x00\x00\x00\x00\x00\x00\x03(' + + b'\x00\x00\x00\x00\x00\x00\x030' + + b'\x00\x00\x00\x00\x00\x00\x038' + + b'\x00\x00\x00\x00\x00\x00\x03@' + + b'\x00\x00\x00\x00\x00\x00\x03H' + + b'\x00\x00\x00\x00\x00\x00\x03P' + + b'\x00\x00\x00\x00\x00\x00\x03X' + + b'\x00\x00\x00\x00\x00\x00\x03`' + + b'\x00\x00\x00\x00\x00\x00\x03h' + + b'\x00\x00\x00\x00\x00\x00\x03p' + + b'\x00\x00\x00\x00\x00\x00\x03x' + + b'\x00\x00\x00\x00\x00\x00\x03\x80' + + b'\x00\x00\x00\x00\x00\x00\x03\x88' + + b'\x00\x00\x00\x00\x00\x00\x03\x90' + + b'\x00\x00\x00\x00\x00\x00\x03\x98' + + b'\x00\x00\x00\x00\x00\x00\x03\xa0' + + b'\x00\x00\x00\x00\x00\x00\x03\xa8' + + b'\x00\x00\x00\x00\x00\x00\x03\xb0' + + b'\x00\x00\x00\x00\x00\x00\x03\xb8' + + b'\x00\x00\x00\x00\x00\x00\x03\xc0' + + b'\x00\x00\x00\x00\x00\x00\x03\xc8' + + b'\x00\x00\x00\x00\x00\x00\x03\xd0' + + b'\x00\x00\x00\x00\x00\x00\x03\xd8' + + b'\x00\x00\x00\x00\x00\x00\x03\xe0' + + b'\x00\x00\x00\x00\x00\x00\x03\xe8' + + b'\x00\x00\x00\x00\x00\x00\x03\xf0' + + b'\x00\x00\x00\x00\x00\x00\x03\xf8') diff --git a/python/t/510-aio-pwrite.py b/python/t/510-aio-pwrite.py index 89599fc..d09e249 100644 --- a/python/t/510-aio-pwrite.py +++ b/python/t/510-aio-pwrite.py @@ -45,6 +45,12 @@ while not h.aio_command_completed(cookie): assert buf == buf2.to_bytearray() +# Check that .from_bytearray() defaults to copying +buf[511] = 0x55 +assert buf != buf1.to_bytearray() +buf[511] = 0xAA +assert buf == buf1.to_bytearray() + with open(datafile, "rb") as f: content = f.read() -- 2.36.1
On Mon, Jun 06, 2022 at 09:08:28PM -0500, Eric Blake wrote:
Add some more coverage of existing behavior, so we can better track that we are not introducing unintended changes in upcoming patches. --- python/t/500-aio-pread.py | 83 +++++++++++++++++++++++++++++++++++--- python/t/510-aio-pwrite.py | 6 +++ 2 files changed, 84 insertions(+), 5 deletions(-)
diff --git a/python/t/500-aio-pread.py b/python/t/500-aio-pread.py index fc1abad..c236163 100644 --- a/python/t/500-aio-pread.py +++ b/python/t/500-aio-pread.py @@ -1,5 +1,5 @@ # libnbd Python bindings -# Copyright (C) 2010-2019 Red Hat Inc. +# Copyright (C) 2010-2022 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 @@ -19,17 +19,25 @@ import nbd
h = nbd.NBD() h.connect_command(["nbdkit", "-s", "--exit-with-parent", "-v", - "pattern", "size=512"]) + "pattern", "size=1024"]) buf = nbd.Buffer(512) cookie = h.aio_pread(buf, 0) while not h.aio_command_completed(cookie): h.poll(-1)
-buf = buf.to_bytearray() +buf1 = buf.to_bytearray()
-print("%r" % buf) +# Prove that .to_bytearray() defaults to copying, even if buf is reused
-assert buf == (b'\x00\x00\x00\x00\x00\x00\x00\x00' +cookie = h.aio_pread(buf, 512) +while not h.aio_command_completed(cookie): + h.poll(-1) + +buf2 = buf.to_bytearray() + +print("%r" % buf1) + +assert buf1 == (b'\x00\x00\x00\x00\x00\x00\x00\x00' + b'\x00\x00\x00\x00\x00\x00\x00\x08' + b'\x00\x00\x00\x00\x00\x00\x00\x10' + b'\x00\x00\x00\x00\x00\x00\x00\x18' @@ -93,3 +101,68 @@ assert buf == (b'\x00\x00\x00\x00\x00\x00\x00\x00' + b'\x00\x00\x00\x00\x00\x00\x01\xe8' + b'\x00\x00\x00\x00\x00\x00\x01\xf0' + b'\x00\x00\x00\x00\x00\x00\x01\xf8') + +assert buf2 == (b'\x00\x00\x00\x00\x00\x00\x02\x00' + + b'\x00\x00\x00\x00\x00\x00\x02\x08' + + b'\x00\x00\x00\x00\x00\x00\x02\x10' + + b'\x00\x00\x00\x00\x00\x00\x02\x18' + + b'\x00\x00\x00\x00\x00\x00\x02 ' + + b'\x00\x00\x00\x00\x00\x00\x02(' + + b'\x00\x00\x00\x00\x00\x00\x020' + + b'\x00\x00\x00\x00\x00\x00\x028' + + b'\x00\x00\x00\x00\x00\x00\x02@' + + b'\x00\x00\x00\x00\x00\x00\x02H' + + b'\x00\x00\x00\x00\x00\x00\x02P' + + b'\x00\x00\x00\x00\x00\x00\x02X' + + b'\x00\x00\x00\x00\x00\x00\x02`' + + b'\x00\x00\x00\x00\x00\x00\x02h' + + b'\x00\x00\x00\x00\x00\x00\x02p' + + b'\x00\x00\x00\x00\x00\x00\x02x' + + b'\x00\x00\x00\x00\x00\x00\x02\x80' + + b'\x00\x00\x00\x00\x00\x00\x02\x88' + + b'\x00\x00\x00\x00\x00\x00\x02\x90' + + b'\x00\x00\x00\x00\x00\x00\x02\x98' + + b'\x00\x00\x00\x00\x00\x00\x02\xa0' + + b'\x00\x00\x00\x00\x00\x00\x02\xa8' + + b'\x00\x00\x00\x00\x00\x00\x02\xb0' + + b'\x00\x00\x00\x00\x00\x00\x02\xb8' + + b'\x00\x00\x00\x00\x00\x00\x02\xc0' + + b'\x00\x00\x00\x00\x00\x00\x02\xc8' + + b'\x00\x00\x00\x00\x00\x00\x02\xd0' + + b'\x00\x00\x00\x00\x00\x00\x02\xd8' + + b'\x00\x00\x00\x00\x00\x00\x02\xe0' + + b'\x00\x00\x00\x00\x00\x00\x02\xe8' + + b'\x00\x00\x00\x00\x00\x00\x02\xf0' + + b'\x00\x00\x00\x00\x00\x00\x02\xf8' + + b'\x00\x00\x00\x00\x00\x00\x03\x00' + + b'\x00\x00\x00\x00\x00\x00\x03\x08' + + b'\x00\x00\x00\x00\x00\x00\x03\x10' + + b'\x00\x00\x00\x00\x00\x00\x03\x18' + + b'\x00\x00\x00\x00\x00\x00\x03 ' + + b'\x00\x00\x00\x00\x00\x00\x03(' + + b'\x00\x00\x00\x00\x00\x00\x030' + + b'\x00\x00\x00\x00\x00\x00\x038' + + b'\x00\x00\x00\x00\x00\x00\x03@' + + b'\x00\x00\x00\x00\x00\x00\x03H' + + b'\x00\x00\x00\x00\x00\x00\x03P' + + b'\x00\x00\x00\x00\x00\x00\x03X' + + b'\x00\x00\x00\x00\x00\x00\x03`' + + b'\x00\x00\x00\x00\x00\x00\x03h' + + b'\x00\x00\x00\x00\x00\x00\x03p' + + b'\x00\x00\x00\x00\x00\x00\x03x' + + b'\x00\x00\x00\x00\x00\x00\x03\x80' + + b'\x00\x00\x00\x00\x00\x00\x03\x88' + + b'\x00\x00\x00\x00\x00\x00\x03\x90' + + b'\x00\x00\x00\x00\x00\x00\x03\x98' + + b'\x00\x00\x00\x00\x00\x00\x03\xa0' + + b'\x00\x00\x00\x00\x00\x00\x03\xa8' + + b'\x00\x00\x00\x00\x00\x00\x03\xb0' + + b'\x00\x00\x00\x00\x00\x00\x03\xb8' + + b'\x00\x00\x00\x00\x00\x00\x03\xc0' + + b'\x00\x00\x00\x00\x00\x00\x03\xc8' + + b'\x00\x00\x00\x00\x00\x00\x03\xd0' + + b'\x00\x00\x00\x00\x00\x00\x03\xd8' + + b'\x00\x00\x00\x00\x00\x00\x03\xe0' + + b'\x00\x00\x00\x00\x00\x00\x03\xe8' + + b'\x00\x00\x00\x00\x00\x00\x03\xf0' + + b'\x00\x00\x00\x00\x00\x00\x03\xf8') diff --git a/python/t/510-aio-pwrite.py b/python/t/510-aio-pwrite.py index 89599fc..d09e249 100644 --- a/python/t/510-aio-pwrite.py +++ b/python/t/510-aio-pwrite.py @@ -45,6 +45,12 @@ while not h.aio_command_completed(cookie):
assert buf == buf2.to_bytearray()
+# Check that .from_bytearray() defaults to copying +buf[511] = 0x55 +assert buf != buf1.to_bytearray() +buf[511] = 0xAA +assert buf == buf1.to_bytearray() + with open(datafile, "rb") as f: content = f.read()
Reviewed-by: Richard W.M. Jones <rjones@redhat.com> Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com Fedora Windows cross-compiler. Compile Windows programs, test, and build Windows installers. Over 100 libraries supported. http://fedoraproject.org/wiki/MinGW
On Tue, Jun 07, 2022 at 02:14:36PM +0100, Richard W.M. Jones wrote:
On Mon, Jun 06, 2022 at 09:08:28PM -0500, Eric Blake wrote:
Add some more coverage of existing behavior, so we can better track that we are not introducing unintended changes in upcoming patches. --- python/t/500-aio-pread.py | 83 +++++++++++++++++++++++++++++++++++--- python/t/510-aio-pwrite.py | 6 +++ 2 files changed, 84 insertions(+), 5 deletions(-)
diff --git a/python/t/500-aio-pread.py b/python/t/500-aio-pread.py index fc1abad..c236163 100644 --- a/python/t/500-aio-pread.py
+print("%r" % buf1) + +assert buf1 == (b'\x00\x00\x00\x00\x00\x00\x00\x00' + b'\x00\x00\x00\x00\x00\x00\x00\x08' + b'\x00\x00\x00\x00\x00\x00\x00\x10' + b'\x00\x00\x00\x00\x00\x00\x00\x18' @@ -93,3 +101,68 @@ assert buf == (b'\x00\x00\x00\x00\x00\x00\x00\x00' + b'\x00\x00\x00\x00\x00\x00\x01\xe8' + b'\x00\x00\x00\x00\x00\x00\x01\xf0' + b'\x00\x00\x00\x00\x00\x00\x01\xf8')
For this one, I may try to shorten the test by using: import array from array a = array('q', range(0, 512, 8)) if sys.byteorder == 'little': a.byteswap() assert buf1 == memoryview(a).cast('B') -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
To make argument parsing/building easier to follow, and make it so that future type changes touch fewer lines of code, consolidate several passes of List.iter to instead produce a list of tuples in one pass. Now the python conversion character(s) are next to the code that character matches. Compress the strings passed to PyArg_ParseTuple and Py_BuildValue. Utilize py_wrap to avoid generated lines that exceed 80 columns. The change is solely in the formatting; no behavioral changes are intended. The generated diff includes chunks such as: | @@ -84,7 +84,8 @@ chunk_wrapper (void *user_data, const vo | Py_DECREF (py_error_mod); | if (!py_error) { PyErr_PrintEx (0); goto out; } | | - py_args = Py_BuildValue ("(" "y#" "K" "I" "O" ")", subbuf, (int) count, offset, status, py_error); | + py_args = Py_BuildValue ("(y#KIO)", subbuf, (int) count, offset, status, | + py_error); | if (!py_args) { PyErr_PrintEx (0); goto out; } | | py_save = PyGILState_Ensure (); ... | @@ -1054,7 +1031,8 @@ nbd_internal_py_set_tls_psk_file (PyObje | } | | PyObject * | -nbd_internal_py_set_request_structured_replies (PyObject *self, PyObject *args) | +nbd_internal_py_set_request_structured_replies (PyObject *self, | + PyObject *args) | { | PyObject *py_h; | struct nbd_handle *h; ... | @@ -2344,8 +2281,7 @@ nbd_internal_py_pread_structured (PyObje | uint32_t flags_u32; | unsigned int flags; /* really uint32_t */ | | - if (!PyArg_ParseTuple (args, "O" "n" "K" "O" "I" | - ":nbd_pread_structured", | + if (!PyArg_ParseTuple (args, "OnKOI:nbd_pread_structured", | &py_h, &count, &offset, &py_chunk_fn, &flags)) | goto out; | h = get_handle (py_h); | @@ -2365,7 +2301,8 @@ nbd_internal_py_pread_structured (PyObje | Py_INCREF (py_chunk_fn); | chunk_user_data->fn = py_chunk_fn; | | - ret = nbd_pread_structured (h, PyByteArray_AS_STRING (buf), count, offset_u64, chunk, flags_u32); | + ret = nbd_pread_structured (h, PyByteArray_AS_STRING (buf), count, | + offset_u64, chunk, flags_u32); | chunk_user_data = NULL; | if (ret == -1) { | raise_exception (); Suggested-by: Richard W.M. Jones <rjones@redhat.com> --- generator/Python.ml | 201 ++++++++++++++++++-------------------------- 1 file changed, 81 insertions(+), 120 deletions(-) diff --git a/generator/Python.ml b/generator/Python.ml index 8239803..6244c8e 100644 --- a/generator/Python.ml +++ b/generator/Python.ml @@ -209,30 +209,31 @@ let ) cbargs; pr "\n"; - pr " py_args = Py_BuildValue (\"(\""; - List.iter ( - function - | CBArrayAndLen (UInt32 n, len) -> pr " \"O\"" - | CBBytesIn (n, len) -> pr " \"y#\"" - | CBInt n -> pr " \"i\"" - | CBInt64 n -> pr " \"L\"" - | CBMutable (Int n) -> pr " \"O\"" - | CBString n -> pr " \"s\"" - | CBUInt n -> pr " \"I\"" - | CBUInt64 n -> pr " \"K\"" - | CBArrayAndLen _ | CBMutable _ -> assert false - ) cbargs; - pr " \")\""; - List.iter ( - function - | CBArrayAndLen (UInt32 n, _) -> pr ", py_%s" n - | CBBytesIn (n, len) -> pr ", %s, (int) %s" n len - | CBMutable (Int n) -> pr ", py_%s" n - | CBInt n | CBInt64 n - | CBString n - | CBUInt n | CBUInt64 n -> pr ", %s" n - | CBArrayAndLen _ | CBMutable _ -> assert false - ) cbargs; + let params = + List.map ( + function + | CBArrayAndLen (UInt32 n, _) -> "O", sprintf "py_%s" n + | CBBytesIn (n, len) -> "y#", sprintf "%s, (int) %s" n len + | CBInt n -> "i", n + | CBInt64 n -> "L", n + | CBMutable (Int n) -> "O", sprintf "py_%s" n + | CBString n -> "s", n + | CBUInt n -> "I", n + | CBUInt64 n -> "K", n + | CBArrayAndLen _ | CBMutable _ -> assert false + ) cbargs in + pr " py_args = Py_BuildValue ("; + pr_wrap ',' (fun () -> + pr "\"("; + List.iter ( + function + | n, _ -> pr "%s" n + ) params; + pr ")\""; + List.iter ( + function + | _, n -> pr ", %s" n + ) params); pr ");\n"; pr " if (!py_args) { PyErr_PrintEx (0); goto out; }\n"; pr "\n"; @@ -284,7 +285,10 @@ let (* Generate the Python binding. *) let print_python_binding name { args; optargs; ret; may_set_error } = pr "PyObject *\n"; - pr "nbd_internal_py_%s (PyObject *self, PyObject *args)\n" name; + pr "nbd_internal_py_%s (" name; + pr_wrap ',' (fun () -> + pr "PyObject *self, PyObject *args"); + pr ")\n"; pr "{\n"; pr " PyObject *py_h;\n"; pr " struct nbd_handle *h;\n"; @@ -353,59 +357,52 @@ let pr "\n"; (* Parse the Python parameters. *) - pr " if (!PyArg_ParseTuple (args, \"O\""; + let params = + List.map ( + function + | Bool n -> "p", sprintf "&%s" n, n + | BytesIn (n, _) -> "y*", sprintf "&%s" n, sprintf "%s.buf, %s.len" n n + | BytesOut (n, count) -> + "n", sprintf "&%s" count, + sprintf "PyByteArray_AS_STRING (%s), %s" n count + | BytesPersistIn (n, _) | BytesPersistOut (n, _) -> + "O", sprintf "&%s" n, sprintf "%s_buf->data, %s_buf->len" n n + | Closure { cbname } -> "O", sprintf "&py_%s_fn" cbname, cbname + | Enum (n, _) -> "i", sprintf "&%s" n, n + | Flags (n, _) -> "I", sprintf "&%s" n, sprintf "%s_u32" n + | Fd n | Int n -> "i", sprintf "&%s" n, n + | Int64 n -> "L", sprintf "&%s" n, sprintf "%s_i64" n + | Path n -> "O&", sprintf "PyUnicode_FSConverter, &py_%s" n, n + | SizeT n -> "n", sprintf "&%s" n, sprintf "(size_t)%s" n + | SockAddrAndLen (n, _) -> + "O", sprintf "&%s" n, + sprintf "(struct sockaddr *) &%s_sa, %s_len" n n + | String n -> "s", sprintf "&%s" n, n + | StringList n -> "O", sprintf "&py_%s" n, n + | UInt n | UIntPtr n -> "I", sprintf "&%s" n, n + | UInt32 n -> "I", sprintf "&%s" n, sprintf "%s_u32" n + | UInt64 n -> "K", sprintf "&%s" n, sprintf "%s_u64" n + ) args in + let optparams = + List.map ( + function + | OClosure { cbname } -> "O", sprintf "&py_%s_fn" cbname, cbname + | OFlags (n, _, _) -> "I", sprintf "&%s" n, sprintf "%s_u32" n + ) optargs in + let params = params @ optparams in + pr " if (!PyArg_ParseTuple (args, \"O"; List.iter ( function - | Bool n -> pr " \"p\"" - | BytesIn (n, _) -> pr " \"y*\"" - | BytesPersistIn (n, _) -> pr " \"O\"" - | BytesOut (_, count) -> pr " \"n\"" - | BytesPersistOut (_, count) -> pr " \"O\"" - | Closure _ -> pr " \"O\"" - | Enum _ -> pr " \"i\"" - | Flags _ -> pr " \"I\"" - | Fd n | Int n -> pr " \"i\"" - | Int64 n -> pr " \"L\"" - | Path n -> pr " \"O&\"" - | SizeT n -> pr " \"n\"" - | SockAddrAndLen (n, _) -> pr " \"O\"" - | String n -> pr " \"s\"" - | StringList n -> pr " \"O\"" - | UInt n | UIntPtr n -> pr " \"I\"" - | UInt32 n -> pr " \"I\"" - | UInt64 n -> pr " \"K\"" - ) args; - List.iter ( - function - | OClosure _ -> pr " \"O\"" - | OFlags _ -> pr " \"I\"" - ) optargs; - pr "\n"; - pr " \":nbd_%s\",\n" name; - pr " &py_h"; - List.iter ( - function - | Bool n -> pr ", &%s" n - | BytesIn (n, _) | BytesPersistIn (n, _) - | BytesPersistOut (n, _) -> pr ", &%s" n - | BytesOut (_, count) -> pr ", &%s" count - | Closure { cbname } -> pr ", &py_%s_fn" cbname - | Enum (n, _) -> pr ", &%s" n - | Flags (n, _) -> pr ", &%s" n - | Fd n | Int n | SizeT n | Int64 n -> pr ", &%s" n - | Path n -> pr ", PyUnicode_FSConverter, &py_%s" n - | SockAddrAndLen (n, _) -> pr ", &%s" n - | String n -> pr ", &%s" n - | StringList n -> pr ", &py_%s" n - | UInt n | UIntPtr n -> pr ", &%s" n - | UInt32 n -> pr ", &%s" n - | UInt64 n -> pr ", &%s" n - ) args; - List.iter ( - function - | OClosure { cbname } -> pr ", &py_%s_fn" cbname - | OFlags (n, _, _) -> pr ", &%s" n - ) optargs; + | n, _, _ -> pr "%s" n + ) params; + pr ":nbd_%s\",\n" name; + pr " "; + pr_wrap ',' (fun () -> + pr "&py_h"; + List.iter ( + function + | _, n, _ -> pr ", %s" n + ) params); pr "))\n"; pr " goto out;\n"; @@ -489,33 +486,13 @@ let pr " %s_buf->initialized = true;\n" n | _ -> () ) args; - pr " ret = nbd_%s (h" name; - List.iter ( - function - | Bool n -> pr ", %s" n - | BytesIn (n, _) -> pr ", %s.buf, %s.len" n n - | BytesOut (n, count) -> pr ", PyByteArray_AS_STRING (%s), %s" n count - | BytesPersistIn (n, _) - | BytesPersistOut (n, _) -> pr ", %s_buf->data, %s_buf->len" n n - | Closure { cbname } -> pr ", %s" cbname - | Enum (n, _) -> pr ", %s" n - | Flags (n, _) -> pr ", %s_u32" n - | Fd n | Int n -> pr ", %s" n - | Int64 n -> pr ", %s_i64" n - | Path n -> pr ", %s" n - | SizeT n -> pr ", (size_t)%s" n - | SockAddrAndLen (n, _) -> pr ", (struct sockaddr *) &%s_sa, %s_len" n n - | String n -> pr ", %s" n - | StringList n -> pr ", %s" n - | UInt n | UIntPtr n -> pr ", %s" n - | UInt32 n -> pr ", %s_u32" n - | UInt64 n -> pr ", %s_u64" n - ) args; - List.iter ( - function - | OClosure { cbname } -> pr ", %s" cbname - | OFlags (n, _, _) -> pr ", %s_u32" n - ) optargs; + pr " ret = nbd_%s (" name; + pr_wrap ',' (fun () -> + pr "h"; + List.iter ( + function + | _, _, n -> pr ", %s" n + ) params); pr ");\n"; List.iter ( function @@ -543,23 +520,7 @@ let pr " py_ret = %s;\n" n; pr " %s = NULL;\n" n; use_ret := false - | Bool _ - | BytesIn _ - | BytesPersistIn _ | BytesPersistOut _ - | Closure _ - | Enum _ - | Flags _ - | Fd _ | Int _ - | Int64 _ - | Path _ - | SockAddrAndLen _ - | SizeT _ - | String _ - | StringList _ - | UInt _ - | UIntPtr _ - | UInt32 _ - | UInt64 _ -> () + | _ -> () ) args; if !use_ret then ( match ret with -- 2.36.1
On Mon, Jun 06, 2022 at 09:08:29PM -0500, Eric Blake wrote:
+ pr ":nbd_%s\",\n" name;
You could put this pr (but without the \n) ...
+ pr " "; + pr_wrap ',' (fun () ->
... inside pr_wrap here, and it would mean you wouldn't need to print spaces to indent (because pr_wrap should do it for you). It all looks sensible and equivalent to the old code, and the output is cleaner too, so: Reviewed-by: Richard W.M. Jones <rjones@redhat.com> Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com nbdkit - Flexible, fast NBD server with plugins https://gitlab.com/nbdkit/nbdkit
On Tue, Jun 07, 2022 at 09:00:08AM +0100, Richard W.M. Jones wrote:
On Mon, Jun 06, 2022 at 09:08:29PM -0500, Eric Blake wrote:
+ pr ":nbd_%s\",\n" name;
You could put this pr (but without the \n) ...
+ pr " "; + pr_wrap ',' (fun () ->
... inside pr_wrap here, and it would mean you wouldn't need to print spaces to indent (because pr_wrap should do it for you).
Will do. It rearranges a few more lines of generated code (now the &py_h argument can sometimes be a line earlier), but is still legible.
It all looks sensible and equivalent to the old code, and the output is cleaner too, so:
Reviewed-by: Richard W.M. Jones <rjones@redhat.com>
I get when we have more than one statement to bundle how the OCaml (fun () -> ... ) construct makes sense, but do we need that for this hunk?
@@ -284,7 +285,10 @@ let (* Generate the Python binding. *) let print_python_binding name { args; optargs; ret; may_set_error } = pr "PyObject *\n"; - pr "nbd_internal_py_%s (PyObject *self, PyObject *args)\n" name; + pr "nbd_internal_py_%s (" name; + pr_wrap ',' (fun () -> + pr "PyObject *self, PyObject *args"); + pr ")\n"; pr "{\n"; pr " PyObject *py_h;\n"; pr " struct nbd_handle *h;\n";
or is there a shorter way to write that one? -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
On Tue, Jun 07, 2022 at 08:53:23AM -0500, Eric Blake wrote:
On Tue, Jun 07, 2022 at 09:00:08AM +0100, Richard W.M. Jones wrote:
On Mon, Jun 06, 2022 at 09:08:29PM -0500, Eric Blake wrote:
+ pr ":nbd_%s\",\n" name;
You could put this pr (but without the \n) ...
+ pr " "; + pr_wrap ',' (fun () ->
... inside pr_wrap here, and it would mean you wouldn't need to print spaces to indent (because pr_wrap should do it for you).
Will do. It rearranges a few more lines of generated code (now the &py_h argument can sometimes be a line earlier), but is still legible.
It all looks sensible and equivalent to the old code, and the output is cleaner too, so:
Reviewed-by: Richard W.M. Jones <rjones@redhat.com>
I get when we have more than one statement to bundle how the OCaml (fun () -> ... ) construct makes sense, but do we need that for this hunk?
@@ -284,7 +285,10 @@ let (* Generate the Python binding. *) let print_python_binding name { args; optargs; ret; may_set_error } = pr "PyObject *\n"; - pr "nbd_internal_py_%s (PyObject *self, PyObject *args)\n" name; + pr "nbd_internal_py_%s (" name; + pr_wrap ',' (fun () -> + pr "PyObject *self, PyObject *args"); + pr ")\n"; pr "{\n"; pr " PyObject *py_h;\n"; pr " struct nbd_handle *h;\n";
or is there a shorter way to write that one?
I don't think so (except to write it all on a single line). The pr_wrap function is a bit weird in that it requires a sub-function that generates the buffer that it then wraps, rather than just taking a string parameter directly. Could imagine a pr_wrap_string function that took the string buffer directly, but that doesn't exist today. Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com nbdkit - Flexible, fast NBD server with plugins https://gitlab.com/nbdkit/nbdkit
Instead of having methods.c poking into the internals of a struct that we defined, have it use PyMemoryView* and Py_buffer*; this separation makes it easier to separate how we store the persistent data (for now, in a PyCapsule) from how we access the data. Eventually, the use of PyMemoryView* will make it easier for future patches to get rid of some layers of copying. nbd_internal_py_init_aio_buffer() is new; for now it doesn't fail, but it can in a future patch. For now, we still have to hang on to a python reference to the PyCapsule object holding our malloc'd C pointer, and since the MemoryView we just created is not backed by a Python object, we must ensure we don't leak it to Python code. But this will change in an upcoming patch. Generated code changes as follows: | --- python/methods.h.bak 2022-06-06 16:26:40.363250044 -0500 | +++ python/methods.h 2022-06-06 16:37:13.625328491 -0500 | @@ -28,17 +28,12 @@ | | #include <assert.h> | | -struct py_aio_buffer { | - Py_ssize_t len; | - void *data; | - bool initialized; | -}; | - | extern char **nbd_internal_py_get_string_list (PyObject *); | extern void nbd_internal_py_free_string_list (char **); | extern int nbd_internal_py_get_sockaddr (PyObject *, | struct sockaddr_storage *, socklen_t *); | -extern struct py_aio_buffer *nbd_internal_py_get_aio_buffer (PyObject *); | +extern PyObject *nbd_internal_py_get_aio_view (PyObject *, bool); | +extern int nbd_internal_py_init_aio_buffer (PyObject *); | | static inline struct nbd_handle * | get_handle (PyObject *obj) | --- python/methods.c.bak 2022-06-06 16:26:40.360250039 -0500 | +++ python/methods.c 2022-06-06 16:37:13.640328513 -0500 | @@ -3080,7 +3080,8 @@ nbd_internal_py_aio_pread (PyObject *sel | int64_t ret; | PyObject *py_ret = NULL; | PyObject *buf; /* PyCapsule pointing to struct py_aio_buffer */ | - struct py_aio_buffer *buf_buf; | + PyObject *buf_view = NULL; /* PyMemoryView of buf */ | + Py_buffer *py_buf; /* buffer of buf_view */ | uint64_t offset_u64; | unsigned long long offset; /* really uint64_t */ | struct user_data *completion_user_data = NULL; | @@ -3110,16 +3111,17 @@ nbd_internal_py_aio_pread (PyObject *sel | else | completion.callback = NULL; /* we're not going to call it */ | flags_u32 = flags; | - buf_buf = nbd_internal_py_get_aio_buffer (buf); | - if (!buf_buf) goto out; | + buf_view = nbd_internal_py_get_aio_view (buf, false); | + if (!buf_view) goto out; | + py_buf = PyMemoryView_GET_BUFFER (buf_view); | /* Increment refcount since buffer may be saved by libnbd. */ | Py_INCREF (buf); | completion_user_data->buf = buf; | offset_u64 = offset; | | - buf_buf->initialized = true; | - ret = nbd_aio_pread (h, buf_buf->data, buf_buf->len, offset_u64, | - completion, flags_u32); | + if (nbd_internal_py_init_aio_buffer (buf) < 0) goto out; | + ret = nbd_aio_pread (h, py_buf->buf, py_buf->len, offset_u64, completion, | + flags_u32); | completion_user_data = NULL; | if (ret == -1) { | raise_exception (); | @@ -3128,6 +3130,7 @@ nbd_internal_py_aio_pread (PyObject *sel | py_ret = PyLong_FromLongLong (ret); | | out: | + Py_XDECREF (buf_view); | free_user_data (completion_user_data); | return py_ret; | } | @@ -3140,7 +3143,8 @@ nbd_internal_py_aio_pread_structured (Py | int64_t ret; | PyObject *py_ret = NULL; | PyObject *buf; /* PyCapsule pointing to struct py_aio_buffer */ | - struct py_aio_buffer *buf_buf; | + PyObject *buf_view = NULL; /* PyMemoryView of buf */ | + Py_buffer *py_buf; /* buffer of buf_view */ | uint64_t offset_u64; | unsigned long long offset; /* really uint64_t */ | struct user_data *chunk_user_data = NULL; | @@ -3175,8 +3179,9 @@ nbd_internal_py_aio_pread_structured (Py | else | completion.callback = NULL; /* we're not going to call it */ | flags_u32 = flags; | - buf_buf = nbd_internal_py_get_aio_buffer (buf); | - if (!buf_buf) goto out; | + buf_view = nbd_internal_py_get_aio_view (buf, false); | + if (!buf_view) goto out; | + py_buf = PyMemoryView_GET_BUFFER (buf_view); | /* Increment refcount since buffer may be saved by libnbd. */ | Py_INCREF (buf); | completion_user_data->buf = buf; | @@ -3192,9 +3197,9 @@ nbd_internal_py_aio_pread_structured (Py | Py_INCREF (py_chunk_fn); | chunk_user_data->fn = py_chunk_fn; | | - buf_buf->initialized = true; | - ret = nbd_aio_pread_structured (h, buf_buf->data, buf_buf->len, | - offset_u64, chunk, completion, flags_u32); | + if (nbd_internal_py_init_aio_buffer (buf) < 0) goto out; | + ret = nbd_aio_pread_structured (h, py_buf->buf, py_buf->len, offset_u64, | + chunk, completion, flags_u32); | chunk_user_data = NULL; | completion_user_data = NULL; | if (ret == -1) { | @@ -3204,6 +3209,7 @@ nbd_internal_py_aio_pread_structured (Py | py_ret = PyLong_FromLongLong (ret); | | out: | + Py_XDECREF (buf_view); | free_user_data (chunk_user_data); | free_user_data (completion_user_data); | return py_ret; | @@ -3217,7 +3223,8 @@ nbd_internal_py_aio_pwrite (PyObject *se | int64_t ret; | PyObject *py_ret = NULL; | PyObject *buf; /* PyCapsule pointing to struct py_aio_buffer */ | - struct py_aio_buffer *buf_buf; | + PyObject *buf_view = NULL; /* PyMemoryView of buf */ | + Py_buffer *py_buf; /* buffer of buf_view */ | uint64_t offset_u64; | unsigned long long offset; /* really uint64_t */ | struct user_data *completion_user_data = NULL; | @@ -3247,19 +3254,16 @@ nbd_internal_py_aio_pwrite (PyObject *se | else | completion.callback = NULL; /* we're not going to call it */ | flags_u32 = flags; | - buf_buf = nbd_internal_py_get_aio_buffer (buf); | - if (!buf_buf) goto out; | + buf_view = nbd_internal_py_get_aio_view (buf, true); | + if (!buf_view) goto out; | + py_buf = PyMemoryView_GET_BUFFER (buf_view); | /* Increment refcount since buffer may be saved by libnbd. */ | Py_INCREF (buf); | completion_user_data->buf = buf; | offset_u64 = offset; | | - if (!buf_buf->initialized) { | - memset (buf_buf->data, 0, buf_buf->len); | - buf_buf->initialized = true; | - } | - ret = nbd_aio_pwrite (h, buf_buf->data, buf_buf->len, offset_u64, | - completion, flags_u32); | + ret = nbd_aio_pwrite (h, py_buf->buf, py_buf->len, offset_u64, completion, | + flags_u32); | completion_user_data = NULL; | if (ret == -1) { | raise_exception (); | @@ -3268,6 +3272,7 @@ nbd_internal_py_aio_pwrite (PyObject *se | py_ret = PyLong_FromLongLong (ret); | | out: | + Py_XDECREF (buf_view); | free_user_data (completion_user_data); | return py_ret; | } --- generator/Python.ml | 38 ++++++++++++++++---------------- python/handle.c | 53 ++++++++++++++++++++++++++++++++++++--------- 2 files changed, 62 insertions(+), 29 deletions(-) diff --git a/generator/Python.ml b/generator/Python.ml index 6244c8e..1afe0cf 100644 --- a/generator/Python.ml +++ b/generator/Python.ml @@ -34,17 +34,12 @@ let pr "#include <assert.h>\n"; pr "\n"; pr "\ -struct py_aio_buffer { - Py_ssize_t len; - void *data; - bool initialized; -}; - extern char **nbd_internal_py_get_string_list (PyObject *); extern void nbd_internal_py_free_string_list (char **); extern int nbd_internal_py_get_sockaddr (PyObject *, struct sockaddr_storage *, socklen_t *); -extern struct py_aio_buffer *nbd_internal_py_get_aio_buffer (PyObject *); +extern PyObject *nbd_internal_py_get_aio_view (PyObject *, bool); +extern int nbd_internal_py_init_aio_buffer (PyObject *); static inline struct nbd_handle * get_handle (PyObject *obj) @@ -306,7 +301,8 @@ let | BytesPersistOut (n, _) -> pr " PyObject *%s; /* PyCapsule pointing to struct py_aio_buffer */\n" n; - pr " struct py_aio_buffer *%s_buf;\n" n + pr " PyObject *%s_view = NULL; /* PyMemoryView of %s */\n" n n; + pr " Py_buffer *py_%s; /* buffer of %s_view */\n" n n | Closure { cbname } -> pr " struct user_data *%s_user_data = NULL;\n" cbname; pr " PyObject *py_%s_fn;\n" cbname; @@ -366,7 +362,7 @@ let "n", sprintf "&%s" count, sprintf "PyByteArray_AS_STRING (%s), %s" n count | BytesPersistIn (n, _) | BytesPersistOut (n, _) -> - "O", sprintf "&%s" n, sprintf "%s_buf->data, %s_buf->len" n n + "O", sprintf "&%s" n, sprintf "py_%s->buf, py_%s->len" n n | Closure { cbname } -> "O", sprintf "&py_%s_fn" cbname, cbname | Enum (n, _) -> "i", sprintf "&%s" n, n | Flags (n, _) -> "I", sprintf "&%s" n, sprintf "%s_u32" n @@ -435,9 +431,17 @@ let | BytesOut (n, count) -> pr " %s = PyByteArray_FromStringAndSize (NULL, %s);\n" n count; pr " if (%s == NULL) goto out;\n" n - | BytesPersistIn (n, _) | BytesPersistOut (n, _) -> - pr " %s_buf = nbd_internal_py_get_aio_buffer (%s);\n" n n; - pr " if (!%s_buf) goto out;\n" n; + | BytesPersistIn (n, _) -> + pr " %s_view = nbd_internal_py_get_aio_view (%s, true);\n" n n; + pr " if (!%s_view) goto out;\n" n; + pr " py_%s = PyMemoryView_GET_BUFFER (%s_view);\n" n n; + pr " /* Increment refcount since buffer may be saved by libnbd. */\n"; + pr " Py_INCREF (%s);\n" n; + pr " completion_user_data->buf = %s;\n" n + | BytesPersistOut (n, _) -> + pr " %s_view = nbd_internal_py_get_aio_view (%s, false);\n" n n; + pr " if (!%s_view) goto out;\n" n; + pr " py_%s = PyMemoryView_GET_BUFFER (%s_view);\n" n n; pr " /* Increment refcount since buffer may be saved by libnbd. */\n"; pr " Py_INCREF (%s);\n" n; pr " completion_user_data->buf = %s;\n" n @@ -477,13 +481,8 @@ let (* Second pass, and call the underlying C function. *) List.iter ( function - | BytesPersistIn (n, _) -> - pr " if (!%s_buf->initialized) {\n" n; - pr " memset (%s_buf->data, 0, %s_buf->len);\n" n n; - pr " %s_buf->initialized = true;\n" n; - pr " }\n" | BytesPersistOut (n, _) -> - pr " %s_buf->initialized = true;\n" n + pr " if (nbd_internal_py_init_aio_buffer (%s) < 0) goto out;\n" n | _ -> () ) args; pr " ret = nbd_%s (" name; @@ -550,7 +549,8 @@ let pr " if (%s.obj)\n" n; pr " PyBuffer_Release (&%s);\n" n | BytesOut (n, _) -> pr " Py_XDECREF (%s);\n" n - | BytesPersistIn _ | BytesPersistOut _ -> () + | BytesPersistIn (n, _) | BytesPersistOut (n, _) -> + pr " Py_XDECREF (%s_view);\n" n | Closure { cbname } -> pr " free_user_data (%s_user_data);\n" cbname | Enum _ -> () diff --git a/python/handle.c b/python/handle.c index d42a563..61dd736 100644 --- a/python/handle.c +++ b/python/handle.c @@ -40,6 +40,12 @@ #include "methods.h" +struct py_aio_buffer { + Py_ssize_t len; + void *data; + bool initialized; +}; + static inline PyObject * put_handle (struct nbd_handle *h) { @@ -98,12 +104,41 @@ nbd_internal_py_display_version (PyObject *self, PyObject *args) static const char aio_buffer_name[] = "nbd.Buffer"; -struct py_aio_buffer * +/* Return internal buffer pointer of nbd.Buffer */ +static struct py_aio_buffer * nbd_internal_py_get_aio_buffer (PyObject *capsule) { return PyCapsule_GetPointer (capsule, aio_buffer_name); } +/* Return new reference to MemoryView wrapping aio_buffer contents */ +PyObject * +nbd_internal_py_get_aio_view (PyObject *capsule, bool require_init) +{ + struct py_aio_buffer *buf = nbd_internal_py_get_aio_buffer (capsule); + + if (!buf) + return NULL; + + if (require_init && !buf->initialized) { + memset (buf->data, 0, buf->len); + buf->initialized = true; + } + + return PyMemoryView_FromMemory (buf->data, buf->len, + require_init ? PyBUF_READ : PyBUF_WRITE); +} + +int +nbd_internal_py_init_aio_buffer (PyObject *capsule) +{ + struct py_aio_buffer *buf = nbd_internal_py_get_aio_buffer (capsule); + + assert (buf); + buf->initialized = true; + return 0; +} + static void free_aio_buffer (PyObject *capsule) { @@ -219,23 +254,21 @@ PyObject * nbd_internal_py_aio_buffer_to_bytearray (PyObject *self, PyObject *args) { PyObject *obj; - struct py_aio_buffer *buf; + PyObject *view; + PyObject *ret; if (!PyArg_ParseTuple (args, "O:nbd_internal_py_aio_buffer_to_bytearray", &obj)) return NULL; - buf = nbd_internal_py_get_aio_buffer (obj); - if (buf == NULL) + view = nbd_internal_py_get_aio_view (obj, true); + if (view == NULL) return NULL; - if (!buf->initialized) { - memset (buf->data, 0, buf->len); - buf->initialized = true; - } - - return PyByteArray_FromStringAndSize (buf->data, buf->len); + ret = PyByteArray_FromObject (view); + Py_DECREF (view); + return ret; } PyObject * -- 2.36.1
On Mon, Jun 06, 2022 at 09:08:30PM -0500, Eric Blake wrote:
Instead of having methods.c poking into the internals of a struct that we defined, have it use PyMemoryView* and Py_buffer*; this separation makes it easier to separate how we store the persistent data (for now, in a PyCapsule) from how we access the data. Eventually, the use of PyMemoryView* will make it easier for future patches to get rid of some layers of copying. nbd_internal_py_init_aio_buffer() is new; for now it doesn't fail, but it can in a future patch.
For now, we still have to hang on to a python reference to the PyCapsule object holding our malloc'd C pointer, and since the MemoryView we just created is not backed by a Python object, we must ensure we don't leak it to Python code. But this will change in an upcoming patch.
[...]
@@ -477,13 +481,8 @@ let (* Second pass, and call the underlying C function. *) List.iter ( function - | BytesPersistIn (n, _) -> - pr " if (!%s_buf->initialized) {\n" n; - pr " memset (%s_buf->data, 0, %s_buf->len);\n" n n; - pr " %s_buf->initialized = true;\n" n; - pr " }\n" | BytesPersistOut (n, _) -> - pr " %s_buf->initialized = true;\n" n + pr " if (nbd_internal_py_init_aio_buffer (%s) < 0) goto out;\n" n
Can't fail, but that was noted in the commit message so that's OK. Reviewed-by: Richard W.M. Jones <rjones@redhat.com> 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
Prior to this commit, we were unwrapping buf._o in nbd.py, which causes cryptic errors when the user passes in the wrong type: $ nbdkit -U - memory 10 --run \ 'nbdsh -u "$uri" -c "h.aio_pread(bytearray(10), 0)"' Traceback (most recent call last): ... File "/usr/lib64/python3.10/site-packages/nbd.py", line 2204, in aio_pread return libnbdmod.aio_pread(self._o, buf._o, offset, completion, AttributeError: 'bytearray' object has no attribute '_o' or worse, if the user passes in an unrelated type that does have an attribute "_o" but is not a PyCapsule wrapper: $ nbdkit -U - memory 10 --run 'nbdsh -u "$uri" -c " class foo(object): def __init__(self): self._o = 1 h.aio_pread(foo(), 0) "' Traceback (most recent call last): ... File "/usr/lib64/python3.10/site-packages/nbd.py", line 2204, in aio_pread return libnbdmod.aio_pread(self._o, buf._o, offset, completion, ValueError: PyCapsule_GetPointer called with invalid PyCapsule object Change things to instead do the unwrapping in our helper function. This will also make it easier for an upcoming patch to accept more types than just nbd.Buffer, with the goal of accepting any python buffer-like object. For now, passing in the wrong type still fails, but with a nicer message: $ ./run nbdkit -U - memory 10 --run \ 'nbdsh -u "$uri" -c "h.aio_pread(bytearray(10), 0)"' Traceback (most recent call last): ... File "/home/eblake/libnbd/python/nbd.py", line 2204, in aio_pread return libnbdmod.aio_pread(self._o, buf, offset, completion, flags) TypeError: aio_buffer: expecting nbd.Buffer instance The next patch will clean up the OCaml code, now that none of the parameter types need alternative text. The generated code changes as follows: | --- python/methods.h.bak 2022-06-06 16:55:27.549200523 -0500 | +++ python/methods.h 2022-06-06 16:55:34.097211464 -0500 | @@ -34,6 +34,7 @@ | struct sockaddr_storage *, socklen_t *); | extern PyObject *nbd_internal_py_get_aio_view (PyObject *, bool); | extern int nbd_internal_py_init_aio_buffer (PyObject *); | +extern PyObject *nbd_internal_py_get_nbd_buffer_type (void); | | static inline struct nbd_handle * | get_handle (PyObject *obj) | --- python/methods.c.bak 2022-06-06 16:55:27.543200513 -0500 | +++ python/methods.c 2022-06-06 16:55:34.114211492 -0500 | @@ -3079,7 +3079,7 @@ nbd_internal_py_aio_pread (PyObject *sel | struct nbd_handle *h; | int64_t ret; | PyObject *py_ret = NULL; | - PyObject *buf; /* PyCapsule pointing to struct py_aio_buffer */ | + PyObject *buf; /* Instance of nbd.Buffer */ | PyObject *buf_view = NULL; /* PyMemoryView of buf */ | Py_buffer *py_buf; /* buffer of buf_view */ | uint64_t offset_u64; | @@ -3142,7 +3142,7 @@ nbd_internal_py_aio_pread_structured (Py | struct nbd_handle *h; | int64_t ret; | PyObject *py_ret = NULL; | - PyObject *buf; /* PyCapsule pointing to struct py_aio_buffer */ | + PyObject *buf; /* Instance of nbd.Buffer */ | PyObject *buf_view = NULL; /* PyMemoryView of buf */ | Py_buffer *py_buf; /* buffer of buf_view */ | uint64_t offset_u64; | @@ -3222,7 +3222,7 @@ nbd_internal_py_aio_pwrite (PyObject *se | struct nbd_handle *h; | int64_t ret; | PyObject *py_ret = NULL; | - PyObject *buf; /* PyCapsule pointing to struct py_aio_buffer */ | + PyObject *buf; /* Instance of nbd.Buffer */ | PyObject *buf_view = NULL; /* PyMemoryView of buf */ | Py_buffer *py_buf; /* buffer of buf_view */ | uint64_t offset_u64; | --- python/nbd.py.bak 2022-06-06 16:55:27.552200528 -0500 | +++ python/nbd.py 2022-06-06 16:55:34.123211507 -0500 | @@ -141,7 +141,7 @@ class Buffer(object): | | def to_bytearray(self): | '''Copy an AIO buffer into a bytearray.''' | - return libnbdmod.aio_buffer_to_bytearray(self._o) | + return libnbdmod.aio_buffer_to_bytearray(self) | | def size(self): | '''Return the size of an AIO buffer.''' | @@ -161,7 +161,7 @@ class Buffer(object): | always returns true. If size > 0, we check the interval | [offset..offset+size-1]. | ''' | - return libnbdmod.aio_buffer_is_zero(self._o, offset, size) | + return libnbdmod.aio_buffer_is_zero(self, offset, size) | | | class NBD(object): | @@ -2208,8 +2208,7 @@ class NBD(object): | alter which scenarios should await a server reply rather | than failing fast. | ''' | - return libnbdmod.aio_pread(self._o, buf._o, offset, completion, | - flags) | + return libnbdmod.aio_pread(self._o, buf, offset, completion, flags) | | def aio_pread_structured(self, buf, offset, chunk, completion=None, | flags=0): | @@ -2243,8 +2242,8 @@ class NBD(object): | alter which scenarios should await a server reply rather | than failing fast. | ''' | - return libnbdmod.aio_pread_structured(self._o, buf._o, offset, | - chunk, completion, flags) | + return libnbdmod.aio_pread_structured(self._o, buf, offset, chunk, | + completion, flags) | | def aio_pwrite(self, buf, offset, completion=None, flags=0): | u'''▶ write to the NBD server | @@ -2267,8 +2266,7 @@ class NBD(object): | alter which scenarios should await a server reply rather | than failing fast. | ''' | - return libnbdmod.aio_pwrite(self._o, buf._o, offset, completion, | - flags) | + return libnbdmod.aio_pwrite(self._o, buf, offset, completion, flags) | | def aio_disconnect(self, flags=0): | u'''▶ disconnect from the NBD server --- generator/Python.ml | 14 +++++++------- python/handle.c | 20 ++++++++++++++------ python/utils.c | 20 +++++++++++++++++++- 3 files changed, 40 insertions(+), 14 deletions(-) diff --git a/generator/Python.ml b/generator/Python.ml index 1afe0cf..101f3e0 100644 --- a/generator/Python.ml +++ b/generator/Python.ml @@ -40,6 +40,7 @@ let struct sockaddr_storage *, socklen_t *); extern PyObject *nbd_internal_py_get_aio_view (PyObject *, bool); extern int nbd_internal_py_init_aio_buffer (PyObject *); +extern PyObject *nbd_internal_py_get_nbd_buffer_type (void); static inline struct nbd_handle * get_handle (PyObject *obj) @@ -299,8 +300,7 @@ let pr " Py_ssize_t %s;\n" count | BytesPersistIn (n, _) | BytesPersistOut (n, _) -> - pr " PyObject *%s; /* PyCapsule pointing to struct py_aio_buffer */\n" - n; + pr " PyObject *%s; /* Instance of nbd.Buffer */\n" n; pr " PyObject *%s_view = NULL; /* PyMemoryView of %s */\n" n n; pr " Py_buffer *py_%s; /* buffer of %s_view */\n" n n | Closure { cbname } -> @@ -755,11 +755,11 @@ let def to_bytearray(self): '''Copy an AIO buffer into a bytearray.''' - return libnbdmod.aio_buffer_to_bytearray(self._o) + return libnbdmod.aio_buffer_to_bytearray(self) def size(self): '''Return the size of an AIO buffer.''' - return libnbdmod.aio_buffer_size(self._o) + return libnbdmod.aio_buffer_size(self) def is_zero(self, offset=0, size=-1): '''Returns true if and only if all bytes in the buffer are zeroes. @@ -775,7 +775,7 @@ let always returns true. If size > 0, we check the interval [offset..offset+size-1]. ''' - return libnbdmod.aio_buffer_is_zero(self._o, offset, size) + return libnbdmod.aio_buffer_is_zero(self, offset, size) class NBD(object): @@ -800,8 +800,8 @@ let | 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) + | BytesPersistIn (n, _) -> n, None, None + | BytesPersistOut (n, _) -> n, None, None | Closure { cbname } -> cbname, None, None | Enum (n, _) -> n, None, None | Flags (n, _) -> n, None, None diff --git a/python/handle.c b/python/handle.c index 61dd736..79b369d 100644 --- a/python/handle.c +++ b/python/handle.c @@ -106,16 +106,24 @@ static const char aio_buffer_name[] = "nbd.Buffer"; /* Return internal buffer pointer of nbd.Buffer */ static struct py_aio_buffer * -nbd_internal_py_get_aio_buffer (PyObject *capsule) +nbd_internal_py_get_aio_buffer (PyObject *object) { - return PyCapsule_GetPointer (capsule, aio_buffer_name); + if (PyObject_IsInstance (object, nbd_internal_py_get_nbd_buffer_type ())) { + PyObject *capsule = PyObject_GetAttrString(object, "_o"); + + return PyCapsule_GetPointer (capsule, aio_buffer_name); + } + + PyErr_SetString (PyExc_TypeError, + "aio_buffer: expecting nbd.Buffer instance"); + return NULL; } /* Return new reference to MemoryView wrapping aio_buffer contents */ PyObject * -nbd_internal_py_get_aio_view (PyObject *capsule, bool require_init) +nbd_internal_py_get_aio_view (PyObject *object, bool require_init) { - struct py_aio_buffer *buf = nbd_internal_py_get_aio_buffer (capsule); + struct py_aio_buffer *buf = nbd_internal_py_get_aio_buffer (object); if (!buf) return NULL; @@ -130,9 +138,9 @@ nbd_internal_py_get_aio_view (PyObject *capsule, bool require_init) } int -nbd_internal_py_init_aio_buffer (PyObject *capsule) +nbd_internal_py_init_aio_buffer (PyObject *object) { - struct py_aio_buffer *buf = nbd_internal_py_get_aio_buffer (capsule); + struct py_aio_buffer *buf = nbd_internal_py_get_aio_buffer (object); assert (buf); buf->initialized = true; diff --git a/python/utils.c b/python/utils.c index 37f0c55..e0df181 100644 --- a/python/utils.c +++ b/python/utils.c @@ -1,5 +1,5 @@ /* NBD client library in userspace - * Copyright (C) 2013-2019 Red Hat Inc. + * Copyright (C) 2013-2022 Red Hat Inc. * * This library is free software; you can redistribute it and/or * modify it under the terms of the GNU Lesser General Public @@ -155,3 +155,21 @@ nbd_internal_py_get_sockaddr (PyObject *addr, return -1; } } + +/* Obtain the type object for nbd.Buffer */ +PyObject * +nbd_internal_py_get_nbd_buffer_type (void) +{ + static PyObject *type; + + if (!type) { + PyObject *modname = PyUnicode_FromString ("nbd"); + PyObject *module = PyImport_Import (modname); + assert (module); + type = PyObject_GetAttrString (module, "Buffer"); + assert (type); + Py_DECREF (modname); + Py_DECREF (module); + } + return type; +} -- 2.36.1
On Mon, Jun 06, 2022 at 09:08:31PM -0500, Eric Blake wrote:
Prior to this commit, we were unwrapping buf._o in nbd.py, which causes cryptic errors when the user passes in the wrong type:
$ nbdkit -U - memory 10 --run \ 'nbdsh -u "$uri" -c "h.aio_pread(bytearray(10), 0)"' Traceback (most recent call last): ... File "/usr/lib64/python3.10/site-packages/nbd.py", line 2204, in aio_pread return libnbdmod.aio_pread(self._o, buf._o, offset, completion, AttributeError: 'bytearray' object has no attribute '_o'
or worse, if the user passes in an unrelated type that does have an attribute "_o" but is not a PyCapsule wrapper:
$ nbdkit -U - memory 10 --run 'nbdsh -u "$uri" -c " class foo(object): def __init__(self): self._o = 1 h.aio_pread(foo(), 0) "' Traceback (most recent call last): ... File "/usr/lib64/python3.10/site-packages/nbd.py", line 2204, in aio_pread return libnbdmod.aio_pread(self._o, buf._o, offset, completion, ValueError: PyCapsule_GetPointer called with invalid PyCapsule object
Change things to instead do the unwrapping in our helper function. This will also make it easier for an upcoming patch to accept more types than just nbd.Buffer, with the goal of accepting any python buffer-like object. For now, passing in the wrong type still fails, but with a nicer message:
$ ./run nbdkit -U - memory 10 --run \ 'nbdsh -u "$uri" -c "h.aio_pread(bytearray(10), 0)"' Traceback (most recent call last): ... File "/home/eblake/libnbd/python/nbd.py", line 2204, in aio_pread return libnbdmod.aio_pread(self._o, buf, offset, completion, flags) TypeError: aio_buffer: expecting nbd.Buffer instance
The next patch will clean up the OCaml code, now that none of the parameter types need alternative text. The generated code changes as follows:
| --- python/methods.h.bak 2022-06-06 16:55:27.549200523 -0500 | +++ python/methods.h 2022-06-06 16:55:34.097211464 -0500 | @@ -34,6 +34,7 @@ | struct sockaddr_storage *, socklen_t *); | extern PyObject *nbd_internal_py_get_aio_view (PyObject *, bool); | extern int nbd_internal_py_init_aio_buffer (PyObject *); | +extern PyObject *nbd_internal_py_get_nbd_buffer_type (void); | | static inline struct nbd_handle * | get_handle (PyObject *obj) | --- python/methods.c.bak 2022-06-06 16:55:27.543200513 -0500 | +++ python/methods.c 2022-06-06 16:55:34.114211492 -0500 | @@ -3079,7 +3079,7 @@ nbd_internal_py_aio_pread (PyObject *sel | struct nbd_handle *h; | int64_t ret; | PyObject *py_ret = NULL; | - PyObject *buf; /* PyCapsule pointing to struct py_aio_buffer */ | + PyObject *buf; /* Instance of nbd.Buffer */ | PyObject *buf_view = NULL; /* PyMemoryView of buf */ | Py_buffer *py_buf; /* buffer of buf_view */ | uint64_t offset_u64; | @@ -3142,7 +3142,7 @@ nbd_internal_py_aio_pread_structured (Py | struct nbd_handle *h; | int64_t ret; | PyObject *py_ret = NULL; | - PyObject *buf; /* PyCapsule pointing to struct py_aio_buffer */ | + PyObject *buf; /* Instance of nbd.Buffer */ | PyObject *buf_view = NULL; /* PyMemoryView of buf */ | Py_buffer *py_buf; /* buffer of buf_view */ | uint64_t offset_u64; | @@ -3222,7 +3222,7 @@ nbd_internal_py_aio_pwrite (PyObject *se | struct nbd_handle *h; | int64_t ret; | PyObject *py_ret = NULL; | - PyObject *buf; /* PyCapsule pointing to struct py_aio_buffer */ | + PyObject *buf; /* Instance of nbd.Buffer */ | PyObject *buf_view = NULL; /* PyMemoryView of buf */ | Py_buffer *py_buf; /* buffer of buf_view */ | uint64_t offset_u64; | --- python/nbd.py.bak 2022-06-06 16:55:27.552200528 -0500 | +++ python/nbd.py 2022-06-06 16:55:34.123211507 -0500 | @@ -141,7 +141,7 @@ class Buffer(object): | | def to_bytearray(self): | '''Copy an AIO buffer into a bytearray.''' | - return libnbdmod.aio_buffer_to_bytearray(self._o) | + return libnbdmod.aio_buffer_to_bytearray(self) | | def size(self): | '''Return the size of an AIO buffer.''' | @@ -161,7 +161,7 @@ class Buffer(object): | always returns true. If size > 0, we check the interval | [offset..offset+size-1]. | ''' | - return libnbdmod.aio_buffer_is_zero(self._o, offset, size) | + return libnbdmod.aio_buffer_is_zero(self, offset, size) | | | class NBD(object): | @@ -2208,8 +2208,7 @@ class NBD(object): | alter which scenarios should await a server reply rather | than failing fast. | ''' | - return libnbdmod.aio_pread(self._o, buf._o, offset, completion, | - flags) | + return libnbdmod.aio_pread(self._o, buf, offset, completion, flags) | | def aio_pread_structured(self, buf, offset, chunk, completion=None, | flags=0): | @@ -2243,8 +2242,8 @@ class NBD(object): | alter which scenarios should await a server reply rather | than failing fast. | ''' | - return libnbdmod.aio_pread_structured(self._o, buf._o, offset, | - chunk, completion, flags) | + return libnbdmod.aio_pread_structured(self._o, buf, offset, chunk, | + completion, flags) | | def aio_pwrite(self, buf, offset, completion=None, flags=0): | u'''▶ write to the NBD server | @@ -2267,8 +2266,7 @@ class NBD(object): | alter which scenarios should await a server reply rather | than failing fast. | ''' | - return libnbdmod.aio_pwrite(self._o, buf._o, offset, completion, | - flags) | + return libnbdmod.aio_pwrite(self._o, buf, offset, completion, flags) | | def aio_disconnect(self, flags=0): | u'''▶ disconnect from the NBD server --- generator/Python.ml | 14 +++++++------- python/handle.c | 20 ++++++++++++++------ python/utils.c | 20 +++++++++++++++++++- 3 files changed, 40 insertions(+), 14 deletions(-)
diff --git a/generator/Python.ml b/generator/Python.ml index 1afe0cf..101f3e0 100644 --- a/generator/Python.ml +++ b/generator/Python.ml @@ -40,6 +40,7 @@ let struct sockaddr_storage *, socklen_t *); extern PyObject *nbd_internal_py_get_aio_view (PyObject *, bool); extern int nbd_internal_py_init_aio_buffer (PyObject *); +extern PyObject *nbd_internal_py_get_nbd_buffer_type (void);
static inline struct nbd_handle * get_handle (PyObject *obj) @@ -299,8 +300,7 @@ let pr " Py_ssize_t %s;\n" count | BytesPersistIn (n, _) | BytesPersistOut (n, _) -> - pr " PyObject *%s; /* PyCapsule pointing to struct py_aio_buffer */\n" - n; + pr " PyObject *%s; /* Instance of nbd.Buffer */\n" n; pr " PyObject *%s_view = NULL; /* PyMemoryView of %s */\n" n n; pr " Py_buffer *py_%s; /* buffer of %s_view */\n" n n | Closure { cbname } -> @@ -755,11 +755,11 @@ let
def to_bytearray(self): '''Copy an AIO buffer into a bytearray.''' - return libnbdmod.aio_buffer_to_bytearray(self._o) + return libnbdmod.aio_buffer_to_bytearray(self)
def size(self): '''Return the size of an AIO buffer.''' - return libnbdmod.aio_buffer_size(self._o) + return libnbdmod.aio_buffer_size(self)
def is_zero(self, offset=0, size=-1): '''Returns true if and only if all bytes in the buffer are zeroes. @@ -775,7 +775,7 @@ let always returns true. If size > 0, we check the interval [offset..offset+size-1]. ''' - return libnbdmod.aio_buffer_is_zero(self._o, offset, size) + return libnbdmod.aio_buffer_is_zero(self, offset, size)
class NBD(object): @@ -800,8 +800,8 @@ let | 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) + | BytesPersistIn (n, _) -> n, None, None + | BytesPersistOut (n, _) -> n, None, None | Closure { cbname } -> cbname, None, None | Enum (n, _) -> n, None, None | Flags (n, _) -> n, None, None diff --git a/python/handle.c b/python/handle.c index 61dd736..79b369d 100644 --- a/python/handle.c +++ b/python/handle.c @@ -106,16 +106,24 @@ static const char aio_buffer_name[] = "nbd.Buffer";
/* Return internal buffer pointer of nbd.Buffer */ static struct py_aio_buffer * -nbd_internal_py_get_aio_buffer (PyObject *capsule) +nbd_internal_py_get_aio_buffer (PyObject *object) { - return PyCapsule_GetPointer (capsule, aio_buffer_name); + if (PyObject_IsInstance (object, nbd_internal_py_get_nbd_buffer_type ())) { + PyObject *capsule = PyObject_GetAttrString(object, "_o"); + + return PyCapsule_GetPointer (capsule, aio_buffer_name); + } + + PyErr_SetString (PyExc_TypeError, + "aio_buffer: expecting nbd.Buffer instance"); + return NULL; }
/* Return new reference to MemoryView wrapping aio_buffer contents */ PyObject * -nbd_internal_py_get_aio_view (PyObject *capsule, bool require_init) +nbd_internal_py_get_aio_view (PyObject *object, bool require_init) { - struct py_aio_buffer *buf = nbd_internal_py_get_aio_buffer (capsule); + struct py_aio_buffer *buf = nbd_internal_py_get_aio_buffer (object);
if (!buf) return NULL; @@ -130,9 +138,9 @@ nbd_internal_py_get_aio_view (PyObject *capsule, bool require_init) }
int -nbd_internal_py_init_aio_buffer (PyObject *capsule) +nbd_internal_py_init_aio_buffer (PyObject *object) { - struct py_aio_buffer *buf = nbd_internal_py_get_aio_buffer (capsule); + struct py_aio_buffer *buf = nbd_internal_py_get_aio_buffer (object);
assert (buf); buf->initialized = true; diff --git a/python/utils.c b/python/utils.c index 37f0c55..e0df181 100644 --- a/python/utils.c +++ b/python/utils.c @@ -1,5 +1,5 @@ /* NBD client library in userspace - * Copyright (C) 2013-2019 Red Hat Inc. + * Copyright (C) 2013-2022 Red Hat Inc. * * This library is free software; you can redistribute it and/or * modify it under the terms of the GNU Lesser General Public @@ -155,3 +155,21 @@ nbd_internal_py_get_sockaddr (PyObject *addr, return -1; } } + +/* Obtain the type object for nbd.Buffer */ +PyObject * +nbd_internal_py_get_nbd_buffer_type (void) +{ + static PyObject *type; + + if (!type) { + PyObject *modname = PyUnicode_FromString ("nbd"); + PyObject *module = PyImport_Import (modname); + assert (module); + type = PyObject_GetAttrString (module, "Buffer"); + assert (type); + Py_DECREF (modname); + Py_DECREF (module); + } + return type; +}
Reviewed-by: Richard W.M. Jones <rjones@redhat.com> Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com nbdkit - Flexible, fast NBD server with plugins https://gitlab.com/nbdkit/nbdkit
Now that none of our parameter types uses a getter sequence, we can simplify the code for generating nbd.py. No change to generated output. --- generator/Python.ml | 49 ++++++++++++++++++++++----------------------- 1 file changed, 24 insertions(+), 25 deletions(-) diff --git a/generator/Python.ml b/generator/Python.ml index 101f3e0..c49af4f 100644 --- a/generator/Python.ml +++ b/generator/Python.ml @@ -797,31 +797,31 @@ let let args = List.map ( function - | Bool n -> n, None, None - | BytesIn (n, _) -> n, None, None - | BytesOut (_, count) -> count, None, None - | BytesPersistIn (n, _) -> n, None, None - | BytesPersistOut (n, _) -> n, None, None - | Closure { cbname } -> cbname, None, None - | Enum (n, _) -> n, None, None - | Flags (n, _) -> n, None, None - | Fd n | Int n -> n, None, None - | Int64 n -> n, None, None - | Path n -> n, None, None - | SizeT 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 - | UIntPtr n -> n, None, None + | Bool n -> n, None + | BytesIn (n, _) -> n, None + | BytesOut (_, count) -> count, None + | BytesPersistIn (n, _) -> n, None + | BytesPersistOut (n, _) -> n, None + | Closure { cbname } -> cbname, None + | Enum (n, _) -> n, None + | Flags (n, _) -> n, None + | Fd n | Int n -> n, None + | Int64 n -> n, None + | Path n -> n, None + | SizeT n -> n, None + | SockAddrAndLen (n, _) -> n, None + | String n -> n, None + | StringList n -> n, None + | UInt n -> n, None + | UInt32 n -> n, None + | UInt64 n -> n, None + | UIntPtr n -> n, None ) args in let optargs = List.map ( function - | OClosure { cbname } -> cbname, Some "None", None - | OFlags (n, _, _) -> n, Some "0", None + | OClosure { cbname } -> cbname, Some "None" + | OFlags (n, _, _) -> n, Some "0" ) optargs in let args = args @ optargs in pr " def %s(" name; @@ -829,8 +829,8 @@ let pr "self"; List.iter ( function - | n, None, _ -> pr ", %s" n - | n, Some default, _ -> pr ", %s=%s" n default + | 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.\\1>" longdesc in @@ -842,8 +842,7 @@ let pr "self._o"; List.iter ( function - | _, _, Some getter -> pr ", %s" getter - | n, _, None -> pr ", %s" n + | n, _ -> pr ", %s" n ) args); pr ")\n"; pr "\n" -- 2.36.1
On Mon, Jun 06, 2022 at 09:08:32PM -0500, Eric Blake wrote:
Now that none of our parameter types uses a getter sequence, we can simplify the code for generating nbd.py. No change to generated output. --- generator/Python.ml | 49 ++++++++++++++++++++++----------------------- 1 file changed, 24 insertions(+), 25 deletions(-)
diff --git a/generator/Python.ml b/generator/Python.ml index 101f3e0..c49af4f 100644 --- a/generator/Python.ml +++ b/generator/Python.ml @@ -797,31 +797,31 @@ let let args = List.map ( function - | Bool n -> n, None, None - | BytesIn (n, _) -> n, None, None - | BytesOut (_, count) -> count, None, None - | BytesPersistIn (n, _) -> n, None, None - | BytesPersistOut (n, _) -> n, None, None - | Closure { cbname } -> cbname, None, None - | Enum (n, _) -> n, None, None - | Flags (n, _) -> n, None, None - | Fd n | Int n -> n, None, None - | Int64 n -> n, None, None - | Path n -> n, None, None - | SizeT 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 - | UIntPtr n -> n, None, None + | Bool n -> n, None + | BytesIn (n, _) -> n, None + | BytesOut (_, count) -> count, None + | BytesPersistIn (n, _) -> n, None + | BytesPersistOut (n, _) -> n, None + | Closure { cbname } -> cbname, None + | Enum (n, _) -> n, None + | Flags (n, _) -> n, None + | Fd n | Int n -> n, None + | Int64 n -> n, None + | Path n -> n, None + | SizeT n -> n, None + | SockAddrAndLen (n, _) -> n, None + | String n -> n, None + | StringList n -> n, None + | UInt n -> n, None + | UInt32 n -> n, None + | UInt64 n -> n, None + | UIntPtr n -> n, None ) args in let optargs = List.map ( function - | OClosure { cbname } -> cbname, Some "None", None - | OFlags (n, _, _) -> n, Some "0", None + | OClosure { cbname } -> cbname, Some "None" + | OFlags (n, _, _) -> n, Some "0" ) optargs in let args = args @ optargs in pr " def %s(" name; @@ -829,8 +829,8 @@ let pr "self"; List.iter ( function - | n, None, _ -> pr ", %s" n - | n, Some default, _ -> pr ", %s=%s" n default + | 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.\\1>" longdesc in @@ -842,8 +842,7 @@ let pr "self._o"; List.iter ( function - | _, _, Some getter -> pr ", %s" getter - | n, _, None -> pr ", %s" n + | n, _ -> pr ", %s" n ) args); pr ")\n"; pr "\n"
Reviewed-by: Richard W.M. Jones <rjones@redhat.com> Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com nbdkit - Flexible, fast NBD server with plugins https://gitlab.com/nbdkit/nbdkit
Instead of storing a PyCapsule in _o and repeatedly doing lookups to dereference a stored malloc'd pointer, it is easier to just directly store a Python buffer-like object as _o. Track initialization via ._init: absent for nbd.Buffer(int), present for nbd.Buffer.from_bytearray or after we have forced initialization due to aio_pread or .to_bytearray. At this point, we are still copying in and out of .{from,to}_bytearray, but we are getting closer to reducing the number of copies. The sheer size in reduced lines of code is a testament to the ease of doing things closer to Python, rather than hidden behind unpacking a PyCapsule layer. Acceptable side effect: nbd.Buffer(-1) changes from: RuntimeError: length < 0 to SystemError: Negative size passed to PyByteArray_FromStringAndSize The generated code changes as follows: | --- python/methods.h.bak 2022-06-06 20:36:01.973327739 -0500 | +++ python/methods.h 2022-06-06 20:36:16.146352508 -0500 | @@ -62,9 +62,6 @@ | extern PyObject *nbd_internal_py_close (PyObject *self, PyObject *args); | extern PyObject *nbd_internal_py_display_version (PyObject *self, PyObject *args); | extern PyObject *nbd_internal_py_alloc_aio_buffer (PyObject *self, PyObject *args); | -extern PyObject *nbd_internal_py_aio_buffer_from_bytearray (PyObject *self, PyObject *args); | -extern PyObject *nbd_internal_py_aio_buffer_to_bytearray (PyObject *self, PyObject *args); | -extern PyObject *nbd_internal_py_aio_buffer_size (PyObject *self, PyObject *args); | extern PyObject *nbd_internal_py_aio_buffer_is_zero (PyObject *self, PyObject *args); | extern PyObject *nbd_internal_py_set_debug (PyObject *self, PyObject *args); | extern PyObject *nbd_internal_py_get_debug (PyObject *self, PyObject *args); | --- python/nbd.py.bak 2022-06-06 20:36:01.978327748 -0500 | +++ python/nbd.py 2022-06-06 20:56:41.866729560 -0500 | @@ -134,18 +134,21 @@ class Buffer(object): | bytearray constructor. Otherwise, ba is copied. Either way, the | resulting AIO buffer is independent from the original. | ''' | - o = libnbdmod.aio_buffer_from_bytearray(ba) | self = cls(0) | - self._o = o | + self._o = bytearray(ba) | + self._init = True | return self | | def to_bytearray(self): | '''Copy an AIO buffer into a bytearray.''' | - return libnbdmod.aio_buffer_to_bytearray(self) | + if not hasattr(self, '_init'): | + self._o = bytearray(len(self._o)) | + self._init = True | + return bytearray(self._o) | | def size(self): | '''Return the size of an AIO buffer.''' | - return libnbdmod.aio_buffer_size(self) | + return len(self._o) | | def is_zero(self, offset=0, size=-1): | '''Returns true if and only if all bytes in the buffer are zeroes. | @@ -161,7 +164,8 @@ class Buffer(object): | always returns true. If size > 0, we check the interval | [offset..offset+size-1]. | ''' | - return libnbdmod.aio_buffer_is_zero(self, offset, size) | + return libnbdmod.aio_buffer_is_zero(self._o, offset, size, | + hasattr(self, '_init')) | | | class NBD(object): --- generator/Python.ml | 20 ++-- python/handle.c | 242 +++++++++----------------------------------- 2 files changed, 56 insertions(+), 206 deletions(-) diff --git a/generator/Python.ml b/generator/Python.ml index c49af4f..86781ac 100644 --- a/generator/Python.ml +++ b/generator/Python.ml @@ -73,9 +73,6 @@ let ) ([ "create"; "close"; "display_version"; "alloc_aio_buffer"; - "aio_buffer_from_bytearray"; - "aio_buffer_to_bytearray"; - "aio_buffer_size"; "aio_buffer_is_zero"] @ List.map fst handle_calls); pr "\n"; @@ -105,9 +102,6 @@ let ) ([ "create"; "close"; "display_version"; "alloc_aio_buffer"; - "aio_buffer_from_bytearray"; - "aio_buffer_to_bytearray"; - "aio_buffer_size"; "aio_buffer_is_zero"] @ List.map fst handle_calls); pr " { NULL, NULL, 0, NULL }\n"; pr "};\n"; @@ -748,18 +742,21 @@ let bytearray constructor. Otherwise, ba is copied. Either way, the resulting AIO buffer is independent from the original. ''' - o = libnbdmod.aio_buffer_from_bytearray(ba) self = cls(0) - self._o = o + self._o = bytearray(ba) + self._init = True return self def to_bytearray(self): '''Copy an AIO buffer into a bytearray.''' - return libnbdmod.aio_buffer_to_bytearray(self) + if not hasattr(self, '_init'): + self._o = bytearray(len(self._o)) + self._init = True + return bytearray(self._o) def size(self): '''Return the size of an AIO buffer.''' - return libnbdmod.aio_buffer_size(self) + return len(self._o) def is_zero(self, offset=0, size=-1): '''Returns true if and only if all bytes in the buffer are zeroes. @@ -775,7 +772,8 @@ let always returns true. If size > 0, we check the interval [offset..offset+size-1]. ''' - return libnbdmod.aio_buffer_is_zero(self, offset, size) + return libnbdmod.aio_buffer_is_zero(self._o, offset, size, + hasattr(self, '_init')) class NBD(object): diff --git a/python/handle.c b/python/handle.c index 79b369d..ca6c8e9 100644 --- a/python/handle.c +++ b/python/handle.c @@ -40,12 +40,6 @@ #include "methods.h" -struct py_aio_buffer { - Py_ssize_t len; - void *data; - bool initialized; -}; - static inline PyObject * put_handle (struct nbd_handle *h) { @@ -102,242 +96,100 @@ nbd_internal_py_display_version (PyObject *self, PyObject *args) return Py_None; } -static const char aio_buffer_name[] = "nbd.Buffer"; - -/* Return internal buffer pointer of nbd.Buffer */ -static struct py_aio_buffer * -nbd_internal_py_get_aio_buffer (PyObject *object) +/* Return new reference to MemoryView wrapping aio_buffer contents */ +PyObject * +nbd_internal_py_get_aio_view (PyObject *object, bool require_init) { + PyObject *buffer = NULL; + if (PyObject_IsInstance (object, nbd_internal_py_get_nbd_buffer_type ())) { - PyObject *capsule = PyObject_GetAttrString(object, "_o"); + buffer = PyObject_GetAttrString (object, "_o"); - return PyCapsule_GetPointer (capsule, aio_buffer_name); + if (require_init && ! PyObject_HasAttrString (object, "_init")) { + assert (PyByteArray_Check (buffer)); + memset (PyByteArray_AS_STRING (buffer), 0, + PyByteArray_GET_SIZE (buffer)); + if (PyObject_SetAttrString (object, "_init", Py_True) < 0) + return NULL; + } } + if (buffer) + return PyMemoryView_FromObject (buffer); + PyErr_SetString (PyExc_TypeError, "aio_buffer: expecting nbd.Buffer instance"); return NULL; } -/* Return new reference to MemoryView wrapping aio_buffer contents */ -PyObject * -nbd_internal_py_get_aio_view (PyObject *object, bool require_init) -{ - struct py_aio_buffer *buf = nbd_internal_py_get_aio_buffer (object); - - if (!buf) - return NULL; - - if (require_init && !buf->initialized) { - memset (buf->data, 0, buf->len); - buf->initialized = true; - } - - return PyMemoryView_FromMemory (buf->data, buf->len, - require_init ? PyBUF_READ : PyBUF_WRITE); -} - int nbd_internal_py_init_aio_buffer (PyObject *object) { - struct py_aio_buffer *buf = nbd_internal_py_get_aio_buffer (object); - - assert (buf); - buf->initialized = true; + if (PyObject_IsInstance (object, nbd_internal_py_get_nbd_buffer_type ())) + return PyObject_SetAttrString (object, "_init", Py_True); return 0; } -static void -free_aio_buffer (PyObject *capsule) -{ - struct py_aio_buffer *buf = PyCapsule_GetPointer (capsule, aio_buffer_name); - - if (buf) - free (buf->data); - free (buf); -} - -/* Allocate a persistent buffer used for nbd_aio_pread. */ +/* Allocate an uninitialized persistent buffer used for nbd_aio_pread. */ PyObject * nbd_internal_py_alloc_aio_buffer (PyObject *self, PyObject *args) { - struct py_aio_buffer *buf; - PyObject *ret; + Py_ssize_t len; - buf = malloc (sizeof *buf); - if (buf == NULL) { - PyErr_NoMemory (); - return NULL; - } - - buf->initialized = false; if (!PyArg_ParseTuple (args, "n:nbd_internal_py_alloc_aio_buffer", - &buf->len)) { - free (buf); + &len)) return NULL; - } - if (buf->len < 0) { - PyErr_SetString (PyExc_RuntimeError, "length < 0"); - free (buf); - return NULL; - } - buf->data = malloc (buf->len); - if (buf->data == NULL) { - PyErr_NoMemory (); - free (buf); - return NULL; - } - - ret = PyCapsule_New (buf, aio_buffer_name, free_aio_buffer); - if (ret == NULL) { - free (buf->data); - free (buf); - return NULL; - } - - return ret; -} - -PyObject * -nbd_internal_py_aio_buffer_from_bytearray (PyObject *self, PyObject *args) -{ - PyObject *obj; - PyObject *arr = NULL; - Py_ssize_t len; - void *data; - struct py_aio_buffer *buf; - PyObject *ret; - - if (!PyArg_ParseTuple (args, - "O:nbd_internal_py_aio_buffer_from_bytearray", - &obj)) - return NULL; - - if (! PyByteArray_Check (obj)) { - arr = PyByteArray_FromObject (obj); - if (arr == NULL) - return NULL; - obj = arr; - } - data = PyByteArray_AsString (obj); - if (!data) { - PyErr_SetString (PyExc_RuntimeError, - "parameter is not a bytearray or buffer"); - Py_XDECREF (arr); - return NULL; - } - len = PyByteArray_Size (obj); - - buf = malloc (sizeof *buf); - if (buf == NULL) { - PyErr_NoMemory (); - Py_XDECREF (arr); - return NULL; - } - - buf->len = len; - buf->data = malloc (len); - if (buf->data == NULL) { - PyErr_NoMemory (); - free (buf); - Py_XDECREF (arr); - return NULL; - } - memcpy (buf->data, data, len); - Py_XDECREF (arr); - buf->initialized = true; - - ret = PyCapsule_New (buf, aio_buffer_name, free_aio_buffer); - if (ret == NULL) { - free (buf->data); - free (buf); - return NULL; - } - - return ret; -} - -PyObject * -nbd_internal_py_aio_buffer_to_bytearray (PyObject *self, PyObject *args) -{ - PyObject *obj; - PyObject *view; - PyObject *ret; - - if (!PyArg_ParseTuple (args, - "O:nbd_internal_py_aio_buffer_to_bytearray", - &obj)) - return NULL; - - view = nbd_internal_py_get_aio_view (obj, true); - if (view == NULL) - return NULL; - - ret = PyByteArray_FromObject (view); - Py_DECREF (view); - return ret; -} - -PyObject * -nbd_internal_py_aio_buffer_size (PyObject *self, PyObject *args) -{ - PyObject *obj; - struct py_aio_buffer *buf; - - if (!PyArg_ParseTuple (args, - "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); + /* Constructing bytearray(len) in python zeroes the memory; doing it this + * way gives uninitialized memory. This correctly flags negative len. + */ + return PyByteArray_FromStringAndSize (NULL, len); } PyObject * nbd_internal_py_aio_buffer_is_zero (PyObject *self, PyObject *args) { - PyObject *obj; - struct py_aio_buffer *buf; + Py_buffer buf; Py_ssize_t offset, size; + int init; + PyObject *ret = NULL; if (!PyArg_ParseTuple (args, - "Onn:nbd_internal_py_aio_buffer_is_zero", - &obj, &offset, &size)) + "y*nnp:nbd_internal_py_aio_buffer_is_zero", + &buf, &offset, &size, &init)) return NULL; - if (size == 0) - Py_RETURN_TRUE; - - buf = nbd_internal_py_get_aio_buffer (obj); - if (buf == NULL) - return NULL; + if (size == 0) { + ret = Py_True; + goto out; + } /* Check the bounds of the offset. */ - if (offset < 0 || offset > buf->len) { + if (offset < 0 || offset > buf.len) { PyErr_SetString (PyExc_IndexError, "offset out of range"); - return NULL; + goto out; } /* Compute or check the length. */ if (size == -1) - size = buf->len - offset; + size = buf.len - offset; else if (size < 0) { PyErr_SetString (PyExc_IndexError, "size cannot be negative, " "except -1 to mean to the end of the buffer"); - return NULL; + goto out; } - else if ((size_t) offset + size > buf->len) { + else if ((size_t) offset + size > buf.len) { PyErr_SetString (PyExc_IndexError, "size out of range"); - return NULL; + goto out; } - if (!buf->initialized || is_zero (buf->data + offset, size)) - Py_RETURN_TRUE; + if (!init || is_zero (buf.buf + offset, size)) + ret = Py_True; else - Py_RETURN_FALSE; + ret = Py_False; + out: + PyBuffer_Release (&buf); + Py_XINCREF (ret); + return ret; } -- 2.36.1
On Mon, Jun 06, 2022 at 09:08:33PM -0500, Eric Blake wrote:
Instead of storing a PyCapsule in _o and repeatedly doing lookups to dereference a stored malloc'd pointer, it is easier to just directly store a Python buffer-like object as _o. Track initialization via ._init: absent for nbd.Buffer(int), present for nbd.Buffer.from_bytearray or after we have forced initialization due to aio_pread or .to_bytearray. At this point, we are still copying in and out of .{from,to}_bytearray, but we are getting closer to reducing the number of copies. The sheer size in reduced lines of code is a testament to the ease of doing things closer to Python, rather than hidden behind unpacking a PyCapsule layer.
Acceptable side effect: nbd.Buffer(-1) changes from: RuntimeError: length < 0 to SystemError: Negative size passed to PyByteArray_FromStringAndSize
The generated code changes as follows:
| --- python/methods.h.bak 2022-06-06 20:36:01.973327739 -0500 | +++ python/methods.h 2022-06-06 20:36:16.146352508 -0500 | @@ -62,9 +62,6 @@ | extern PyObject *nbd_internal_py_close (PyObject *self, PyObject *args); | extern PyObject *nbd_internal_py_display_version (PyObject *self, PyObject *args); | extern PyObject *nbd_internal_py_alloc_aio_buffer (PyObject *self, PyObject *args); | -extern PyObject *nbd_internal_py_aio_buffer_from_bytearray (PyObject *self, PyObject *args); | -extern PyObject *nbd_internal_py_aio_buffer_to_bytearray (PyObject *self, PyObject *args); | -extern PyObject *nbd_internal_py_aio_buffer_size (PyObject *self, PyObject *args); | extern PyObject *nbd_internal_py_aio_buffer_is_zero (PyObject *self, PyObject *args); | extern PyObject *nbd_internal_py_set_debug (PyObject *self, PyObject *args); | extern PyObject *nbd_internal_py_get_debug (PyObject *self, PyObject *args); | --- python/nbd.py.bak 2022-06-06 20:36:01.978327748 -0500 | +++ python/nbd.py 2022-06-06 20:56:41.866729560 -0500 | @@ -134,18 +134,21 @@ class Buffer(object): | bytearray constructor. Otherwise, ba is copied. Either way, the | resulting AIO buffer is independent from the original. | ''' | - o = libnbdmod.aio_buffer_from_bytearray(ba) | self = cls(0) | - self._o = o | + self._o = bytearray(ba) | + self._init = True | return self | | def to_bytearray(self): | '''Copy an AIO buffer into a bytearray.''' | - return libnbdmod.aio_buffer_to_bytearray(self) | + if not hasattr(self, '_init'): | + self._o = bytearray(len(self._o)) | + self._init = True | + return bytearray(self._o) | | def size(self): | '''Return the size of an AIO buffer.''' | - return libnbdmod.aio_buffer_size(self) | + return len(self._o) | | def is_zero(self, offset=0, size=-1): | '''Returns true if and only if all bytes in the buffer are zeroes. | @@ -161,7 +164,8 @@ class Buffer(object): | always returns true. If size > 0, we check the interval | [offset..offset+size-1]. | ''' | - return libnbdmod.aio_buffer_is_zero(self, offset, size) | + return libnbdmod.aio_buffer_is_zero(self._o, offset, size, | + hasattr(self, '_init')) | | | class NBD(object): --- generator/Python.ml | 20 ++-- python/handle.c | 242 +++++++++----------------------------------- 2 files changed, 56 insertions(+), 206 deletions(-)
diff --git a/generator/Python.ml b/generator/Python.ml index c49af4f..86781ac 100644 --- a/generator/Python.ml +++ b/generator/Python.ml @@ -73,9 +73,6 @@ let ) ([ "create"; "close"; "display_version"; "alloc_aio_buffer"; - "aio_buffer_from_bytearray"; - "aio_buffer_to_bytearray"; - "aio_buffer_size"; "aio_buffer_is_zero"] @ List.map fst handle_calls);
pr "\n"; @@ -105,9 +102,6 @@ let ) ([ "create"; "close"; "display_version"; "alloc_aio_buffer"; - "aio_buffer_from_bytearray"; - "aio_buffer_to_bytearray"; - "aio_buffer_size"; "aio_buffer_is_zero"] @ List.map fst handle_calls); pr " { NULL, NULL, 0, NULL }\n"; pr "};\n"; @@ -748,18 +742,21 @@ let bytearray constructor. Otherwise, ba is copied. Either way, the resulting AIO buffer is independent from the original. ''' - o = libnbdmod.aio_buffer_from_bytearray(ba) self = cls(0) - self._o = o + self._o = bytearray(ba) + self._init = True return self
def to_bytearray(self): '''Copy an AIO buffer into a bytearray.''' - return libnbdmod.aio_buffer_to_bytearray(self) + if not hasattr(self, '_init'): + self._o = bytearray(len(self._o)) + self._init = True + return bytearray(self._o)
def size(self): '''Return the size of an AIO buffer.''' - return libnbdmod.aio_buffer_size(self) + return len(self._o)
def is_zero(self, offset=0, size=-1): '''Returns true if and only if all bytes in the buffer are zeroes. @@ -775,7 +772,8 @@ let always returns true. If size > 0, we check the interval [offset..offset+size-1]. ''' - return libnbdmod.aio_buffer_is_zero(self, offset, size) + return libnbdmod.aio_buffer_is_zero(self._o, offset, size, + hasattr(self, '_init'))
class NBD(object): diff --git a/python/handle.c b/python/handle.c index 79b369d..ca6c8e9 100644 --- a/python/handle.c +++ b/python/handle.c @@ -40,12 +40,6 @@
#include "methods.h"
-struct py_aio_buffer { - Py_ssize_t len; - void *data; - bool initialized; -}; - static inline PyObject * put_handle (struct nbd_handle *h) { @@ -102,242 +96,100 @@ nbd_internal_py_display_version (PyObject *self, PyObject *args) return Py_None; }
-static const char aio_buffer_name[] = "nbd.Buffer"; - -/* Return internal buffer pointer of nbd.Buffer */ -static struct py_aio_buffer * -nbd_internal_py_get_aio_buffer (PyObject *object) +/* Return new reference to MemoryView wrapping aio_buffer contents */ +PyObject * +nbd_internal_py_get_aio_view (PyObject *object, bool require_init) { + PyObject *buffer = NULL; + if (PyObject_IsInstance (object, nbd_internal_py_get_nbd_buffer_type ())) { - PyObject *capsule = PyObject_GetAttrString(object, "_o"); + buffer = PyObject_GetAttrString (object, "_o");
- return PyCapsule_GetPointer (capsule, aio_buffer_name); + if (require_init && ! PyObject_HasAttrString (object, "_init")) { + assert (PyByteArray_Check (buffer)); + memset (PyByteArray_AS_STRING (buffer), 0, + PyByteArray_GET_SIZE (buffer)); + if (PyObject_SetAttrString (object, "_init", Py_True) < 0) + return NULL; + } }
+ if (buffer) + return PyMemoryView_FromObject (buffer); + PyErr_SetString (PyExc_TypeError, "aio_buffer: expecting nbd.Buffer instance"); return NULL; }
-/* Return new reference to MemoryView wrapping aio_buffer contents */ -PyObject * -nbd_internal_py_get_aio_view (PyObject *object, bool require_init) -{ - struct py_aio_buffer *buf = nbd_internal_py_get_aio_buffer (object); - - if (!buf) - return NULL; - - if (require_init && !buf->initialized) { - memset (buf->data, 0, buf->len); - buf->initialized = true; - } - - return PyMemoryView_FromMemory (buf->data, buf->len, - require_init ? PyBUF_READ : PyBUF_WRITE); -} - int nbd_internal_py_init_aio_buffer (PyObject *object) { - struct py_aio_buffer *buf = nbd_internal_py_get_aio_buffer (object); - - assert (buf); - buf->initialized = true; + if (PyObject_IsInstance (object, nbd_internal_py_get_nbd_buffer_type ())) + return PyObject_SetAttrString (object, "_init", Py_True); return 0; }
-static void -free_aio_buffer (PyObject *capsule) -{ - struct py_aio_buffer *buf = PyCapsule_GetPointer (capsule, aio_buffer_name); - - if (buf) - free (buf->data); - free (buf); -} - -/* Allocate a persistent buffer used for nbd_aio_pread. */ +/* Allocate an uninitialized persistent buffer used for nbd_aio_pread. */ PyObject * nbd_internal_py_alloc_aio_buffer (PyObject *self, PyObject *args) { - struct py_aio_buffer *buf; - PyObject *ret; + Py_ssize_t len;
- buf = malloc (sizeof *buf); - if (buf == NULL) { - PyErr_NoMemory (); - return NULL; - } - - buf->initialized = false; if (!PyArg_ParseTuple (args, "n:nbd_internal_py_alloc_aio_buffer", - &buf->len)) { - free (buf); + &len)) return NULL; - }
- if (buf->len < 0) { - PyErr_SetString (PyExc_RuntimeError, "length < 0"); - free (buf); - return NULL; - } - buf->data = malloc (buf->len); - if (buf->data == NULL) { - PyErr_NoMemory (); - free (buf); - return NULL; - } - - ret = PyCapsule_New (buf, aio_buffer_name, free_aio_buffer); - if (ret == NULL) { - free (buf->data); - free (buf); - return NULL; - } - - return ret; -} - -PyObject * -nbd_internal_py_aio_buffer_from_bytearray (PyObject *self, PyObject *args) -{ - PyObject *obj; - PyObject *arr = NULL; - Py_ssize_t len; - void *data; - struct py_aio_buffer *buf; - PyObject *ret; - - if (!PyArg_ParseTuple (args, - "O:nbd_internal_py_aio_buffer_from_bytearray", - &obj)) - return NULL; - - if (! PyByteArray_Check (obj)) { - arr = PyByteArray_FromObject (obj); - if (arr == NULL) - return NULL; - obj = arr; - } - data = PyByteArray_AsString (obj); - if (!data) { - PyErr_SetString (PyExc_RuntimeError, - "parameter is not a bytearray or buffer"); - Py_XDECREF (arr); - return NULL; - } - len = PyByteArray_Size (obj); - - buf = malloc (sizeof *buf); - if (buf == NULL) { - PyErr_NoMemory (); - Py_XDECREF (arr); - return NULL; - } - - buf->len = len; - buf->data = malloc (len); - if (buf->data == NULL) { - PyErr_NoMemory (); - free (buf); - Py_XDECREF (arr); - return NULL; - } - memcpy (buf->data, data, len); - Py_XDECREF (arr); - buf->initialized = true; - - ret = PyCapsule_New (buf, aio_buffer_name, free_aio_buffer); - if (ret == NULL) { - free (buf->data); - free (buf); - return NULL; - } - - return ret; -} - -PyObject * -nbd_internal_py_aio_buffer_to_bytearray (PyObject *self, PyObject *args) -{ - PyObject *obj; - PyObject *view; - PyObject *ret; - - if (!PyArg_ParseTuple (args, - "O:nbd_internal_py_aio_buffer_to_bytearray", - &obj)) - return NULL; - - view = nbd_internal_py_get_aio_view (obj, true); - if (view == NULL) - return NULL; - - ret = PyByteArray_FromObject (view); - Py_DECREF (view); - return ret; -} - -PyObject * -nbd_internal_py_aio_buffer_size (PyObject *self, PyObject *args) -{ - PyObject *obj; - struct py_aio_buffer *buf; - - if (!PyArg_ParseTuple (args, - "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); + /* Constructing bytearray(len) in python zeroes the memory; doing it this + * way gives uninitialized memory. This correctly flags negative len. + */ + return PyByteArray_FromStringAndSize (NULL, len); }
PyObject * nbd_internal_py_aio_buffer_is_zero (PyObject *self, PyObject *args) { - PyObject *obj; - struct py_aio_buffer *buf; + Py_buffer buf; Py_ssize_t offset, size; + int init; + PyObject *ret = NULL;
if (!PyArg_ParseTuple (args, - "Onn:nbd_internal_py_aio_buffer_is_zero", - &obj, &offset, &size)) + "y*nnp:nbd_internal_py_aio_buffer_is_zero", + &buf, &offset, &size, &init)) return NULL;
- if (size == 0) - Py_RETURN_TRUE; - - buf = nbd_internal_py_get_aio_buffer (obj); - if (buf == NULL) - return NULL; + if (size == 0) { + ret = Py_True; + goto out; + }
/* Check the bounds of the offset. */ - if (offset < 0 || offset > buf->len) { + if (offset < 0 || offset > buf.len) { PyErr_SetString (PyExc_IndexError, "offset out of range"); - return NULL; + goto out; }
/* Compute or check the length. */ if (size == -1) - size = buf->len - offset; + size = buf.len - offset; else if (size < 0) { PyErr_SetString (PyExc_IndexError, "size cannot be negative, " "except -1 to mean to the end of the buffer"); - return NULL; + goto out; } - else if ((size_t) offset + size > buf->len) { + else if ((size_t) offset + size > buf.len) { PyErr_SetString (PyExc_IndexError, "size out of range"); - return NULL; + goto out; }
- if (!buf->initialized || is_zero (buf->data + offset, size)) - Py_RETURN_TRUE; + if (!init || is_zero (buf.buf + offset, size)) + ret = Py_True; else - Py_RETURN_FALSE; + ret = Py_False; + out: + PyBuffer_Release (&buf); + Py_XINCREF (ret); + return ret; }
Looks like a nice simplification! Reviewed-by: Richard W.M. Jones <rjones@redhat.com> 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 Tue, Jun 07, 2022 at 02:20:35PM +0100, Richard W.M. Jones wrote:
On Mon, Jun 06, 2022 at 09:08:33PM -0500, Eric Blake wrote:
Instead of storing a PyCapsule in _o and repeatedly doing lookups to dereference a stored malloc'd pointer, it is easier to just directly store a Python buffer-like object as _o. Track initialization via ._init: absent for nbd.Buffer(int), present for nbd.Buffer.from_bytearray or after we have forced initialization due to aio_pread or .to_bytearray. At this point, we are still copying in and out of .{from,to}_bytearray, but we are getting closer to reducing the number of copies. The sheer size in reduced lines of code is a testament to the ease of doing things closer to Python, rather than hidden behind unpacking a PyCapsule layer.
Looks like a nice simplification!
Reviewed-by: Richard W.M. Jones <rjones@redhat.com>
Series now in as 08a3804..4f15d0e. Next up - fixing [aio_]pread_structured to expose slices of the underlying Python object, and teaching nbd.Buffer to reuse (rather than copy) buffers. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
participants (2)
-
Eric Blake -
Richard W.M. Jones