Bug 1687712

+++ This bug was initially created as a clone of Bug #1687114 +++

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>
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>
    Reviewed-by: Pavel Hrdina <phrdina>

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.

--- Additional comment from Peter Krempa on 2019-03-12 09:17:06 CET ---

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>
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>