Note: This is a beta release of Red Hat Bugzilla 5.0. The data contained within is a snapshot of the live data so any changes you make will not be reflected in the production Bugzilla. Also email is disabled so feel free to test any aspect of the site that you want. File any problems you find or give feedback here.
Bug 1687114 - blockCopy fail with bogus error: "TypeError: block params must be a dictionary"
Summary: blockCopy fail with bogus error: "TypeError: block params must be a dictionary"
Keywords:
Status: POST
Alias: None
Product: Fedora
Classification: Fedora
Component: libvirt-python
Version: 30
Hardware: Unspecified
OS: Unspecified
unspecified
urgent
Target Milestone: ---
Assignee: Daniel Berrange
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks: 1687712
TreeView+ depends on / blocked
 
Reported: 2019-03-10 00:24 UTC by Nir Soffer
Modified: 2019-04-06 21:57 UTC (History)
5 users (show)

Fixed In Version:
Doc Type: If docs needed, set a value
Doc Text:
Clone Of:
: 1687712 (view as bug list)
Environment:
Last Closed:


Attachments (Terms of Use)
Proposed patch (deleted)
2019-03-10 18:01 UTC, Nir Soffer
no flags Details

Description Nir Soffer 2019-03-10 00:24:39 UTC
Description of problem:

When blockCopy is called without specifying the params argument, the call
fails with:

    TypeError: block params must be a dictionary

blockCopy is defined as:

    def blockCopy(self, disk, destxml, params=None, flags=0):
        """Copy the guest-visible contents of a disk image to a new file described by destxml """
        ret = libvirtmod.virDomainBlockCopy(self._o, disk, destxml, params, flags)
        if ret == -1: raise libvirtError ('virDomainBlockCopy() failed', dom=self)
        return ret

So when calling it without the params kwarg, params is None.

Vdsm calls blockCopy like this:

    def _startDriveReplication(self, drive):
        destxml = xmlutils.tostring(drive.getReplicaXML())
        self.log.debug("Replicating drive %s to %s", drive.name, destxml)

        flags = (libvirt.VIR_DOMAIN_BLOCK_COPY_SHALLOW |
                 libvirt.VIR_DOMAIN_BLOCK_COPY_REUSE_EXT |
                 libvirt.VIR_DOMAIN_BLOCK_COPY_TRANSIENT_JOB)

        self._dom.blockCopy(drive.name, destxml, flags=flags)

Version-Release number of selected component (if applicable):

$ rpm -q python2-libvirt
python2-libvirt-5.0.0-2.fc28.x86_64

$ rpm -q libvirt-daemon
libvirt-daemon-5.0.0-3.fc28.x86_64


How reproducible:
Always

Additional info:

I think this was broken by this patch:

commit 2b4bd07e0a2239cd9a4da49b9bd5229ac46e7875
Author: John Ferlan <jferlan@redhat.com>
Date:   Tue Nov 20 11:56:47 2018 -0500

    Add check for params, nparams being a dictionary
    
    If PyDict_Check fails, we should force an error rather than
    blindly continuing on.
    
    Signed-off-by: John Ferlan <jferlan@redhat.com>
    Reviewed-by: Pavel Hrdina <phrdina@redhat.com>

Adding this check:

    if (PyDict_Check(pyobj_dict)) {
        if (virPyDictToTypedParams(pyobj_dict, &params, &nparams,
                                   virPyDomainBlockCopyParams,
                                   VIR_N_ELEMENTS(virPyDomainBlockCopyParams)) < 0) {
            return NULL; 
        }     
    } else {
        PyErr_Format(PyExc_TypeError, "block params must be a dictionary");
        return NULL; 
    }

The correct check should allow dict or None.

Comment 1 Nir Soffer 2019-03-10 00:26:16 UTC
John, can you take a look?

Comment 2 Nir Soffer 2019-03-10 18:01:16 UTC
Created attachment 1542644 [details]
Proposed patch

I sent the patch to libvirt mailing list, but for some reason I don't see it
in the archive:
https://www.redhat.com/archives/libvir-list/2019-March/date.html

Comment 3 John Ferlan 2019-03-11 13:12:07 UTC
I see Daniel has reviewed your patch already.

Still to "answer" your question - the referenced commit was part of other changes to more consistently handle the arguments - something done as a result of code review comments from my initial posting of the series of patches (see 3/3 from the v1 https://www.redhat.com/archives/libvir-list/2018-November/msg00772.html). Seems I must have missed that "...|O..." means optional object argument, sorry about that.

Comment 4 Peter Krempa 2019-03-12 08:17:06 UTC
Broken in:
$ git desc --contains 2b4bd07e0a22
v4.10.0~4
Will be fixed in 5.2.0

commit 5d6228d4171c0a9d1299ad742dc878092287675a (HEAD -> master, origin/master, origin/HEAD)
Author: Nir Soffer <nirsof@gmail.com>
Date:   Mon Mar 11 23:34:50 2019 +0200

    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. Add a check for Py_None, so we accept either a dict or
    None and fail with TypeError with anything else.
    
    Resolves: https://bugzilla.redhat.com/1687114
    
    Signed-off-by: Nir Soffer <nsoffer@redhat.com>

Comment 5 Cole Robinson 2019-04-06 21:57:53 UTC
libvirt-python-5.2.0-1.fc31 is in rawhide now, but we can backport to fedora 30 too


Note You need to log in before you can comment on or make changes to this bug.