Libguestfs - Nov 2019 - [PATCH nbdkit 1/2] python: For v2 API, avoid copy by passing a buffer to pread.

As suggested by Nir, here:
https://www.redhat.com/archives/libguestfs/2019-November/thread.html#00220

It's more efficient if we pass the C buffer directly to Python code.
In some cases the Python code will be able to write directly into the
C buffer using functions like file.readinto and socket.recv_into.
This avoids an extra copy.

Thanks: Nir Soffer
---
 plugins/python/example.py               |  8 ++++---
 plugins/python/nbdkit-python-plugin.pod | 16 +++++--------
 plugins/python/python.c                 | 32 +++++++++++++++----------
 tests/python-exception.py               |  4 ++--
 tests/shebang.py                        |  5 ++--
 5 files changed, 36 insertions(+), 29 deletions(-)

diff --git a/plugins/python/example.py b/plugins/python/example.py
index c85d2f8..c04b7e2 100644
--- a/plugins/python/example.py
+++ b/plugins/python/example.py
@@ -60,10 +60,12 @@ def get_size(h):
     return len(disk)
 
 
-def pread(h, count, offset, flags):
+def pread(h, buf, offset, flags):
     global disk
-    return disk[offset:offset+count]
-
+    end = offset + len(buf)
+    buf[:] = disk[offset:end]
+    # or if reading from a file you can use:
+    #f.readinto(buf)
 
 def pwrite(h, buf, offset, flags):
     global disk
diff --git a/plugins/python/nbdkit-python-plugin.pod
b/plugins/python/nbdkit-python-plugin.pod
index 0b31ebc..4065ec7 100644
--- a/plugins/python/nbdkit-python-plugin.pod
+++ b/plugins/python/nbdkit-python-plugin.pod
@@ -38,7 +38,7 @@ C<__main__> module):
    # see below
  def get_size(h):
    # see below
- def pread(h, count, offset, flags):
+ def pread(h, buf, offset, flags):
    # see below
 
 Note that the subroutines must have those literal names (like C<open>),
@@ -249,16 +249,12 @@ contents will be garbage collected.
 
 (Required)
 
- def pread(h, count, offset, flags):
-   # construct a buffer of length count bytes and return it
+ def pread(h, buf, offset, flags):
+   # read into the buffer
 
