[PATCH nbdkit 1/2] tests/test-python-plugin.py: Allow test to use large disks

[PATCH libnbd CI] ci: Drop Fedora...

[v2v PATCH] output_rhv: restrict...

Richard W.M. Jones

Tuesday, 21 December 2021 Tue, 21 Dec '21

3:21 p.m.

The Python test harness uses a plugin which always created a fully allocated disk backed by an in-memory bytearray. This prevented us from testing very large disks (since we could run out of memory easily). Add a feature allowing large all-zero disks to be tested. The disk is not allocated and non-zero writes will fail, but everything else works. --- tests/test-python-plugin.py | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-) diff --git a/tests/test-python-plugin.py b/tests/test-python-plugin.py index 70d545db..a30b7f64 100644 --- a/tests/test-python-plugin.py +++ b/tests/test-python-plugin.py @@ -51,13 +51,17 @@ def config_complete(): def open(readonly): + if cfg.get('no_disk', False): + disk = None + else: + disk = bytearray(cfg.get('size', 0)) return { - 'disk': bytearray(cfg.get('size', 0)) + 'disk': disk } def get_size(h): - return len(h['disk']) + return cfg.get('size', 0) def is_rotational(h): @@ -123,6 +127,7 @@ def pwrite(h, buf, offset, flags): actual_fua = bool(flags & nbdkit.FLAG_FUA) assert expect_fua == actual_fua end = offset + len(buf) + assert h['disk'] is not None h['disk'][offset:end] = buf @@ -134,7 +139,8 @@ def trim(h, count, offset, flags): expect_fua = cfg.get('trim_expect_fua', False) actual_fua = bool(flags & nbdkit.FLAG_FUA) assert expect_fua == actual_fua - h['disk'][offset:offset+count] = bytearray(count) + if h['disk'] is not None: + h['disk'][offset:offset+count] = bytearray(count) def zero(h, count, offset, flags): @@ -147,7 +153,8 @@ def zero(h, count, offset, flags): expect_fast_zero = cfg.get('zero_expect_fast_zero', False) actual_fast_zero = bool(flags & nbdkit.FLAG_FAST_ZERO) assert expect_fast_zero == actual_fast_zero - h['disk'][offset:offset+count] = bytearray(count) + if h['disk'] is not None: + h['disk'][offset:offset+count] = bytearray(count) def cache(h, count, offset, flags): -- 2.32.0

Show replies by date

Richard W.M. Jones

Tuesday, 21 December Tue, 21 Dec

3:21 p.m.

New subject: [PATCH nbdkit 2/2] python: More precise Python parameter passing

