Libguestfs - Aug 2020 - Re: [PATCH nbdkit] python: Implement can_extents + extents.

---
 plugins/python/nbdkit-python-plugin.pod | 19 ++++++-
 plugins/python/python.c                 | 75 +++++++++++++++++++++++++
 tests/test-python-plugin.py             |  8 ++-
 tests/test_python.py                    | 73 +++++++++++++++++++++++-
 4 files changed, 169 insertions(+), 6 deletions(-)

diff --git a/plugins/python/nbdkit-python-plugin.pod
b/plugins/python/nbdkit-python-plugin.pod
index b20f51f7..d7b6033f 100644
--- a/plugins/python/nbdkit-python-plugin.pod
+++ b/plugins/python/nbdkit-python-plugin.pod
@@ -271,6 +271,13 @@ contents will be garbage collected.
    # return nbdkit.CACHE_NONE or nbdkit.CACHE_EMULATE
    # or nbdkit.CACHE_NATIVE
 
+=item C<can_extents>
+
+(Optional)
+
+ def can_extents(h):
+   # return a boolean
+
 =item C<pread>
 
 (Required)
@@ -365,6 +372,14 @@ indicated range.
 If the cache operation fails, your function should throw an exception,
 optionally using C<nbdkit.set_error> first.
 
+=item C<extents>
+
+(Optional)
+
+ def extents(h, count, offset, flags):
+   # return a list of (offset, length, type) tuples:
+   return [ (off1, len1, type1), (off2, len2, type2), ... ]
+
 =back
 
 =head2 Missing callbacks
@@ -382,9 +397,7 @@ C<version>,
 C<longname>,
 C<description>,
 C<config_help>,
-C<magic_config_key>,
-C<can_extents>,
-C<extents>.
+C<magic_config_key>.
 
 These are not yet supported.
 
diff --git a/plugins/python/python.c b/plugins/python/python.c
index 398473f5..585cd9e6 100644
--- a/plugins/python/python.c
+++ b/plugins/python/python.c
@@ -1020,6 +1020,79 @@ py_can_cache (void *handle)
     return NBDKIT_CACHE_NONE;
 }
 
