[Libguestfs] [libnbd PATCH v2 8/8] python: Make nbd.Buffer lighter-weight
Richard W.M. Jones
rjones at redhat.com
Tue Jun 7 13:20:35 UTC 2022
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 at 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
More information about the Libguestfs
mailing list