Nir Soffer
2021-Dec-21 22:03 UTC
[Libguestfs] [PATCH nbdkit 2/2] python: More precise Python parameter passing
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 * 77 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
2021-Dec-21 22:24 UTC
[Libguestfs] [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