+static int
+py_can_extents (void *handle)
+{
+  ACQUIRE_PYTHON_GIL_FOR_CURRENT_SCOPE;
+  return boolean_callback (handle, "can_extents",
"extents");
+}
+
+static int
+py_extents (void *handle, uint32_t count, uint64_t offset,
+            uint32_t flags, struct nbdkit_extents *extents)
+{
+  ACQUIRE_PYTHON_GIL_FOR_CURRENT_SCOPE;
+  struct handle *h = handle;
+  PyObject *fn;
+  PyObject *r;
+  Py_ssize_t i, size;
+
+  if (callback_defined ("extents", &fn)) {
+    PyErr_Clear ();
+
+    r = PyObject_CallFunction (fn, "OiLI", h->py_h, count, offset,
flags);
+    Py_DECREF (fn);
+    if (check_python_failure ("extents") == -1)
+      return -1;
+
+    /* We expect a list of extents to be returned.  Each extent is a
+     * tuple (offset, length, type).
+     */
+    if (!PyList_Check (r)) {
+      nbdkit_error ("extents method did not return a list");
+      Py_DECREF (r);
+      return -1;
+    }
+
+    size = PyList_Size (r);
+    for (i = 0; i < size; ++i) {
+      PyObject *t, *py_offset, *py_length, *py_type;
+      uint64_t extent_offset, extent_length;
+      uint32_t extent_type;
+
+      t = PyList_GetItem (r, i);
+      if (!PyTuple_Check (t) || PyTuple_Size (t) != 3) {
+        nbdkit_error ("extents method did not return a list of
3-tuples");
+        Py_DECREF (r);
+        return -1;
+      }
+      py_offset = PyTuple_GetItem (t, 0);
+      py_length = PyTuple_GetItem (t, 1);
+      py_type = PyTuple_GetItem (t, 2);
+      extent_offset = PyLong_AsUnsignedLongLong (py_offset);
+      extent_length = PyLong_AsUnsignedLongLong (py_length);
+      extent_type = PyLong_AsUnsignedLong (py_type);
+      if (check_python_failure ("PyLong") == -1) {
+        Py_DECREF (r);
+        return -1;
+      }
+      if (nbdkit_add_extent (extents,
+                             extent_offset, extent_length, extent_type) == -1)
{
+        Py_DECREF (r);
+        return -1;
+      }
+    }
+
+    Py_DECREF (r);
+  }
+  else {
+    nbdkit_error ("%s not implemented", "extents");
+    return -1;
+  }
+
+  return 0;
+}
+
 #define py_config_help \
   "script=<FILENAME>     (required) The Python plugin to
run.\n" \
   "[other arguments may be used by the plugin that you load]"
@@ -1058,6 +1131,7 @@ static struct nbdkit_plugin plugin = {
   .can_fast_zero     = py_can_fast_zero,
   .can_fua           = py_can_fua,
   .can_cache         = py_can_cache,
+  .can_extents       = py_can_extents,
 
   .pread             = py_pread,
   .pwrite            = py_pwrite,
@@ -1065,6 +1139,7 @@ static struct nbdkit_plugin plugin = {
   .trim              = py_trim,
   .zero              = py_zero,
   .cache             = py_cache,
+  .extents           = py_extents,
 };
 
 NBDKIT_REGISTER_PLUGIN (plugin)
diff --git a/tests/test-python-plugin.py b/tests/test-python-plugin.py
index 8e90bc23..d7331df9 100644
--- a/tests/test-python-plugin.py
+++ b/tests/test-python-plugin.py
@@ -1,5 +1,5 @@
 # nbdkit test plugin
-# Copyright (C) 2019 Red Hat Inc.
+# Copyright (C) 2019-2020 Red Hat Inc.
 #
 # Redistribution and use in source and binary forms, with or without
 # modification, are permitted provided that the following conditions are
@@ -95,6 +95,9 @@ def can_cache (h):
     elif cache == "native":
         return nbdkit.CACHE_NATIVE
 
+def can_extents (h):
+    return cfg.get ('can_extents', False)
+
 def pread (h, buf, offset, flags):
     assert flags == 0
     end = offset + len(buf)
@@ -131,3 +134,6 @@ def zero (h, count, offset, flags):
 def cache (h, count, offset, flags):
     assert flags == 0
     # do nothing
+
+def extents(h, count, offset, flags):
+    return cfg.get ('extents', [])
diff --git a/tests/test_python.py b/tests/test_python.py
index 6b9f2979..a8a1a79f 100755
--- a/tests/test_python.py
+++ b/tests/test_python.py
@@ -1,6 +1,6 @@
 #!/usr/bin/env python3
 # nbdkit
-# Copyright (C) 2019 Red Hat Inc.
+# Copyright (C) 2019-2020 Red Hat Inc.
 #
 # Redistribution and use in source and binary forms, with or without
 # modification, are permitted provided that the following conditions are
@@ -153,7 +153,19 @@ class Test (unittest.TestCase):
         self.connect ({"size": 512, "can_cache":
"native"})
         assert self.h.can_cache()
 
-    # Not yet implemented: can_extents.
+    # In theory we could use a test like this, but nbdkit can
+    # always synthesize base:allocation block_status responses
+    # even if the plugin doesn't support them.
+    #
+    #def test_can_extents_true (self):
+    #    self.h.add_meta_context ("base:allocation")
+    #    self.connect ({"size": 512, "can_extents": True})
+    #    assert self.h.can_meta_context ("base:allocation")
+    #
+    #def test_can_extents_false (self):
+    #    self.h.add_meta_context ("base:allocation")
+    #    self.connect ({"size": 512, "can_extents": False})
+    #    assert not self.h.can_meta_context ("base:allocation")
 
     def test_pread (self):
         """Test pread."""
@@ -220,3 +232,60 @@ class Test (unittest.TestCase):
         """Test cache."""
         self.connect ({"size": 512, "can_cache":
"native"})
         self.h.cache (512, 0)
+
+    # We don't have access to the magic constants defined in the
+    # nbdkit module, so redefine them here.
+    EXTENT_HOLE = 1
+    EXTENT_ZERO = 2
+
+    def test_extents_1 (self):
+        """Test extents."""
+
+        offset = None
+        entries = []
+
+        def f(meta_context, o, e, err):
+            nonlocal offset, entries
+            if meta_context != "base:allocation": return
+            offset = o
+            entries = e
+
+        self.h.add_meta_context ("base:allocation")
+        self.connect ({"size": 512,
+                       "can_extents": True,
+                       "extents":
+                       [ (0, 512, self.EXTENT_HOLE|self.EXTENT_ZERO) ]})
+
+        self.h.block_status (512, 0, lambda *args : f (*args))
+        assert offset == 0
+        assert entries == [ 512, self.EXTENT_HOLE|self.EXTENT_ZERO ]
+
+    def test_extents_2 (self):
+        """Test extents."""
+
+        offset = None
+        entries = []
+
+        def f(meta_context, o, e, err):
+            nonlocal offset, entries
+            if meta_context != "base:allocation": return
+            offset = o
+            entries = e
+
+        self.h.add_meta_context ("base:allocation")
+        self.connect ({"size": 2048,
+                       "can_extents": True,
+                       "extents":
+                       [ (0, 512, self.EXTENT_HOLE|self.EXTENT_ZERO),
+                         (512, 512, 0),
+                         (1024, 1024, self.EXTENT_ZERO) ]})
+
+        self.h.block_status (2048, 0, lambda *args : f (*args))
+        assert offset == 0
+        assert entries == [ 512, self.EXTENT_HOLE|self.EXTENT_ZERO,
+                            512, 0,
+                            1024, self.EXTENT_ZERO ]
+
+        self.h.block_status (1024, 1024, lambda *args : f (*args))
+        assert offset == 1024
+        assert entries == [ 1024, self.EXTENT_ZERO ]
-- 
2.27.0

On Mon, Aug 10, 2020 at 11:50:59AM +0100, Richard W.M. Jones
wrote:
> +    def test_extents_1 (self):
> +        """Test extents."""
> +
> +        offset = None
> +        entries = []
> +
> +        def f(meta_context, o, e, err):
> +            nonlocal offset, entries
> +            if meta_context != "base:allocation": return
> +            offset = o
> +            entries = e
> +
> +        self.h.add_meta_context ("base:allocation")
> +        self.connect ({"size": 512,
> +                       "can_extents": True,
> +                       "extents":
> +                       [ (0, 512, self.EXTENT_HOLE|self.EXTENT_ZERO) ]})
> +
> +        self.h.block_status (512, 0, lambda *args : f (*args))
This can of course just be "f" instead of using the pointless
lambda (and in other places).  I updated my copy.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-top is 'top' for virtual machines.  Tiny program with many
powerful monitoring features, net stats, disk stats, logging, etc.
http://people.redhat.com/~rjones/virt-top

On 8/10/20 5:50 AM, Richard W.M. Jones wrote:
> ---
>   plugins/python/nbdkit-python-plugin.pod | 19 ++++++-
>   plugins/python/python.c                 | 75 +++++++++++++++++++++++++
>   tests/test-python-plugin.py             |  8 ++-
>   tests/test_python.py                    | 73 +++++++++++++++++++++++-
>   4 files changed, 169 insertions(+), 6 deletions(-)
> 
> diff --git a/plugins/python/nbdkit-python-plugin.pod
b/plugins/python/nbdkit-python-plugin.pod
> index b20f51f7..d7b6033f 100644
> --- a/plugins/python/nbdkit-python-plugin.pod
> +++ b/plugins/python/nbdkit-python-plugin.pod
> @@ -271,6 +271,13 @@ contents will be garbage collected.
>      # return nbdkit.CACHE_NONE or nbdkit.CACHE_EMULATE
>      # or nbdkit.CACHE_NATIVE
We expose various constants through 'import nbdkit', so...
> +++ b/plugins/python/python.c
> @@ -1020,6 +1020,79 @@ py_can_cache (void *handle)
>       return NBDKIT_CACHE_NONE;
>   }
>   
> +static int
> +py_can_extents (void *handle)
> +{
> +  ACQUIRE_PYTHON_GIL_FOR_CURRENT_SCOPE;
> +  return boolean_callback (handle, "can_extents",
"extents");
> +}
> +
> +static int
> +py_extents (void *handle, uint32_t count, uint64_t offset,
> +            uint32_t flags, struct nbdkit_extents *extents)
> +{
> +  ACQUIRE_PYTHON_GIL_FOR_CURRENT_SCOPE;
> +  struct handle *h = handle;
> +  PyObject *fn;
> +  PyObject *r;
> +  Py_ssize_t i, size;
> +
> +  if (callback_defined ("extents", &fn)) {
> +    PyErr_Clear ();
> +
> +    r = PyObject_CallFunction (fn, "OiLI", h->py_h, count,
offset, flags);
> +    Py_DECREF (fn);
> +    if (check_python_failure ("extents") == -1)
> +      return -1;
> +
> +    /* We expect a list of extents to be returned.  Each extent is a
> +     * tuple (offset, length, type).
> +     */
> +    if (!PyList_Check (r)) {
> +      nbdkit_error ("extents method did not return a list");
> +      Py_DECREF (r);
> +      return -1;
> +    }
> +
> +    size = PyList_Size (r);
> +    for (i = 0; i < size; ++i) {
> +      PyObject *t, *py_offset, *py_length, *py_type;
> +      uint64_t extent_offset, extent_length;
> +      uint32_t extent_type;
> +
> +      t = PyList_GetItem (r, i);
> +      if (!PyTuple_Check (t) || PyTuple_Size (t) != 3) {
> +        nbdkit_error ("extents method did not return a list of
3-tuples");
> +        Py_DECREF (r);
> +        return -1;
> +      }
> +      py_offset = PyTuple_GetItem (t, 0);
> +      py_length = PyTuple_GetItem (t, 1);
> +      py_type = PyTuple_GetItem (t, 2);
> +      extent_offset = PyLong_AsUnsignedLongLong (py_offset);
> +      extent_length = PyLong_AsUnsignedLongLong (py_length);
> +      extent_type = PyLong_AsUnsignedLong (py_type);
> +      if (check_python_failure ("PyLong") == -1) {
Is it really right to be doing error checking only once, but after three 
calls into PyLong_* functions?  I'm thinking of someone returning 
('a',1,1).  But as long as the later successful calls don't wipe out
an
earlier failure, this looks like it works.
> +        Py_DECREF (r);
> +        return -1;
> +      }
> +      if (nbdkit_add_extent (extents,
> +                             extent_offset, extent_length, extent_type) ==
-1) {
> +        Py_DECREF (r);
> +        return -1;
> +      }
> +    }
> +
> +    Py_DECREF (r);
> +  }
> +  else {
> +    nbdkit_error ("%s not implemented", "extents");
> +    return -1;
Here, we may be better off doing the same fallback as the C code, and 
just reporting the entire image as a single data extent, rather than 
always failing.
> +++ b/tests/test_python.py
> @@ -220,3 +232,60 @@ class Test (unittest.TestCase):
>           """Test cache."""
>           self.connect ({"size": 512, "can_cache":
"native"})
>           self.h.cache (512, 0)
> +
> +    # We don't have access to the magic constants defined in the
> +    # nbdkit module, so redefine them here.
> +    EXTENT_HOLE = 1
> +    EXTENT_ZERO = 2
...these constants should have been available through 'import nbdkit'.

Otherwise this looks reasonable.

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

On Mon, Aug 10, 2020 at 08:07:37AM -0500, Eric Blake
wrote:
> >+      py_offset = PyTuple_GetItem (t, 0);
> >+      py_length = PyTuple_GetItem (t, 1);
> >+      py_type = PyTuple_GetItem (t, 2);
> >+      extent_offset = PyLong_AsUnsignedLongLong (py_offset);
> >+      extent_length = PyLong_AsUnsignedLongLong (py_length);
> >+      extent_type = PyLong_AsUnsignedLong (py_type);
> >+      if (check_python_failure ("PyLong") == -1) {
> 
> Is it really right to be doing error checking only once, but after
> three calls into PyLong_* functions?  I'm thinking of someone
> returning ('a',1,1).  But as long as the later successful calls
> don't wipe out an earlier failure, this looks like it works.
Yes, the error is thread-local and only cleared by calling
PyErr_Clear().

I modified the test to return ('a', 1, 1) element and it failed:

  nbdkit: python: error: test-python-plugin.py: PyLong: error: an integer is
required
> >+        Py_DECREF (r);
> >+        return -1;
> >+      }
> >+      if (nbdkit_add_extent (extents,
> >+                             extent_offset, extent_length,
extent_type) == -1) {
> >+        Py_DECREF (r);
> >+        return -1;
> >+      }
> >+    }
> >+
> >+    Py_DECREF (r);
> >+  }
> >+  else {
> >+    nbdkit_error ("%s not implemented",
"extents");
> >+    return -1;
> 
> Here, we may be better off doing the same fallback as the C code,
> and just reporting the entire image as a single data extent, rather
> than always failing.
Yes, good idea, that's an easy change to make.
> >+++ b/tests/test_python.py
> 
> >@@ -220,3 +232,60 @@ class Test (unittest.TestCase):
> >          """Test cache."""
> >          self.connect ({"size": 512, "can_cache":
"native"})
> >          self.h.cache (512, 0)
> >+
> >+    # We don't have access to the magic constants defined in the
> >+    # nbdkit module, so redefine them here.
> >+    EXTENT_HOLE = 1
> >+    EXTENT_ZERO = 2
> 
> ...these constants should have been available through 'import
nbdkit'.
Not in this test, because nbdkit isn't a real module, it's one
synthesized in python.c.  Perhaps it _should_ be, but that's not how
it's currently implemented.
> Otherwise this looks reasonable.
Thanks,

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-top is 'top' for virtual machines.  Tiny program with many
powerful monitoring features, net stats, disk stats, logging, etc.
http://people.redhat.com/~rjones/virt-top

On Mon, Aug 10, 2020 at 1:51 PM Richard W.M. Jones <rjones@redhat.com>
wrote:
>
> ---
>  plugins/python/nbdkit-python-plugin.pod | 19 ++++++-
>  plugins/python/python.c                 | 75 +++++++++++++++++++++++++
>  tests/test-python-plugin.py             |  8 ++-
>  tests/test_python.py                    | 73 +++++++++++++++++++++++-
>  4 files changed, 169 insertions(+), 6 deletions(-)
>
> diff --git a/plugins/python/nbdkit-python-plugin.pod
b/plugins/python/nbdkit-python-plugin.pod
> index b20f51f7..d7b6033f 100644
> --- a/plugins/python/nbdkit-python-plugin.pod
> +++ b/plugins/python/nbdkit-python-plugin.pod
> @@ -271,6 +271,13 @@ contents will be garbage collected.
>     # return nbdkit.CACHE_NONE or nbdkit.CACHE_EMULATE
>     # or nbdkit.CACHE_NATIVE
>
> +=item C<can_extents>
> +
> +(Optional)
> +
> + def can_extents(h):
> +   # return a boolean
> +
>  =item C<pread>
>
>  (Required)
> @@ -365,6 +372,14 @@ indicated range.
>  If the cache operation fails, your function should throw an exception,
>  optionally using C<nbdkit.set_error> first.
>
> +=item C<extents>
> +
> +(Optional)
> +
> + def extents(h, count, offset, flags):
> +   # return a list of (offset, length, type) tuples:
> +   return [ (off1, len1, type1), (off2, len2, type2), ... ]
Better to accept an iterable of tuples. This allows the plugin to do:

    def extents(h, count, offset, flags):
        for extent in client.extents():
            if extent.offset > offset:
                return
            yield extent.start, extent.length, type

The logic is just an example to illustrate the benefit of using
generators. The plugin does not have to build a list of extent,
and nbdkit also may not need this list. This may have performance
benefits, and may make the code nicer.
> +
>  =back
>
>  =head2 Missing callbacks
> @@ -382,9 +397,7 @@ C<version>,
>  C<longname>,
>  C<description>,
>  C<config_help>,
> -C<magic_config_key>,
> -C<can_extents>,
> -C<extents>.
> +C<magic_config_key>.
>
>  These are not yet supported.
>
> diff --git a/plugins/python/python.c b/plugins/python/python.c
> index 398473f5..585cd9e6 100644
> --- a/plugins/python/python.c
> +++ b/plugins/python/python.c
> @@ -1020,6 +1020,79 @@ py_can_cache (void *handle)
>      return NBDKIT_CACHE_NONE;
>  }
>
> +static int
> +py_can_extents (void *handle)
> +{
> +  ACQUIRE_PYTHON_GIL_FOR_CURRENT_SCOPE;
> +  return boolean_callback (handle, "can_extents",
"extents");
> +}
> +
> +static int
> +py_extents (void *handle, uint32_t count, uint64_t offset,
> +            uint32_t flags, struct nbdkit_extents *extents)
> +{
> +  ACQUIRE_PYTHON_GIL_FOR_CURRENT_SCOPE;
> +  struct handle *h = handle;
> +  PyObject *fn;
> +  PyObject *r;
> +  Py_ssize_t i, size;
> +
> +  if (callback_defined ("extents", &fn)) {
> +    PyErr_Clear ();
> +
> +    r = PyObject_CallFunction (fn, "OiLI", h->py_h, count,
offset, flags);
> +    Py_DECREF (fn);
> +    if (check_python_failure ("extents") == -1)
> +      return -1;
> +
> +    /* We expect a list of extents to be returned.  Each extent is a
> +     * tuple (offset, length, type).
> +     */
> +    if (!PyList_Check (r)) {
This will work only with the list. We want to check if we got a type
we can iterated on. I don't know the APi needed but I'm sure this is
supported by the C API.
> +      nbdkit_error ("extents method did not return a list");
> +      Py_DECREF (r);
> +      return -1;
> +    }
> +
> +    size = PyList_Size (r);
> +    for (i = 0; i < size; ++i) {
> +      PyObject *t, *py_offset, *py_length, *py_type;
> +      uint64_t extent_offset, extent_length;
> +      uint32_t extent_type;
> +
> +      t = PyList_GetItem (r, i);
> +      if (!PyTuple_Check (t) || PyTuple_Size (t) != 3) {
> +        nbdkit_error ("extents method did not return a list of
3-tuples");
> +        Py_DECREF (r);
> +        return -1;
> +      }
> +      py_offset = PyTuple_GetItem (t, 0);
> +      py_length = PyTuple_GetItem (t, 1);
> +      py_type = PyTuple_GetItem (t, 2);
> +      extent_offset = PyLong_AsUnsignedLongLong (py_offset);
> +      extent_length = PyLong_AsUnsignedLongLong (py_length);
> +      extent_type = PyLong_AsUnsignedLong (py_type);
> +      if (check_python_failure ("PyLong") == -1) {
> +        Py_DECREF (r);
> +        return -1;
> +      }
> +      if (nbdkit_add_extent (extents,
> +                             extent_offset, extent_length, extent_type) ==
-1) {
Based on this nbdkit really does not need a list, only way to iterate on client
response and call add_extent for each item. This is exactly the use case for
generator functions.
> +        Py_DECREF (r);
> +        return -1;
> +      }
> +    }
> +
> +    Py_DECREF (r);
> +  }
> +  else {
> +    nbdkit_error ("%s not implemented", "extents");
> +    return -1;
> +  }
> +
> +  return 0;
> +}
> +
>  #define py_config_help \
>    "script=<FILENAME>     (required) The Python plugin to
run.\n" \
>    "[other arguments may be used by the plugin that you load]"
> @@ -1058,6 +1131,7 @@ static struct nbdkit_plugin plugin = {
>    .can_fast_zero     = py_can_fast_zero,
>    .can_fua           = py_can_fua,
>    .can_cache         = py_can_cache,
> +  .can_extents       = py_can_extents,
>
>    .pread             = py_pread,
>    .pwrite            = py_pwrite,
> @@ -1065,6 +1139,7 @@ static struct nbdkit_plugin plugin = {
>    .trim              = py_trim,
>    .zero              = py_zero,
>    .cache             = py_cache,
> +  .extents           = py_extents,
>  };
>
>  NBDKIT_REGISTER_PLUGIN (plugin)
> diff --git a/tests/test-python-plugin.py b/tests/test-python-plugin.py
> index 8e90bc23..d7331df9 100644
> --- a/tests/test-python-plugin.py
> +++ b/tests/test-python-plugin.py
> @@ -1,5 +1,5 @@
>  # nbdkit test plugin
> -# Copyright (C) 2019 Red Hat Inc.
> +# Copyright (C) 2019-2020 Red Hat Inc.
>  #
>  # Redistribution and use in source and binary forms, with or without
>  # modification, are permitted provided that the following conditions are
> @@ -95,6 +95,9 @@ def can_cache (h):
>      elif cache == "native":
>          return nbdkit.CACHE_NATIVE
>
> +def can_extents (h):
> +    return cfg.get ('can_extents', False)
> +
>  def pread (h, buf, offset, flags):
>      assert flags == 0
>      end = offset + len(buf)
> @@ -131,3 +134,6 @@ def zero (h, count, offset, flags):
>  def cache (h, count, offset, flags):
>      assert flags == 0
>      # do nothing
> +
> +def extents(h, count, offset, flags):
> +    return cfg.get ('extents', [])
> diff --git a/tests/test_python.py b/tests/test_python.py
> index 6b9f2979..a8a1a79f 100755
> --- a/tests/test_python.py
> +++ b/tests/test_python.py
> @@ -1,6 +1,6 @@
>  #!/usr/bin/env python3
>  # nbdkit
> -# Copyright (C) 2019 Red Hat Inc.
> +# Copyright (C) 2019-2020 Red Hat Inc.
>  #
>  # Redistribution and use in source and binary forms, with or without
>  # modification, are permitted provided that the following conditions are
> @@ -153,7 +153,19 @@ class Test (unittest.TestCase):
>          self.connect ({"size": 512, "can_cache":
"native"})
>          assert self.h.can_cache()
>
> -    # Not yet implemented: can_extents.
> +    # In theory we could use a test like this, but nbdkit can
> +    # always synthesize base:allocation block_status responses
> +    # even if the plugin doesn't support them.
> +    #
> +    #def test_can_extents_true (self):
> +    #    self.h.add_meta_context ("base:allocation")
> +    #    self.connect ({"size": 512, "can_extents":
True})
> +    #    assert self.h.can_meta_context ("base:allocation")
> +    #
> +    #def test_can_extents_false (self):
> +    #    self.h.add_meta_context ("base:allocation")
> +    #    self.connect ({"size": 512, "can_extents":
False})
> +    #    assert not self.h.can_meta_context ("base:allocation")
>
>      def test_pread (self):
>          """Test pread."""
> @@ -220,3 +232,60 @@ class Test (unittest.TestCase):
>          """Test cache."""
>          self.connect ({"size": 512, "can_cache":
"native"})
>          self.h.cache (512, 0)
> +
> +    # We don't have access to the magic constants defined in the
> +    # nbdkit module, so redefine them here.
> +    EXTENT_HOLE = 1
> +    EXTENT_ZERO = 2
> +
> +    def test_extents_1 (self):
> +        """Test extents."""
> +
> +        offset = None
> +        entries = []
> +
> +        def f(meta_context, o, e, err):
> +            nonlocal offset, entries
> +            if meta_context != "base:allocation": return
> +            offset = o
> +            entries = e
> +
> +        self.h.add_meta_context ("base:allocation")
> +        self.connect ({"size": 512,
> +                       "can_extents": True,
> +                       "extents":
> +                       [ (0, 512, self.EXTENT_HOLE|self.EXTENT_ZERO) ]})
> +
> +        self.h.block_status (512, 0, lambda *args : f (*args))
> +        assert offset == 0
> +        assert entries == [ 512, self.EXTENT_HOLE|self.EXTENT_ZERO ]
> +
> +    def test_extents_2 (self):
> +        """Test extents."""
> +
> +        offset = None
> +        entries = []
> +
> +        def f(meta_context, o, e, err):
> +            nonlocal offset, entries
> +            if meta_context != "base:allocation": return
> +            offset = o
> +            entries = e
> +
> +        self.h.add_meta_context ("base:allocation")
> +        self.connect ({"size": 2048,
> +                       "can_extents": True,
> +                       "extents":
> +                       [ (0, 512, self.EXTENT_HOLE|self.EXTENT_ZERO),
> +                         (512, 512, 0),
> +                         (1024, 1024, self.EXTENT_ZERO) ]})
> +
> +        self.h.block_status (2048, 0, lambda *args : f (*args))
> +        assert offset == 0
> +        assert entries == [ 512, self.EXTENT_HOLE|self.EXTENT_ZERO,
> +                            512, 0,
> +                            1024, self.EXTENT_ZERO ]
> +
> +        self.h.block_status (1024, 1024, lambda *args : f (*args))
> +        assert offset == 1024
> +        assert entries == [ 1024, self.EXTENT_ZERO ]
> --
> 2.27.0
>
Returning extent tuples looks much better than the C version in file plugin.

