Libguestfs - Dec 2021 - [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

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