On Fri, Jun 03, 2022 at 05:26:31PM -0500, Eric Blake wrote: Reviewed-by: Richard W.M. Jones <rjones(a)redhat.com&gt; -- 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

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

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

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 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

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

On Fri, Jun 03, 2022 at 05:26:35PM -0500, Eric Blake wrote: Sorry, a late comment on this one. Should we add a test for this, since it's plausibly something that could regress in future? 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