[libvirt] [PATCH python] Fix handling of optional params in blockCopy()
Commit 2b4bd07e0a22 (Add check for params, nparams being a dictionary) changed the way the optional params argument is treated. If libvirt.virDomain.blockCopy() is called without specifying params, params is None, and the call will fail with: TypeError: block params must be a dictionary This is wrong as params is defined as kwarg, breaking existing libvirt users like oVirt. Remove the check, restoring the previous behaviour. Resolves: https://bugzilla.redhat.com/1687114 Signed-off-by: Nir Soffer <nsoffer@redhat.com> --- libvirt-override.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/libvirt-override.c b/libvirt-override.c index 857c018..127d71c 100644 --- a/libvirt-override.c +++ b/libvirt-override.c @@ -8829,19 +8829,17 @@ libvirt_virDomainBlockCopy(PyObject *self ATTRIBUTE_UNUSED, if (!PyArg_ParseTuple(args, (char *) "Ozz|OI:virDomainBlockCopy", &pyobj_dom, &disk, &destxml, &pyobj_dict, &flags)) return NULL; + /* Note: params is optional in libvirt.py */ if (PyDict_Check(pyobj_dict)) { if (virPyDictToTypedParams(pyobj_dict, ¶ms, &nparams, virPyDomainBlockCopyParams, VIR_N_ELEMENTS(virPyDomainBlockCopyParams)) < 0) { return NULL; } - } else { - PyErr_Format(PyExc_TypeError, "block params must be a dictionary"); - return NULL; } dom = (virDomainPtr) PyvirDomain_Get(pyobj_dom); LIBVIRT_BEGIN_ALLOW_THREADS; -- 2.17.2
On Sun, Mar 10, 2019 at 03:03:25AM +0200, Nir Soffer wrote:
Commit 2b4bd07e0a22 (Add check for params, nparams being a dictionary) changed the way the optional params argument is treated. If libvirt.virDomain.blockCopy() is called without specifying params, params is None, and the call will fail with:
TypeError: block params must be a dictionary
This is wrong as params is defined as kwarg, breaking existing libvirt users like oVirt. Remove the check, restoring the previous behaviour.
Resolves: https://bugzilla.redhat.com/1687114
Signed-off-by: Nir Soffer <nsoffer@redhat.com> --- libvirt-override.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/libvirt-override.c b/libvirt-override.c index 857c018..127d71c 100644 --- a/libvirt-override.c +++ b/libvirt-override.c @@ -8829,19 +8829,17 @@ libvirt_virDomainBlockCopy(PyObject *self ATTRIBUTE_UNUSED,
if (!PyArg_ParseTuple(args, (char *) "Ozz|OI:virDomainBlockCopy", &pyobj_dom, &disk, &destxml, &pyobj_dict, &flags)) return NULL;
+ /* Note: params is optional in libvirt.py */ if (PyDict_Check(pyobj_dict)) { if (virPyDictToTypedParams(pyobj_dict, ¶ms, &nparams, virPyDomainBlockCopyParams, VIR_N_ELEMENTS(virPyDomainBlockCopyParams)) < 0) { return NULL; } - } else { - PyErr_Format(PyExc_TypeError, "block params must be a dictionary"); - return NULL; }
Rather than removing this else branch, I think we should have an explict check for None first if (pyobj_dict == Py_None) { ...no op.. } else if (PyDict_Check(....)) { } else { .... error ... } Can you do that & validate that it works with oVirt. Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On Mon, Mar 11, 2019 at 12:00 PM Daniel P. Berrangé <berrange@redhat.com> wrote:
On Sun, Mar 10, 2019 at 03:03:25AM +0200, Nir Soffer wrote:
Commit 2b4bd07e0a22 (Add check for params, nparams being a dictionary) changed the way the optional params argument is treated. If libvirt.virDomain.blockCopy() is called without specifying params, params is None, and the call will fail with:
TypeError: block params must be a dictionary
This is wrong as params is defined as kwarg, breaking existing libvirt users like oVirt. Remove the check, restoring the previous behaviour.
Resolves: https://bugzilla.redhat.com/1687114
Signed-off-by: Nir Soffer <nsoffer@redhat.com> --- libvirt-override.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/libvirt-override.c b/libvirt-override.c index 857c018..127d71c 100644 --- a/libvirt-override.c +++ b/libvirt-override.c @@ -8829,19 +8829,17 @@ libvirt_virDomainBlockCopy(PyObject *self ATTRIBUTE_UNUSED,
if (!PyArg_ParseTuple(args, (char *) "Ozz|OI:virDomainBlockCopy", &pyobj_dom, &disk, &destxml, &pyobj_dict, &flags)) return NULL;
+ /* Note: params is optional in libvirt.py */ if (PyDict_Check(pyobj_dict)) { if (virPyDictToTypedParams(pyobj_dict, ¶ms, &nparams, virPyDomainBlockCopyParams,
VIR_N_ELEMENTS(virPyDomainBlockCopyParams)) < 0) {
return NULL; } - } else { - PyErr_Format(PyExc_TypeError, "block params must be a
dictionary");
- return NULL; }
Rather than removing this else branch, I think we should have an explict check for None first
if (pyobj_dict == Py_None) { ...no op.. } else if (PyDict_Check(....)) { } else { .... error ... }
Can you do that & validate that it works with oVirt.
Sure, I'll send v2. Nir
participants (3)
-
Daniel P. Berrangé -
Nir Soffer -
Nir Soffer