Eric Blake
2022-Jun-03 22:26 UTC
[Libguestfs] [libnbd PATCH 0/5] python: Optimize aio_p{read, write}
I didn't get to integrate this with my h.{aio_}pread_structured copy
reductions, but I'm liking how this one turned out.
Eric Blake (5):
python: Avoid memleak on (unlikely) module failure
python: Don't unwrap nbd.Buffer in nbd.py
python: Simplify python generator
python: Make nbd.Buffer lighter-weight
python: Accept all buffer-like objects in aio_p{read,write}
generator/Python.ml | 113 +++++++++++++-------------
python/handle.c | 192 +++++++++-----------------------------------
python/utils.c | 20 ++++-
3 files changed, 110 insertions(+), 215 deletions(-)
--
2.36.1
Eric Blake
2022-Jun-03 22:26 UTC
[Libguestfs] [libnbd PATCH 1/5] python: Avoid memleak on (unlikely) module failure
Python 3.10 added PyModule_AddObjectRef() to more easily avoid a
common memory leak when ading to a module fails (unlikely in our case,
since we initialize early in the python process, but still something
we must worry about for corner-case correctness). But since we target
older Python, we must check for errors and clean up ourselves.
Fixes: 259d46cb ("python: Raise a custome exception containing error string
and errno.", v0.1.6)
---
generator/Python.ml | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/generator/Python.ml b/generator/Python.ml
index 1c4446e..3f672ba 100644
--- a/generator/Python.ml
+++ b/generator/Python.ml
@@ -143,9 +143,11 @@ let
return NULL;
nbd_internal_py_Error = PyErr_NewException (\"nbd.Error\", NULL,
NULL);
- if (nbd_internal_py_Error == NULL)
+ if (PyModule_AddObject (mod, \"Error\", nbd_internal_py_Error) <
0) {
+ Py_XDECREF (nbd_internal_py_Error);
+ Py_DECREF (mod);
return NULL;
- PyModule_AddObject (mod, \"Error\", nbd_internal_py_Error);
+ }
return mod;
}
--
2.36.1
Eric Blake
2022-Jun-03 22:26 UTC
[Libguestfs] [libnbd PATCH 2/5] python: Don't unwrap nbd.Buffer in nbd.py
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-03 12:16:58.200633552 -0500
| +++ python/methods.h 2022-06-03 12:17:16.605661419 -0500
| @@ -38,6 +38,7 @@
| 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_nbd_buffer_type (void);
|
| static inline struct nbd_handle *
| get_handle (PyObject *obj)
| --- python/methods.c.bak 2022-06-02 14:54:37.016940545 -0500
| +++ python/methods.c 2022-06-03 12:17:16.615661434 -0500
| @@ -3162,8 +3162,8 @@ 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 */
| - struct py_aio_buffer *buf_buf;
| + PyObject *buf; /* instance of nbd.Buffer */
| + struct py_aio_buffer *buf_buf; /* Contents of nbd.Buffer */
| uint64_t offset_u64;
| unsigned long long offset; /* really uint64_t */
| struct user_data *completion_user_data = NULL;
| @@ -3221,8 +3221,8 @@ 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 */
| - struct py_aio_buffer *buf_buf;
| + PyObject *buf; /* instance of nbd.Buffer */
| + struct py_aio_buffer *buf_buf; /* Contents of nbd.Buffer */
| uint64_t offset_u64;
| unsigned long long offset; /* really uint64_t */
| struct user_data *chunk_user_data = NULL;
| @@ -3296,8 +3296,8 @@ 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 */
| - struct py_aio_buffer *buf_buf;
| + PyObject *buf; /* instance of nbd.Buffer */
| + struct py_aio_buffer *buf_buf; /* Contents of nbd.Buffer */
| uint64_t offset_u64;
| unsigned long long offset; /* really uint64_t */
| struct user_data *completion_user_data = NULL;
| --- python/nbd.py.bak 2022-06-03 12:16:28.320588309 -0500
| +++ python/nbd.py 2022-06-03 12:38:01.974645685 -0500
| @@ -136,11 +136,11 @@ 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'''
| - return libnbdmod.aio_buffer_size(self._o)
| + return libnbdmod.aio_buffer_size(self)
|
| def is_zero(self, offset=0, size=-1):
| '''
| @@ -154,7 +154,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):
| @@ -2201,8 +2201,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):
| @@ -2236,8 +2235,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):
| '''? write to the NBD server
| @@ -2260,8 +2259,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):
| '''? disconnect from the NBD server
---
generator/Python.ml | 20 ++++++++++----------
python/handle.c | 11 +++++++++--
python/utils.c | 20 +++++++++++++++++++-
3 files changed, 38 insertions(+), 13 deletions(-)
diff --git a/generator/Python.ml b/generator/Python.ml
index 3f672ba..fcab6bd 100644
--- a/generator/Python.ml
+++ b/generator/Python.ml
@@ -44,6 +44,7 @@ let
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_nbd_buffer_type (void);
static inline struct nbd_handle *
get_handle (PyObject *obj)
@@ -299,9 +300,8 @@ 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 " struct py_aio_buffer *%s_buf;\n" n
+ pr " PyObject *%s; /* instance of nbd.Buffer */\n" n;
+ pr " struct py_aio_buffer *%s_buf; /* Contents of nbd.Buffer
*/\n" n
| Closure { cbname } ->
pr " struct user_data *%s_user_data = NULL;\n" cbname;
pr " PyObject *py_%s_fn;\n" cbname;
@@ -357,9 +357,9 @@ let
function
| Bool n -> pr " \"p\""
| BytesIn (n, _) -> pr " \"y*\""
- | BytesPersistIn (n, _) -> pr " \"O\""
+ | BytesPersistIn _ -> pr " \"O\""
| BytesOut (_, count) -> pr " \"n\""
- | BytesPersistOut (_, count) -> pr " \"O\""
+ | BytesPersistOut _ -> pr " \"O\""
| Closure _ -> pr " \"O\""
| Enum _ -> pr " \"i\""
| Flags _ -> pr " \"I\""
@@ -776,11 +776,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):
'''
@@ -794,7 +794,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):
@@ -819,8 +819,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 9fe3f8e..f84c6e0 100644
--- a/python/handle.c
+++ b/python/handle.c
@@ -99,9 +99,16 @@ nbd_internal_py_display_version (PyObject *self, PyObject
*args)
static const char aio_buffer_name[] = "nbd.Buffer";
struct py_aio_buffer *
-nbd_internal_py_get_aio_buffer (PyObject *capsule)
+nbd_internal_py_get_aio_buffer (PyObject *buffer)
{
- return PyCapsule_GetPointer (capsule, aio_buffer_name);
+ if (PyObject_IsInstance (buffer, nbd_internal_py_get_nbd_buffer_type ())) {
+ PyObject *capsule = PyObject_GetAttrString(buffer, "_o");
+ return PyCapsule_GetPointer (capsule, aio_buffer_name);
+ }
+
+ PyErr_SetString (PyExc_TypeError,
+ "aio_buffer: expecting nbd.Buffer instance");
+ return NULL;
}
static void
diff --git a/python/utils.c b/python/utils.c
index 37f0c55..bc7d6a1 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");
+ Py_DECREF (modname);
+ Py_DECREF (module);
+ }
+ assert (type);
+ return type;
+}
--
2.36.1
Eric Blake
2022-Jun-03 22:26 UTC
[Libguestfs] [libnbd PATCH 3/5] python: Simplify python generator
Now that none of our parameter types uses a getter sequence, we can
simplify the code for generating nbd.py.
---
generator/Python.ml | 49 ++++++++++++++++++++++-----------------------
1 file changed, 24 insertions(+), 25 deletions(-)
diff --git a/generator/Python.ml b/generator/Python.ml
index fcab6bd..b862b44 100644
--- a/generator/Python.ml
+++ b/generator/Python.ml
@@ -816,31 +816,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;
@@ -848,8 +848,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
@@ -861,8 +861,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
Eric Blake
2022-Jun-03 22:26 UTC
[Libguestfs] [libnbd PATCH 4/5] python: Make nbd.Buffer lighter-weight
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. Then, instead of using
Py_{INC,DEC}REF across the paired aio_p{read,write} and corresponding
completion function, we now rely on PyObject_GetBuffer and
ByBuffer_Release. The use of memoryview protects us from the python
user changing the buffer out from under our feet:
$ ./run nbdsh
nbd>
h.connect_command(['nbdkit','-s','memory','10'])
nbd> b = bytearray(10)
nbd> b1 = nbd.Buffer.from_bytearray(b)
nbd> h.aio_pread(b1, 0)
1
nbd> b.pop()
Traceback (most recent call last):
File "/usr/lib64/python3.10/code.py", line 90, in runcode
exec(code, self.locals)
File "<console>", line 1, in <module>
BufferError: Existing exports of data: object cannot be re-sized
nbd> h.poll(-1)
1
nbd> h.aio_command_completed(1)
True
nbd> b1 = None
nbd> b.pop()
0
This ALSO means that we're doing less copying! now that
nbd.Buffer.from_bytearray() is reusing an existing python object
instead of copying to a C malloc'd buffer, we have noticeable effects
on performance. For example, on my machine:
$ export script='
m=1024*1024
size=h.get_size()
zero=bytearray(m)
for i in range(size // m):
c = h.aio_pwrite(nbd.Buffer.from_bytearray(zero), m*i)
while not h.aio_command_completed(c):
h.poll(-1)
'
$ time nbdkit -U - null 10G --run 'nbdsh -u $uri -c "$script"'
takes 2.9s pre-patch, and 2.1s post-patch.
I noticed that nbd.Buffer(-1) changes from:
RuntimeError: length < 0
to
SystemError: Negative size passed to PyByteArray_FromStringAndSize
It would not be hard to revert that part, if needed.
Pure Python doesn't have any quick isinstance() test for whether an
object is buffer-like (only C has that, in PyObject_CheckBuffer) [1].
That makes it interesting to preserve our pre-existing semantics (from
the recent commit d477f7c7) where nbd.Buffer(int) gives uninitialized
memory, but nbd.Buffer.from_bytearray(int) relies on the
bytearray(int) constructor to give us initialized memory. For the
constructor, I had to use C, but for .from_bytearray (which is now
slightly misnamed, oh well), I chose to stick to a python try/except
instead of deferring to a C helper function.
[1]
https://stackoverflow.com/questions/30017991/check-if-an-object-supports-the-buffer-protocol-python
The generated code changes as follows:
| --- python/methods.h.bak 2022-06-03 17:10:24.533842689 -0500
| +++ python/methods.h 2022-06-03 17:10:33.833858093 -0500
| @@ -28,16 +28,11 @@
|
| #include <assert.h>
|
| -struct py_aio_buffer {
| - Py_ssize_t len;
| - void *data;
| -};
| -
| 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_buffer (PyObject *);
| extern PyObject *nbd_internal_py_get_nbd_buffer_type (void);
|
| static inline struct nbd_handle *
| @@ -66,9 +61,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/methods.c.bak 2022-06-03 17:10:25.741844689 -0500
| +++ python/methods.c 2022-06-03 17:13:09.689116273 -0500
| @@ -37,7 +37,7 @@
| */
| struct user_data {
| PyObject *fn; /* Optional pointer to Python function. */
| - PyObject *buf; /* Optional pointer to persistent buffer. */
| + Py_buffer view; /* persistent buffer, if view->obj set. */
| };
|
| static struct user_data *
| @@ -58,7 +58,7 @@ free_user_data (void *user_data)
|
| if (data) {
| Py_XDECREF (data->fn);
| - Py_XDECREF (data->buf);
| + PyBuffer_Release(&data->view);
| free (data);
| }
| }
| @@ -3163,7 +3163,7 @@ nbd_internal_py_aio_pread (PyObject *sel
| int64_t ret;
| PyObject *py_ret = NULL;
| PyObject *buf; /* instance of nbd.Buffer */
| - struct py_aio_buffer *buf_buf; /* Contents of nbd.Buffer */
| + PyObject *buf_buf; /* Contents of nbd.Buffer */
| uint64_t offset_u64;
| unsigned long long offset; /* really uint64_t */
| struct user_data *completion_user_data = NULL;
| @@ -3196,12 +3196,11 @@ nbd_internal_py_aio_pread (PyObject *sel
| flags_u32 = flags;
| buf_buf = nbd_internal_py_get_aio_buffer (buf);
| if (!buf_buf) goto out;
| - /* Increment refcount since buffer may be saved by libnbd. */
| - Py_INCREF (buf);
| - completion_user_data->buf = buf;
| + if (PyObject_GetBuffer(buf_buf, &completion_user_data->view,
| + PyBUF_CONTIG) < 0) goto out;
| offset_u64 = offset;
|
| - ret = nbd_aio_pread (h, buf_buf->data, buf_buf->len, offset_u64,
completion, flags_u32);
| + ret = nbd_aio_pread (h, completion_user_data->view.buf,
completion_user_data->view.len, offset_u64, completion, flags_u32);
| completion_user_data = NULL;
| if (ret == -1) {
| raise_exception ();
| @@ -3210,6 +3209,7 @@ nbd_internal_py_aio_pread (PyObject *sel
| py_ret = PyLong_FromLongLong (ret);
|
| out:
| + Py_XDECREF (buf_buf);
| free_user_data (completion_user_data);
| return py_ret;
| }
| @@ -3222,7 +3222,7 @@ nbd_internal_py_aio_pread_structured (Py
| int64_t ret;
| PyObject *py_ret = NULL;
| PyObject *buf; /* instance of nbd.Buffer */
| - struct py_aio_buffer *buf_buf; /* Contents of nbd.Buffer */
| + PyObject *buf_buf; /* Contents of nbd.Buffer */
| uint64_t offset_u64;
| unsigned long long offset; /* really uint64_t */
| struct user_data *chunk_user_data = NULL;
| @@ -3259,9 +3259,8 @@ nbd_internal_py_aio_pread_structured (Py
| flags_u32 = flags;
| buf_buf = nbd_internal_py_get_aio_buffer (buf);
| if (!buf_buf) goto out;
| - /* Increment refcount since buffer may be saved by libnbd. */
| - Py_INCREF (buf);
| - completion_user_data->buf = buf;
| + if (PyObject_GetBuffer(buf_buf, &completion_user_data->view,
| + PyBUF_CONTIG) < 0) goto out;
| offset_u64 = offset;
| chunk.user_data = chunk_user_data = alloc_user_data ();
| if (chunk_user_data == NULL) goto out;
| @@ -3274,7 +3273,7 @@ nbd_internal_py_aio_pread_structured (Py
| Py_INCREF (py_chunk_fn);
| chunk_user_data->fn = py_chunk_fn;
|
| - ret = nbd_aio_pread_structured (h, buf_buf->data, buf_buf->len,
offset_u64, chunk, completion, flags_u32);
| + ret = nbd_aio_pread_structured (h, completion_user_data->view.buf,
completion_user_data->view.len, offset_u64, chunk, completion, flags_u32);
| chunk_user_data = NULL;
| completion_user_data = NULL;
| if (ret == -1) {
| @@ -3284,6 +3283,7 @@ nbd_internal_py_aio_pread_structured (Py
| py_ret = PyLong_FromLongLong (ret);
|
| out:
| + Py_XDECREF (buf_buf);
| free_user_data (chunk_user_data);
| free_user_data (completion_user_data);
| return py_ret;
| @@ -3297,7 +3297,7 @@ nbd_internal_py_aio_pwrite (PyObject *se
| int64_t ret;
| PyObject *py_ret = NULL;
| PyObject *buf; /* instance of nbd.Buffer */
| - struct py_aio_buffer *buf_buf; /* Contents of nbd.Buffer */
| + PyObject *buf_buf; /* Contents of nbd.Buffer */
| uint64_t offset_u64;
| unsigned long long offset; /* really uint64_t */
| struct user_data *completion_user_data = NULL;
| @@ -3330,12 +3330,11 @@ nbd_internal_py_aio_pwrite (PyObject *se
| flags_u32 = flags;
| buf_buf = nbd_internal_py_get_aio_buffer (buf);
| if (!buf_buf) goto out;
| - /* Increment refcount since buffer may be saved by libnbd. */
| - Py_INCREF (buf);
| - completion_user_data->buf = buf;
| + if (PyObject_GetBuffer(buf_buf, &completion_user_data->view,
| + PyBUF_CONTIG_RO) < 0) goto out;
| offset_u64 = offset;
|
| - ret = nbd_aio_pwrite (h, buf_buf->data, buf_buf->len, offset_u64,
completion, flags_u32);
| + ret = nbd_aio_pwrite (h, completion_user_data->view.buf,
completion_user_data->view.len, offset_u64, completion, flags_u32);
| completion_user_data = NULL;
| if (ret == -1) {
| raise_exception ();
| @@ -3344,6 +3343,7 @@ nbd_internal_py_aio_pwrite (PyObject *se
| py_ret = PyLong_FromLongLong (ret);
|
| out:
| + Py_XDECREF (buf_buf);
| free_user_data (completion_user_data);
| return py_ret;
| }
| --- python/nbd.py.bak 2022-06-03 17:10:23.350840729 -0500
| +++ python/nbd.py 2022-06-03 17:10:33.852858124 -0500
| @@ -128,19 +128,21 @@ class Buffer(object):
|
| @classmethod
| def from_bytearray(cls, ba):
| - '''create an AIO buffer from a bytearray'''
| - o = libnbdmod.aio_buffer_from_bytearray(ba)
| + '''create an AIO buffer from a bytearray or other
buffer'''
| self = cls(0)
| - self._o = o
| + try:
| + self._o = memoryview(ba).cast('B')
| + except TypeError:
| + self._o = bytearray(ba)
| return self
|
| def to_bytearray(self):
| '''copy an AIO buffer into a bytearray'''
| - return libnbdmod.aio_buffer_to_bytearray(self)
| + 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):
| '''
| @@ -154,7 +156,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, offset, size)
| + return libnbdmod.aio_buffer_is_zero(self._o, offset, size)
|
|
| class NBD(object):
---
generator/Python.ml | 52 ++++++------
python/handle.c | 188 +++++++-------------------------------------
2 files changed, 52 insertions(+), 188 deletions(-)
diff --git a/generator/Python.ml b/generator/Python.ml
index b862b44..270858f 100644
--- a/generator/Python.ml
+++ b/generator/Python.ml
@@ -34,16 +34,11 @@ let
pr "#include <assert.h>\n";
pr "\n";
pr "\
-struct py_aio_buffer {
- Py_ssize_t len;
- void *data;
-};
-
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_buffer (PyObject *);
extern PyObject *nbd_internal_py_get_nbd_buffer_type (void);
static inline struct nbd_handle *
@@ -77,9 +72,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";
@@ -109,9 +101,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";
@@ -301,7 +290,7 @@ let
| BytesPersistIn (n, _)
| BytesPersistOut (n, _) ->
pr " PyObject *%s; /* instance of nbd.Buffer */\n" n;
- pr " struct py_aio_buffer *%s_buf; /* Contents of nbd.Buffer
*/\n" n
+ pr " PyObject *%s_buf; /* Contents of nbd.Buffer */\n" n
| Closure { cbname } ->
pr " struct user_data *%s_user_data = NULL;\n" cbname;
pr " PyObject *py_%s_fn;\n" cbname;
@@ -436,12 +425,16 @@ 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, _) ->
+ | BytesPersistIn (n, _) ->
pr " %s_buf = nbd_internal_py_get_aio_buffer (%s);\n" n n;
pr " if (!%s_buf) goto out;\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
+ pr " if (PyObject_GetBuffer(%s_buf,
&completion_user_data->view,\n" n;
+ pr " PyBUF_CONTIG_RO) < 0) goto
out;\n"
+ | BytesPersistOut (n, _) ->
+ pr " %s_buf = nbd_internal_py_get_aio_buffer (%s);\n" n n;
+ pr " if (!%s_buf) goto out;\n" n;
+ pr " if (PyObject_GetBuffer(%s_buf,
&completion_user_data->view,\n" n;
+ pr " PyBUF_CONTIG) < 0) goto
out;\n"
| Closure { cbname } ->
pr " %s.user_data = %s_user_data = alloc_user_data ();\n"
cbname cbname;
pr " if (%s_user_data == NULL) goto out;\n" cbname;
@@ -482,8 +475,8 @@ let
| 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
+ | BytesPersistIn _ | BytesPersistOut _ ->
+ pr ", completion_user_data->view.buf,
completion_user_data->view.len"
| Closure { cbname } -> pr ", %s" cbname
| Enum (n, _) -> pr ", %s" n
| Flags (n, _) -> pr ", %s_u32" n
@@ -576,7 +569,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_buf);\n" n
| Closure { cbname } ->
pr " free_user_data (%s_user_data);\n" cbname
| Enum _ -> ()
@@ -625,7 +619,7 @@ let
pr " */\n";
pr "struct user_data {\n";
pr " PyObject *fn; /* Optional pointer to Python function.
*/\n";
- pr " PyObject *buf; /* Optional pointer to persistent buffer.
*/\n";
+ pr " Py_buffer view; /* persistent buffer, if view->obj set.
*/\n";
pr "};\n";
pr "\n";
pr "static struct user_data *\n";
@@ -646,7 +640,7 @@ let
pr "\n";
pr " if (data) {\n";
pr " Py_XDECREF (data->fn);\n";
- pr " Py_XDECREF (data->buf);\n";
+ pr " PyBuffer_Release(&data->view);\n";
pr " free (data);\n";
pr " }\n";
pr "}\n";
@@ -768,19 +762,21 @@ let
@classmethod
def from_bytearray(cls, ba):
- '''create an AIO buffer from a bytearray'''
- o = libnbdmod.aio_buffer_from_bytearray(ba)
+ '''create an AIO buffer from a bytearray or other
buffer'''
self = cls(0)
- self._o = o
+ try:
+ self._o = memoryview(ba).cast('B')
+ except TypeError:
+ self._o = bytearray(ba)
return self
def to_bytearray(self):
'''copy an AIO buffer into a bytearray'''
- return libnbdmod.aio_buffer_to_bytearray(self)
+ 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):
'''
@@ -794,7 +790,7 @@ 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)
class NBD(object):
diff --git a/python/handle.c b/python/handle.c
index f84c6e0..7f67159 100644
--- a/python/handle.c
+++ b/python/handle.c
@@ -98,205 +98,73 @@ nbd_internal_py_display_version (PyObject *self, PyObject
*args)
static const char aio_buffer_name[] = "nbd.Buffer";
-struct py_aio_buffer *
+PyObject *
nbd_internal_py_get_aio_buffer (PyObject *buffer)
{
- if (PyObject_IsInstance (buffer, nbd_internal_py_get_nbd_buffer_type ())) {
- PyObject *capsule = PyObject_GetAttrString(buffer, "_o");
- return PyCapsule_GetPointer (capsule, aio_buffer_name);
- }
+ if (PyObject_IsInstance (buffer, nbd_internal_py_get_nbd_buffer_type ()))
+ return PyObject_GetAttrString(buffer, "_o");
PyErr_SetString (PyExc_TypeError,
"aio_buffer: expecting nbd.Buffer instance");
return NULL;
}
-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. */
PyObject *
nbd_internal_py_alloc_aio_buffer (PyObject *self, PyObject *args)
{
- struct py_aio_buffer *buf;
- PyObject *ret;
-
- buf = malloc (sizeof *buf);
- if (buf == NULL) {
- PyErr_NoMemory ();
- return NULL;
- }
-
- if (!PyArg_ParseTuple (args, (char *)
"n:nbd_internal_py_alloc_aio_buffer",
- &buf->len)) {
- free (buf);
- 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,
- (char *)
"O:nbd_internal_py_aio_buffer_from_bytearray",
- &obj))
+ if (!PyArg_ParseTuple (args, (char *)
"n:nbd_internal_py_alloc_aio_buffer",
+ &len))
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);
-
- 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;
- struct py_aio_buffer *buf;
-
- if (!PyArg_ParseTuple (args,
- (char *)
"O:nbd_internal_py_aio_buffer_to_bytearray",
- &obj))
- return NULL;
-
- buf = nbd_internal_py_get_aio_buffer (obj);
- if (buf == NULL)
- return NULL;
-
- return PyByteArray_FromStringAndSize (buf->data, buf->len);
-}
-
-PyObject *
-nbd_internal_py_aio_buffer_size (PyObject *self, PyObject *args)
-{
- PyObject *obj;
- struct py_aio_buffer *buf;
-
- if (!PyArg_ParseTuple (args,
- (char *)
"O:nbd_internal_py_aio_buffer_size",
- &obj))
- return NULL;
-
- buf = nbd_internal_py_get_aio_buffer (obj);
- if (buf == NULL)
- return NULL;
-
- return PyLong_FromSsize_t (buf->len);
+ /* 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;
+ PyObject *ret = NULL;
if (!PyArg_ParseTuple (args,
- (char *)
"Onn:nbd_internal_py_aio_buffer_is_zero",
- &obj, &offset, &size))
+ (char *)
"y*nn:nbd_internal_py_aio_buffer_is_zero",
+ &buf, &offset, &size))
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;
+ Py_INCREF (ret);
+ 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 (is_zero (buf->data + offset, size))
- Py_RETURN_TRUE;
- else
- Py_RETURN_FALSE;
+ ret = PyBool_FromLong (is_zero (buf.buf + offset, size));
+ out:
+ PyBuffer_Release(&buf);
+ return ret;
}
--
2.36.1
Eric Blake
2022-Jun-03 22:26 UTC
[Libguestfs] [libnbd PATCH 5/5] python: Accept all buffer-like objects in aio_p{read, write}
After the work in the previous patch, this one is a trivial feature
addition ;) Now you can do h.aio_pwrite(b'123', 0).
---
python/handle.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/python/handle.c b/python/handle.c
index 7f67159..8575097 100644
--- a/python/handle.c
+++ b/python/handle.c
@@ -104,6 +104,11 @@ nbd_internal_py_get_aio_buffer (PyObject *buffer)
if (PyObject_IsInstance (buffer, nbd_internal_py_get_nbd_buffer_type ()))
return PyObject_GetAttrString(buffer, "_o");
+ if (PyObject_CheckBuffer (buffer)) {
+ Py_INCREF (buffer);
+ return buffer;
+ }
+
PyErr_SetString (PyExc_TypeError,
"aio_buffer: expecting nbd.Buffer instance");
return NULL;
--
2.36.1
Eric Blake
2022-Jun-04 14:54 UTC
[Libguestfs] [libnbd PATCH 0/5] python: Optimize aio_p{read, write}
On Fri, Jun 03, 2022 at 05:26:30PM -0500, Eric Blake wrote:> I didn't get to integrate this with my h.{aio_}pread_structured copy > reductions, but I'm liking how this one turned out.I think, but am still in the middle of testing, that with this series applied first, I don't even need to worry about creating/applying a PySlice object to a PyMemoryView. Instead, I can copy a Py_buffer, tweak the underlying pointer and length to match the subset in use, the use PyMemoryView_FromBuffer. The trickiest part will be getting the reference counting correct. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org