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