Nir Soffer's commit 715e9c2386 ("plugins/python: Fix extents() count format string") fixed the signedness of the count parameter of the extents call. Following this commit I looked at a few other places where we pass parameters from C to Python using the PyObject_Call* family of functions, and fixed a few things: - Consistently use "I" (unsigned int) for passing count parameters, where we previously used "i" (signed int). This probably doesn't affect pread, but might affect zero, trim etc. - Consistently use a boolean for the readonly and is_tls parameters, where we previously used a mix of booleans or "i" (signed int). --- plugins/python/plugin.c | 23 +++++++++++++++-------- tests/test_python.py | 7 +++++++ 2 files changed, 22 insertions(+), 8 deletions(-) diff --git a/plugins/python/plugin.c b/plugins/python/plugin.c index 366619f9..aa9f35c8 100644 --- a/plugins/python/plugin.c +++ b/plugins/python/plugin.c @@ -330,7 +330,10 @@ py_list_exports (int readonly, int is_tls, struct nbdkit_exports *exports) PyErr_Clear (); - r = PyObject_CallFunction (fn, "ii", readonly, is_tls); + r = PyObject_CallFunctionObjArgs (fn, + readonly ? Py_True : Py_False, + is_tls ? Py_True : Py_False, + NULL); Py_DECREF (fn); if (check_python_failure ("list_exports") == -1) return -1; @@ -394,7 +397,10 @@ py_default_export (int readonly, int is_tls) PyErr_Clear (); - r = PyObject_CallFunction (fn, "ii", readonly, is_tls); + r = PyObject_CallFunctionObjArgs (fn, + readonly ? Py_True : Py_False, + is_tls ? Py_True : Py_False, + NULL); Py_DECREF (fn); if (check_python_failure ("default_export") == -1) return NULL; @@ -567,7 +573,7 @@ py_pread (void *handle, void *buf, uint32_t count, uint64_t offset, switch (py_api_version) { case 1: - r = PyObject_CallFunction (fn, "OiL", h->py_h, count, offset); + r = PyObject_CallFunction (fn, "OIL", h->py_h, count, offset); break; case 2: r = PyObject_CallFunction (fn, "ONLI", h->py_h, @@ -694,10 +700,10 @@ py_trim (void *handle, uint32_t count, uint64_t offset, uint32_t flags) switch (py_api_version) { case 1: - r = PyObject_CallFunction (fn, "OiL", h->py_h, count, offset); + r = PyObject_CallFunction (fn, "OIL", h->py_h, count, offset); break; case 2: - r = PyObject_CallFunction (fn, "OiLI", h->py_h, count, offset, flags); + r = PyObject_CallFunction (fn, "OILI", h->py_h, count, offset, flags); break; default: abort (); } @@ -729,13 +735,13 @@ py_zero (void *handle, uint32_t count, uint64_t offset, uint32_t flags) switch (py_api_version) { case 1: { int may_trim = flags & NBDKIT_FLAG_MAY_TRIM; - r = PyObject_CallFunction (fn, "OiLO", + r = PyObject_CallFunction (fn, "OILO", h->py_h, count, offset, may_trim ? Py_True : Py_False); break; } case 2: - r = PyObject_CallFunction (fn, "OiLI", h->py_h, count, offset, flags); + r = PyObject_CallFunction (fn, "OILI", h->py_h, count, offset, flags); break; default: abort (); } @@ -775,7 +781,8 @@ py_cache (void *handle, uint32_t count, uint64_t offset, uint32_t flags) switch (py_api_version) { case 1: case 2: - r = PyObject_CallFunction (fn, "OiLI", h->py_h, count, offset, flags, NULL); + r = PyObject_CallFunction (fn, "OILI", + h->py_h, count, offset, flags, NULL); break; default: abort (); } diff --git a/tests/test_python.py b/tests/test_python.py index 506edea3..d89d5533 100755 --- a/tests/test_python.py +++ b/tests/test_python.py @@ -231,6 +231,13 @@ class Test(unittest.TestCase): "zero_expect_fast_zero": True}) self.h.zero(512, 0, nbd.CMD_FLAG_NO_HOLE | nbd.CMD_FLAG_FAST_ZERO) + def test_zero_large(self): + """Test large zero.""" + self.connect({"size": 0x10_0000_0000, + "can_zero": True, + "no_disk": True}) + self.h.zero(0xf000_0000, 0, nbd.CMD_FLAG_NO_HOLE) + def test_cache(self): """Test cache.""" self.connect({"size": 512, "can_cache": "native"}) -- 2.32.0

Nir Soffer

4:03 p.m.

New subject: [PATCH nbdkit 2/2] python: More precise Python parameter passing

On Tue, Dec 21, 2021 at 11:21 PM Richard W.M. Jones <rjones(a)redhat.com> wrote:

...

This is not needed. I think the accepted way to pass boolean values to python is "i". Internally python True and False are 1 and 0, and you can use them as such:

...

>> True * 7

7 Python code is not expected to use: if readonly is True: or even: if readonly == True: which works but is not idiomatic, but: if readonly: Which works for anything that is falsy.

...

--- plugins/python/plugin.c | 23 +++++++++++++++-------- tests/test_python.py | 7 +++++++ 2 files changed, 22 insertions(+), 8 deletions(-) diff --git a/plugins/python/plugin.c b/plugins/python/plugin.c index 366619f9..aa9f35c8 100644 --- a/plugins/python/plugin.c +++ b/plugins/python/plugin.c @@ -330,7 +330,10 @@ py_list_exports (int readonly, int is_tls, struct nbdkit_exports *exports) PyErr_Clear (); - r = PyObject_CallFunction (fn, "ii", readonly, is_tls); + r = PyObject_CallFunctionObjArgs (fn, + readonly ? Py_True : Py_False, + is_tls ? Py_True : Py_False, + NULL);

However PyObject_CallFunctionObjArgs is more efficient than PyObject_CallFunction so this looks good.

...

Py_DECREF (fn); if (check_python_failure ("list_exports") == -1) return -1; @@ -394,7 +397,10 @@ py_default_export (int readonly, int is_tls) PyErr_Clear (); - r = PyObject_CallFunction (fn, "ii", readonly, is_tls); + r = PyObject_CallFunctionObjArgs (fn, + readonly ? Py_True : Py_False, + is_tls ? Py_True : Py_False, + NULL); Py_DECREF (fn); if (check_python_failure ("default_export") == -1) return NULL; @@ -567,7 +573,7 @@ py_pread (void *handle, void *buf, uint32_t count, uint64_t offset, switch (py_api_version) { case 1: - r = PyObject_CallFunction (fn, "OiL", h->py_h, count, offset); + r = PyObject_CallFunction (fn, "OIL", h->py_h, count, offset); break; case 2: r = PyObject_CallFunction (fn, "ONLI", h->py_h, @@ -694,10 +700,10 @@ py_trim (void *handle, uint32_t count, uint64_t offset, uint32_t flags) switch (py_api_version) { case 1: - r = PyObject_CallFunction (fn, "OiL", h->py_h, count, offset); + r = PyObject_CallFunction (fn, "OIL", h->py_h, count, offset); break; case 2: - r = PyObject_CallFunction (fn, "OiLI", h->py_h, count, offset, flags); + r = PyObject_CallFunction (fn, "OILI", h->py_h, count, offset, flags); break; default: abort (); } @@ -729,13 +735,13 @@ py_zero (void *handle, uint32_t count, uint64_t offset, uint32_t flags) switch (py_api_version) { case 1: { int may_trim = flags & NBDKIT_FLAG_MAY_TRIM; - r = PyObject_CallFunction (fn, "OiLO", + r = PyObject_CallFunction (fn, "OILO", h->py_h, count, offset, may_trim ? Py_True : Py_False); break; } case 2: - r = PyObject_CallFunction (fn, "OiLI", h->py_h, count, offset, flags); + r = PyObject_CallFunction (fn, "OILI", h->py_h, count, offset, flags); break; default: abort (); } @@ -775,7 +781,8 @@ py_cache (void *handle, uint32_t count, uint64_t offset, uint32_t flags) switch (py_api_version) { case 1: case 2: - r = PyObject_CallFunction (fn, "OiLI", h->py_h, count, offset, flags, NULL); + r = PyObject_CallFunction (fn, "OILI", + h->py_h, count, offset, flags, NULL); break; default: abort (); } diff --git a/tests/test_python.py b/tests/test_python.py index 506edea3..d89d5533 100755 --- a/tests/test_python.py +++ b/tests/test_python.py @@ -231,6 +231,13 @@ class Test(unittest.TestCase): "zero_expect_fast_zero": True}) self.h.zero(512, 0, nbd.CMD_FLAG_NO_HOLE | nbd.CMD_FLAG_FAST_ZERO) + def test_zero_large(self): + """Test large zero.""" + self.connect({"size": 0x10_0000_0000, + "can_zero": True, + "no_disk": True}) + self.h.zero(0xf000_0000, 0, nbd.CMD_FLAG_NO_HOLE)

0x10_0000_0000 is very confusing, why not 64 * GiB? For the zero count, should we use 2**32 - 1 to make sure zero works with the maximum count?

...

+ def test_cache(self): """Test cache.""" self.connect({"size": 512, "can_cache": "native"}) -- 2.32.0

Otherwise it looks good. Nir

Richard W.M. Jones

4:24 p.m.

New subject: [PATCH nbdkit 2/2] python: More precise Python parameter passing

On Wed, Dec 22, 2021 at 12:03:44AM +0200, Nir Soffer wrote:

...

This is not needed. I think the accepted way to pass boolean values to python is "i". Internally python True and False are 1 and 0, and you can use them as such:

The documentation suggests "p" for bools: https://docs.python.org/3/c-api/arg.html#c.Py_BuildValue But that's not relevant for the patch because: ...

...

However PyObject_CallFunctionObjArgs is more efficient than PyObject_CallFunction so this looks good.

Indeed.

...

0x10_0000_0000 is very confusing, why not 64 * GiB? For the zero count, should we use 2**32 - 1 to make sure zero works with the maximum count?

Is "GiB" defined somewhere?

...

>> print(GiB)

Traceback (most recent call last): File "<stdin>", line 1, in <module> NameError: name 'GiB' is not defined Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-builder quickly builds VMs from scratch http://libguestfs.org/virt-builder.1.html

Nir Soffer

4:37 p.m.

New subject: [PATCH nbdkit 2/2] python: More precise Python parameter passing

On Wed, Dec 22, 2021 at 12:24 AM Richard W.M. Jones <rjones(a)redhat.com> wrote:

...

On Wed, Dec 22, 2021 at 12:03:44AM +0200, Nir Soffer wrote: > This is not needed. I think the accepted way to pass boolean values > to python is "i". Internally python True and False are 1 and 0, and > you can use them as such: The documentation suggests "p" for bools: https://docs.python.org/3/c-api/arg.html#c.Py_BuildValue

According to the docs, "p" is only for parsing arguments: https://docs.python.org/3.6/c-api/arg.html#parsing-arguments And for building value there is no such option: https://docs.python.org/3.6/c-api/arg.html#building-values

...

But that's not relevant for the patch because: ... > However PyObject_CallFunctionObjArgs is more efficient than > PyObject_CallFunction > so this looks good. Indeed. > 0x10_0000_0000 is very confusing, why not 64 * GiB? > > For the zero count, should we use 2**32 - 1 to make sure zero > works with the maximum count? Is "GiB" defined somewhere? >>> print(GiB) Traceback (most recent call last): File "<stdin>", line 1, in <module> NameError: name 'GiB' is not defined

We use this in vdsm: https://github.com/oVirt/vdsm/blob/master/lib/vdsm/common/units.py qemu also have: https://github.com/qemu/qemu/blob/master/include/qemu/units.h

Nir Soffer

4:21 p.m.

On Tue, Dec 21, 2021 at 11:21 PM Richard W.M. Jones <rjones(a)redhat.com> wrote:

...

get() default is None, so the idiomatic way is: if cfg.get("no_disk"):

...

+ disk = None + else: + disk = bytearray(cfg.get('size', 0)) return { - 'disk': bytearray(cfg.get('size', 0)) + 'disk': disk } def get_size(h): - return len(h['disk']) + return cfg.get('size', 0) def is_rotational(h): @@ -123,6 +127,7 @@ def pwrite(h, buf, offset, flags): actual_fua = bool(flags & nbdkit.FLAG_FUA) assert expect_fua == actual_fua end = offset + len(buf) + assert h['disk'] is not None h['disk'][offset:end] = buf @@ -134,7 +139,8 @@ def trim(h, count, offset, flags): expect_fua = cfg.get('trim_expect_fua', False) actual_fua = bool(flags & nbdkit.FLAG_FUA) assert expect_fua == actual_fua - h['disk'][offset:offset+count] = bytearray(count) + if h['disk'] is not None: + h['disk'][offset:offset+count] = bytearray(count) def zero(h, count, offset, flags): @@ -147,7 +153,8 @@ def zero(h, count, offset, flags): expect_fast_zero = cfg.get('zero_expect_fast_zero', False) actual_fast_zero = bool(flags & nbdkit.FLAG_FAST_ZERO) assert expect_fast_zero == actual_fast_zero - h['disk'][offset:offset+count] = bytearray(count) + if h['disk'] is not None: + h['disk'][offset:offset+count] = bytearray(count) def cache(h, count, offset, flags): -- 2.32.0

Using double negative: {"no_disk": False, ...} is little confusing, maybe the default should be: {"create_disk": True, ...} And test that don't want to create disk will use: {"create_disk": False, ...} Otherwise this looks useful. It would be help also if we can test extents argument, currently the plugin ignores the argument so we did not detect the issue with the wrong format string. Nir

Richard W.M. Jones

4:25 p.m.

On Wed, Dec 22, 2021 at 12:21:25AM +0200, Nir Soffer wrote:

...

On Tue, Dec 21, 2021 at 11:21 PM Richard W.M. Jones <rjones(a)redhat.com> wrote: > > The Python test harness uses a plugin which always created a fully > allocated disk backed by an in-memory bytearray. This prevented us > from testing very large disks (since we could run out of memory > easily). > > Add a feature allowing large all-zero disks to be tested. The disk is > not allocated and non-zero writes will fail, but everything else > works. > --- > tests/test-python-plugin.py | 15 +++++++++++---- > 1 file changed, 11 insertions(+), 4 deletions(-) > > diff --git a/tests/test-python-plugin.py b/tests/test-python-plugin.py > index 70d545db..a30b7f64 100644 > --- a/tests/test-python-plugin.py > +++ b/tests/test-python-plugin.py > @@ -51,13 +51,17 @@ def config_complete(): > > > def open(readonly): > + if cfg.get('no_disk', False): get() default is None, so the idiomatic way is: if cfg.get("no_disk"): > + disk = None > + else: > + disk = bytearray(cfg.get('size', 0)) > return { > - 'disk': bytearray(cfg.get('size', 0)) > + 'disk': disk > } > > > def get_size(h): > - return len(h['disk']) > + return cfg.get('size', 0) > > > def is_rotational(h): > @@ -123,6 +127,7 @@ def pwrite(h, buf, offset, flags): > actual_fua = bool(flags & nbdkit.FLAG_FUA) > assert expect_fua == actual_fua > end = offset + len(buf) > + assert h['disk'] is not None > h['disk'][offset:end] = buf > > > @@ -134,7 +139,8 @@ def trim(h, count, offset, flags): > expect_fua = cfg.get('trim_expect_fua', False) > actual_fua = bool(flags & nbdkit.FLAG_FUA) > assert expect_fua == actual_fua > - h['disk'][offset:offset+count] = bytearray(count) > + if h['disk'] is not None: > + h['disk'][offset:offset+count] = bytearray(count) > > > def zero(h, count, offset, flags): > @@ -147,7 +153,8 @@ def zero(h, count, offset, flags): > expect_fast_zero = cfg.get('zero_expect_fast_zero', False) > actual_fast_zero = bool(flags & nbdkit.FLAG_FAST_ZERO) > assert expect_fast_zero == actual_fast_zero > - h['disk'][offset:offset+count] = bytearray(count) > + if h['disk'] is not None: > + h['disk'][offset:offset+count] = bytearray(count) > > > def cache(h, count, offset, flags): > -- > 2.32.0 > Using double negative: {"no_disk": False, ...} is little confusing, maybe the default should be: {"create_disk": True, ...} And test that don't want to create disk will use: {"create_disk": False, ...} Otherwise this looks useful.

Yes, that's a good idea.

...

It would be help also if we can test extents argument, currently the plugin ignores the argument so we did not detect the issue with the wrong format string.

OK, I'll take a look. Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-df lists disk usage of guests without needing to install any software inside the virtual machine. Supports Linux and Windows. http://people.redhat.com/~rjones/virt-df/

Richard W.M. Jones

4:36 p.m.

On Tue, Dec 21, 2021 at 10:25:35PM +0000, Richard W.M. Jones wrote:

...

On Wed, Dec 22, 2021 at 12:21:25AM +0200, Nir Soffer wrote: > It would be help also if we can test extents argument, currently the > plugin ignores the argument so we did not detect the issue with the > wrong format string. OK, I'll take a look.

I see - the plugin function is defined just to return the extents sent in (for testing): def extents(h, count, offset, flags): return cfg.get('extents', []) This means we don't test the parameters. However to test those realistically I suppose we would really need to support sparse disks in this test plugin, which seems quite tricky. Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com libguestfs lets you edit virtual machines. Supports shell scripting, bindings from many languages. http://libguestfs.org

Nir Soffer

4:47 p.m.

On Wed, Dec 22, 2021 at 12:36 AM Richard W.M. Jones <rjones(a)redhat.com> wrote:

...

On Tue, Dec 21, 2021 at 10:25:35PM +0000, Richard W.M. Jones wrote: > On Wed, Dec 22, 2021 at 12:21:25AM +0200, Nir Soffer wrote: > > It would be help also if we can test extents argument, currently the > > plugin ignores the argument so we did not detect the issue with the > > wrong format string. > > OK, I'll take a look. I see - the plugin function is defined just to return the extents sent in (for testing): def extents(h, count, offset, flags): return cfg.get('extents', []) This means we don't test the parameters. However to test those realistically I suppose we would really need to support sparse disks in this test plugin, which seems quite tricky.

I think what can work is this: - pass the actual extents to the plugin For example empty sparse file: extents = [(0, 10 * GiB, nbdkit.EXTENT_ZERO)] Now when we request: extents(4*GB -1, 0, 0) The plugin can return: return [(0, count, nbdkit.EXTENT_ZERO)] The logic to return extents from the existing list is a little tricky. I have similar code in a memory backend for imageio (in review): https://github.com/oVirt/ovirt-imageio/pull/7/commits/9068f84c96f1239d841... And this is also the code for getting extents from imageio, posted here: https://listman.redhat.com/archives/libguestfs/2021-December/msg00197.html

Richard W.M. Jones

Wednesday, 22 December Wed, 22 Dec

4:54 a.m.

On Wed, Dec 22, 2021 at 12:47:59AM +0200, Nir Soffer wrote:

...

On Wed, Dec 22, 2021 at 12:36 AM Richard W.M. Jones <rjones(a)redhat.com> wrote: > > On Tue, Dec 21, 2021 at 10:25:35PM +0000, Richard W.M. Jones wrote: > > On Wed, Dec 22, 2021 at 12:21:25AM +0200, Nir Soffer wrote: > > > It would be help also if we can test extents argument, currently the > > > plugin ignores the argument so we did not detect the issue with the > > > wrong format string. > > > > OK, I'll take a look. > > I see - the plugin function is defined just to return the extents sent > in (for testing): > > def extents(h, count, offset, flags): > return cfg.get('extents', []) > > This means we don't test the parameters. However to test those > realistically I suppose we would really need to support sparse disks > in this test plugin, which seems quite tricky. I think what can work is this: - pass the actual extents to the plugin For example empty sparse file: extents = [(0, 10 * GiB, nbdkit.EXTENT_ZERO)] Now when we request: extents(4*GB -1, 0, 0) The plugin can return: return [(0, count, nbdkit.EXTENT_ZERO)]

Isn't this what the code above already does?

...

The logic to return extents from the existing list is a little tricky. I have similar code in a memory backend for imageio (in review): https://github.com/oVirt/ovirt-imageio/pull/7/commits/9068f84c96f1239d841... And this is also the code for getting extents from imageio, posted here: https://listman.redhat.com/archives/libguestfs/2021-December/msg00197.html

I'm confused about this bit. Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-builder quickly builds VMs from scratch http://libguestfs.org/virt-builder.1.html

1096

days inactive

1097

days old

guestfs@lists.libguestfs.org

Manage subscription

9 comments

2 participants

tags (0)

participants (2)

Nir Soffer
Richard W.M. Jones

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

2012

2011

2010

2009

[PATCH nbdkit 1/2] tests/test-python-plugin.py: Allow test to use large disks