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 <rjo...@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 _______________________________________________ Libguestfs mailing list Libguestfs@redhat.com https://listman.redhat.com/mailman/listinfo/libguestfs