Nir

On Mon, Aug 10, 2020 at 07:28:30PM +0300, Nir Soffer
wrote:
> On Mon, Aug 10, 2020 at 1:51 PM Richard W.M. Jones
<rjones@redhat.com> wrote:
> > + def extents(h, count, offset, flags):
> > +   # return a list of (offset, length, type) tuples:
> > +   return [ (off1, len1, type1), (off2, len2, type2), ... ]
> 
> Better to accept an iterable of tuples. This allows the plugin to do:
> 
>     def extents(h, count, offset, flags):
>         for extent in client.extents():
>             if extent.offset > offset:
>                 return
>             yield extent.start, extent.length, type
> 
> The logic is just an example to illustrate the benefit of using
> generators. The plugin does not have to build a list of extent,
> and nbdkit also may not need this list. This may have performance
> benefits, and may make the code nicer.
Yes, I think we should do this, and I believe the change is
backwards compatible too (just as well because I just pushed
this into RHEL ...)

...
> > +    /* We expect a list of extents to be returned.  Each extent is a
> > +     * tuple (offset, length, type).
> > +     */
> > +    if (!PyList_Check (r)) {
> 
> This will work only with the list. We want to check if we got a type
> we can iterated on. I don't know the APi needed but I'm sure this
is
> supported by the C API.
I wonder how it works w.r.t. threads ...  Does it use another thread
or does it just have some kind of suspension which is evaluated when
requested?

This seems to be the API so I'll have a play around with it:

https://docs.python.org/3/c-api/iter.html
> Returning extent tuples looks much better than the C version in file
plugin.
Certainly easier to use :-/

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-p2v converts physical machines to virtual machines.  Boot with a
live CD or over the network (PXE) and turn machines into KVM guests.
http://libguestfs.org/virt-v2v