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

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).

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

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