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