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 <[email protected]> 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 _______________________________________________ Libguestfs mailing list [email protected] https://listman.redhat.com/mailman/listinfo/libguestfs