-The body of your C<pread> function should construct a buffer of length
-(at least) C<count> bytes.  You should read C<count> bytes from the
-disk starting at C<offset>.  C<flags> is always 0.
-
-The returned buffer can be any type compatible with the Python 3
-buffer protocol, such as bytearray, bytes or memoryview
-(L<https://docs.python.org/3/c-api/buffer.html>)
+The body of your C<pread> function should read exactly C<len(buf)>
+bytes of data starting at disk C<offset> and write it into the buffer
+C<buf>.  C<flags> is always 0.
 
 NBD only supports whole reads, so your function should try to read
 the whole region (perhaps requiring a loop).  If the read fails or
diff --git a/plugins/python/python.c b/plugins/python/python.c
index 252ca37..79766df 100644
--- a/plugins/python/python.c
+++ b/plugins/python/python.c
@@ -527,7 +527,9 @@ py_pread (void *handle, void *buf, uint32_t count, uint64_t
offset,
     r = PyObject_CallFunction (fn, "OiL", obj, count, offset, NULL);
     break;
   case 2:
-    r = PyObject_CallFunction (fn, "OiLI", obj, count, offset, flags,
NULL);
+    r = PyObject_CallFunction (fn, "ONLI", obj,
+          PyMemoryView_FromMemory ((char *)buf, count, PyBUF_WRITE),
+          offset, flags, NULL);
     break;
   default: abort ();
   }
@@ -535,19 +537,25 @@ py_pread (void *handle, void *buf, uint32_t count,
uint64_t offset,
   if (check_python_failure ("pread") == -1)
     return ret;
 
-  if (PyObject_GetBuffer (r, &view, PyBUF_SIMPLE) == -1) {
-    nbdkit_error ("%s: value returned from pread does not support the
"
-                  "buffer protocol",
-                  script);
-    goto out;
-  }
+  if (py_api_version == 1) {
+    /* In API v1 the Python pread function had to return a buffer
+     * protocol compatible function.  In API v2+ it writes directly to
+     * the C buffer so this code is not used.
+     */
+    if (PyObject_GetBuffer (r, &view, PyBUF_SIMPLE) == -1) {
+      nbdkit_error ("%s: value returned from pread does not support the
"
+                    "buffer protocol",
+                    script);
+      goto out;
+    }
 
-  if (view.len < count) {
-    nbdkit_error ("%s: buffer returned from pread is too small",
script);
-    goto out;
-  }
+    if (view.len < count) {
+      nbdkit_error ("%s: buffer returned from pread is too small",
script);
+      goto out;
+    }
 
-  memcpy (buf, view.buf, count);
+    memcpy (buf, view.buf, count);
+  }
   ret = 0;
 
 out:
diff --git a/tests/python-exception.py b/tests/python-exception.py
index d0c79bb..ee4a3f3 100644
--- a/tests/python-exception.py
+++ b/tests/python-exception.py
@@ -62,5 +62,5 @@ def get_size(h):
     return 0
 
 
-def pread(h, count, offset):
-    return ""
+def pread(h, buf, offset):
+    buf[:] = bytearray(len(buf))
diff --git a/tests/shebang.py b/tests/shebang.py
index 6f33623..0634589 100755
--- a/tests/shebang.py
+++ b/tests/shebang.py
@@ -13,6 +13,7 @@ def get_size(h):
     return len(disk)
 
 
-def pread(h, count, offset):
+def pread(h, buf, offset):
     global disk
-    return disk[offset:offset+count]
+    end = offset + len(buf)
+    buf[:] = disk[offset:end]
-- 
2.23.0

---
 tests/test-python-plugin.py | 12 +++---------
 tests/test_python.py        | 18 ++----------------
 2 files changed, 5 insertions(+), 25 deletions(-)

diff --git a/tests/test-python-plugin.py b/tests/test-python-plugin.py
index d7335bd..8e90bc2 100644
--- a/tests/test-python-plugin.py
+++ b/tests/test-python-plugin.py
@@ -95,16 +95,10 @@ def can_cache (h):
     elif cache == "native":
         return nbdkit.CACHE_NATIVE
 
-def pread (h, count, offset, flags):
+def pread (h, buf, offset, flags):
     assert flags == 0
-    pread_result = cfg.get ('pread_result', "bytearray")
-    b = h['disk'][offset:offset+count]
-    if pread_result == "bytearray":
-        return b
-    elif pread_result == "bytes":
-        return bytes (b)
-    elif pread_result == "memoryview":
-        return memoryview (b)
+    end = offset + len(buf)
+    buf[:] = h['disk'][offset:end]
 
 def pwrite (h, buf, offset, flags):
     expect_fua = cfg.get ('pwrite_expect_fua', False)
diff --git a/tests/test_python.py b/tests/test_python.py
index b2c60f0..6b9f297 100755
--- a/tests/test_python.py
+++ b/tests/test_python.py
@@ -155,26 +155,12 @@ class Test (unittest.TestCase):
 
     # Not yet implemented: can_extents.
 
-    def test_pread_bytearray (self):
-        """Test pread returning bytearray."""
+    def test_pread (self):
+        """Test pread."""
         self.connect ({"size": 512})
         buf = self.h.pread (512, 0)
         assert buf == bytearray (512)
 
-    def test_pread_bytes (self):
-        """Test pread returning bytes."""
-        self.connect ({"size": 512,
-                       "pread_result": "bytes"})
-        buf = self.h.pread (512, 0)
-        assert buf == bytearray (512)
-
-    def test_pread_memoryview (self):
-        """Test pread returning memoryview."""
-        self.connect ({"size": 512,
-                       "pread_result": "memoryview"})
-        buf = self.h.pread (512, 0)
-        assert buf == bytearray (512)
-
     # Test pwrite + flags.
     def test_pwrite (self):
         self.connect ({"size": 512})
-- 
2.23.0

On 11/25/19 7:17 AM, Richard W.M. Jones wrote:
> It's more efficient if we pass the C buffer directly to Python code.
> In some cases the Python code will be able to write directly into the
> C buffer using functions like file.readinto and socket.recv_into.
> This avoids an extra copy.
> 
> Thanks: Nir Soffer
> ---
>   plugins/python/example.py               |  8 ++++---
>   plugins/python/nbdkit-python-plugin.pod | 16 +++++--------
>   plugins/python/python.c                 | 32 +++++++++++++++----------
>   tests/python-exception.py               |  4 ++--
>   tests/shebang.py                        |  5 ++--
>   5 files changed, 36 insertions(+), 29 deletions(-)
Hmm, my counterproposal that adds kwarg parameters without requiring 
API_VERSION makes it impossible to make this sort of switch.  So this 
alone is a good reason why a clean break in python plugins opting in to 
API_VERSION 1 or 2 makes sense (and maybe we still want v2 to use 
keywords instead of positional parameters, but with less impact to v1 
clients).

As I said with my counterproposal, now is the time to decide what we 
like best out of both proposals, and probably another round of churn on 
what our final interface will be.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org