[Libguestfs] [libnbd PATCH 3/2] python: Slice pread_structured buffer from original

[Eric Blake]

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