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

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

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

On Tue, Dec 21, 2021 at 11:21 PM Richard W.M. Jones <rjones at 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.

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