[Libguestfs] [PATCH nbdkit] python: Implement can_extents + extents.
Nir Soffer
nsoffer at redhat.com
Mon Aug 10 16:28:30 UTC 2020
On Mon, Aug 10, 2020 at 1:51 PM Richard W.M. Jones <rjones at 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
More information about the Libguestfs
mailing list