Re: [Libguestfs] [libnbd PATCH 3/2] python: Slice pread_structured buffer from original
On Tue, May 31, 2022 at 07:24:03PM -0500, Eric Blake wrote:
On Wed, Jun 01, 2022 at 02:20:48AM +0300, Nir Soffer wrote:
> On Tue, May 31, 2022 at 6:52 PM Eric Blake <eblake(a)redhat.com> wrote:
> >
> > On Tue, May 31, 2022 at 10:49:03AM -0500, Eric Blake wrote:
> > > This patch fixes the corner-case regression introduced by the previous
> > > patch where the pread_structured callback buffer lifetime ends as soon
> > > as the callback (that is, where accessing a stashed callback parameter
> > > can result in ValueError instead of modifying a harmless copy). With
> > > careful effort, we can create a memoryview of the Python object that
> > > we read into, then slice that memoryview as part of the callback; now
> > > the slice will be valid as long as the original object is also valid.
> > >
> >
> > > | @@ -76,8 +77,24 @@ chunk_wrapper (void *user_data, const vo
> > > | PyObject *py_subbuf = NULL;
> > > | PyObject *py_error = NULL;
> > > |
> > > | - /* Cast away const */
> > > | - py_subbuf = PyMemoryView_FromMemory ((char *) subbuf, count,
PyBUF_READ);
> > > | + if (data->buf) {
>
> In which case we don't have data->buf?
Right now, in nbd_internal_py_aio_read_structured. Fixing that will
eventually become patch 4/2 for this series (the idea is that instead
of requiring the user to pass in an nbd.Buffer object, we should take
any buffer-like object, populate data->buf with zero-copy semantics,
and we're good to go. But to avoid breaking back-compat, we either
have to also special-case existing code using nbd.Buffer, or enhance
the nbd.Buffer class to implement the buffer-like interface).
Following up on this, after first reworking aio_pread to be more
efficient in other series that landed first, I was able to respin this
series to no longer care about pread_structured
vs. aio_pread_structured, and with no temporary regression in being
unable to stash off information that survives past the callback:
https://listman.redhat.com/archives/libguestfs/2022-June/029148.html
I'm still working on patches to make nbd.Buffer not do as much
copying; right now, I'm leaning towards this design:
Add nbd.Buffer.copy_default, set to False.
Enhance existing functions along the lines of:
@classmethod
def nbd.Buffer.from_bytearray(cls, buffer, copy=None):
if copy is None:
copy = nbd.Buffer.copy_default
if copy:
buffer = bytearray(buffer)
self = cls(0)
self._o = buffer
self._init = True
return self
def nbd.Buffer.to_bytearray(self, copy=None):
if copy is None:
copy = self.copy_default # falls back to nbd.Buffer.copy_default as needed
if not hasattr(self, '_init'):
self._o = bytearray(len(self))
self._init = True
if copy:
return bytearray(self._o)
return self._o
With this design, newer libnbd clients default to zero-copy, and can
request a copy when needed; while code that wants to be portable to
older and newer libnbd at the same time can start out by doing:
nbd.Buffer.copy_default = True
at initialization time (since they can't pass a copy= parameter to
.{to,from}_bytearray() in older libnbd).
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org