On Mon, Nov 26, 2018 at 05:12:06PM +0100, Philipp Hahn wrote:

Hello,

Am 26.11.18 um 16:28 schrieb Michal Privoznik:

...

On 11/21/18 8:17 AM, Philipp Hahn wrote:

...

while working on the Python type annotations for the Python libvirt binding I noticed the following code in libvirt-override-virDomainSnapshot.py:

...

def listAllChildren(self, flags=0): """List all child snapshots and returns a list of snapshot objects"""

...

...

raise libvirtError("..., conn=self)

"self" is an instance of virDomainSnapshot here.

I found two similar cases where "conn" was not a "virConnect" instance in listAllSnapshots() and listAllVolumes().

Compare that with the declaration of libvirtError in libvirt-override.py:

...

class libvirtError(Exception): def __init__(self, defmsg, conn=None, dom=None, net=None, pool=None, vol=None):

Looking further at the implementation of that method only "defmsg" is used; all other references are not accessed and never stored in an instance variable.

Should I add a new "snap" argument to libvirtError.__init__() or should we stop passing those references to the constructor altogether?

I think we should pass whatever object the error occurred on. Your patch #2 demonstrates this beautifully - with the current code conn will point to something that is not instance of virConnect rather than virDomain class.

The fields have been deprecated with git:f60dc0bc09f09c6817d6706a9edb1579a3e2b2b8 in "libvirt" and there is this warning in include/libvirt/virterror.h:

...

142 /** 143 * virError: 144 * 145 * A libvirt Error instance. 146 * 147 * The conn, dom and net fields should be used with extreme care. 148 * Reference counts are not incremented so the underlying objects 149 * may be deleted without notice after the error has been delivered. 150 */

The variables are not used anywhere in the Python code.

This is referring to the C level virError struct and is related to a historical mistake a very long time ago. Essentially it was created when we only have virDomain / virConnect objects. We mistakenly changed ABI when we added virNetwork object to it. At at the time we decided to stop adding extra objects to the C level virError struct. It also has the problem with reference counting mentioned here tough that isn't fatal if the C code is being very careful in how it uses the virError object. This deprecation at the C level, however, should not have any bearing on what we do at the Python level. We are passing in the python wrapped objects to libvirtError(), so don't suffer from the reference counting problems. So I don't see a compelling reason to remove these python object arguments. We should just fix the usage to actually pass in the correct objects & add parameters for the missing object types.

So I'm included to instead remove the arguments completely from the Python binding as well - see attached patch - that would break code where a user raises libvirtError() by itself outside the library binding code, but IMHO no-one should do that anyway.

...

...

Patch 2 and 3 might be applied to the current branch already, patch 1 currently depends on my other work.

ACK to them, do you want me to apply them now or should I wait (e.g. will you send them in some series?)

For now please hold off and let's discuss the removal first.

Philipp

From 973f5a5a8dc909a7ebca66cae8a2b87ae13431df Mon Sep 17 00:00:00 2001 Message-Id: <973f5a5a8dc909a7ebca66cae8a2b87ae13431df.1543248488.git.hahn@univention.de> From: Philipp Hahn <hahn@univention.de> Date: Wed, 21 Nov 2018 07:55:57 +0100 Subject: [PATCH libvirt-python] *: Remove legacy libvirtError arguments To: libvir-list@redhat.com

The fields have been deprecated with git:f60dc0bc09f09c6817d6706a9edb1579a3e2b2b8 The are not used anywhere in the Python code.

sed -i '/raise libvirtError/s/, \w\+=self//' *.py

Signed-off-by: Philipp Hahn <hahn@univention.de> --- generator.py | 38 ++++++++++++------------- libvirt-override-virConnect.py | 52 +++++++++++++++++------------------ libvirt-override-virDomain.py | 12 ++++---- libvirt-override-virDomainSnapshot.py | 2 +- libvirt-override-virStoragePool.py | 2 +- libvirt-override.py | 7 +---- 6 files changed, 54 insertions(+), 59 deletions(-)

diff --git a/generator.py b/generator.py index d854d48..cea9f79 100755 --- a/generator.py +++ b/generator.py @@ -1603,31 +1603,31 @@ def buildWrappers(module # type: str else: if classname == "virConnect": classes.write( - " if ret is None:raise libvirtError('%s() failed', conn=self)\n" % + " if ret is None:raise libvirtError('%s() failed')\n" % (name)) elif classname == "virDomain": classes.write( - " if ret is None:raise libvirtError('%s() failed', dom=self)\n" % + " if ret is None:raise libvirtError('%s() failed')\n" % (name)) elif classname == "virNetwork": classes.write( - " if ret is None:raise libvirtError('%s() failed', net=self)\n" % + " if ret is None:raise libvirtError('%s() failed')\n" % (name)) elif classname == "virInterface": classes.write( - " if ret is None:raise libvirtError('%s() failed', net=self)\n" % + " if ret is None:raise libvirtError('%s() failed')\n" % (name)) elif classname == "virStoragePool": classes.write( - " if ret is None:raise libvirtError('%s() failed', pool=self)\n" % + " if ret is None:raise libvirtError('%s() failed')\n" % (name)) elif classname == "virStorageVol": classes.write( - " if ret is None:raise libvirtError('%s() failed', vol=self)\n" % + " if ret is None:raise libvirtError('%s() failed')\n" % (name)) elif classname == "virDomainSnapshot": classes.write( - " if ret is None:raise libvirtError('%s() failed', dom=self._dom)\n" % + " if ret is None:raise libvirtError('%s() failed'._dom)\n" % (name)) else: classes.write( @@ -1657,27 +1657,27 @@ def buildWrappers(module # type: str test = functions_int_default_test if classname == "virConnect": classes.write((" if " + test + - ": raise libvirtError('%s() failed', conn=self)\n") % + ": raise libvirtError('%s() failed')\n") % ("ret", name)) elif classname == "virDomain": classes.write((" if " + test + - ": raise libvirtError('%s() failed', dom=self)\n") % + ": raise libvirtError('%s() failed')\n") % ("ret", name)) elif classname == "virNetwork": classes.write((" if " + test + - ": raise libvirtError('%s() failed', net=self)\n") % + ": raise libvirtError('%s() failed')\n") % ("ret", name)) elif classname == "virInterface": classes.write((" if " + test + - ": raise libvirtError('%s() failed', net=self)\n") % + ": raise libvirtError('%s() failed')\n") % ("ret", name)) elif classname == "virStoragePool": classes.write((" if " + test + - ": raise libvirtError('%s() failed', pool=self)\n") % + ": raise libvirtError('%s() failed')\n") % ("ret", name)) elif classname == "virStorageVol": classes.write((" if " + test + - ": raise libvirtError('%s() failed', vol=self)\n") % + ": raise libvirtError('%s() failed')\n") % ("ret", name)) else: classes.write((" if " + test + @@ -1690,27 +1690,27 @@ def buildWrappers(module # type: str if name not in functions_noexcept: if classname == "virConnect": classes.write((" if %s is None" + - ": raise libvirtError('%s() failed', conn=self)\n") % + ": raise libvirtError('%s() failed')\n") % ("ret", name)) elif classname == "virDomain": classes.write((" if %s is None" + - ": raise libvirtError('%s() failed', dom=self)\n") % + ": raise libvirtError('%s() failed')\n") % ("ret", name)) elif classname == "virNetwork": classes.write((" if %s is None" + - ": raise libvirtError('%s() failed', net=self)\n") % + ": raise libvirtError('%s() failed')\n") % ("ret", name)) elif classname == "virInterface": classes.write((" if %s is None" + - ": raise libvirtError('%s() failed', net=self)\n") % + ": raise libvirtError('%s() failed')\n") % ("ret", name)) elif classname == "virStoragePool": classes.write((" if %s is None" + - ": raise libvirtError('%s() failed', pool=self)\n") % + ": raise libvirtError('%s() failed')\n") % ("ret", name)) elif classname == "virStorageVol": classes.write((" if %s is None" + - ": raise libvirtError('%s() failed', vol=self)\n") % + ": raise libvirtError('%s() failed')\n") % ("ret", name)) else: classes.write((" if %s is None" + diff --git a/libvirt-override-virConnect.py b/libvirt-override-virConnect.py index e005a46..345024b 100644 --- a/libvirt-override-virConnect.py +++ b/libvirt-override-virConnect.py @@ -32,7 +32,7 @@ del self.domainEventCallbacks ret = libvirtmod.virConnectDomainEventDeregister(self._o, self) if ret == -1: - raise libvirtError('virConnectDomainEventDeregister() failed', conn=self) + raise libvirtError('virConnectDomainEventDeregister() failed') except AttributeError: pass

@@ -48,7 +48,7 @@ self.domainEventCallbacks = {cb: opaque} # type: Dict[Callable[[virConnect, virDomain, int, int, _T], int], _T] ret = libvirtmod.virConnectDomainEventRegister(self._o, self) if ret == -1: - raise libvirtError('virConnectDomainEventRegister() failed', conn=self) + raise libvirtError('virConnectDomainEventRegister() failed')

def _dispatchDomainEventCallbacks(self, dom, # type: virDomain @@ -397,7 +397,7 @@ try: ret = libvirtmod.virConnectDomainEventDeregisterAny(self._o, callbackID) if ret == -1: - raise libvirtError('virConnectDomainEventDeregisterAny() failed', conn=self) + raise libvirtError('virConnectDomainEventDeregisterAny() failed') del self.domainEventCallbackID[callbackID] except AttributeError: pass @@ -424,7 +424,7 @@ try: ret = libvirtmod.virConnectNetworkEventDeregisterAny(self._o, callbackID) if ret == -1: - raise libvirtError('virConnectNetworkEventDeregisterAny() failed', conn=self) + raise libvirtError('virConnectNetworkEventDeregisterAny() failed') del self.networkEventCallbackID[callbackID] except AttributeError: pass @@ -445,7 +445,7 @@ else: ret = libvirtmod.virConnectNetworkEventRegisterAny(self._o, net._o, eventID, cbData) if ret == -1: - raise libvirtError('virConnectNetworkEventRegisterAny() failed', conn=self) + raise libvirtError('virConnectNetworkEventRegisterAny() failed') self.networkEventCallbackID[ret] = opaque return ret

@@ -465,7 +465,7 @@ else: ret = libvirtmod.virConnectDomainEventRegisterAny(self._o, dom._o, eventID, cbData) if ret == -1: - raise libvirtError('virConnectDomainEventRegisterAny() failed', conn=self) + raise libvirtError('virConnectDomainEventRegisterAny() failed') self.domainEventCallbackID[ret] = opaque return ret

@@ -505,7 +505,7 @@ try: ret = libvirtmod.virConnectStoragePoolEventDeregisterAny(self._o, callbackID) if ret == -1: - raise libvirtError('virConnectStoragePoolEventDeregisterAny() failed', conn=self) + raise libvirtError('virConnectStoragePoolEventDeregisterAny() failed') del self.storagePoolEventCallbackID[callbackID] except AttributeError: pass @@ -526,7 +526,7 @@ else: ret = libvirtmod.virConnectStoragePoolEventRegisterAny(self._o, pool._o, eventID, cbData) if ret == -1: - raise libvirtError('virConnectStoragePoolEventRegisterAny() failed', conn=self) + raise libvirtError('virConnectStoragePoolEventRegisterAny() failed') self.storagePoolEventCallbackID[ret] = opaque return ret

@@ -566,7 +566,7 @@ try: ret = libvirtmod.virConnectNodeDeviceEventDeregisterAny(self._o, callbackID) if ret == -1: - raise libvirtError('virConnectNodeDeviceEventDeregisterAny() failed', conn=self) + raise libvirtError('virConnectNodeDeviceEventDeregisterAny() failed') del self.nodeDeviceEventCallbackID[callbackID] except AttributeError: pass @@ -587,7 +587,7 @@ else: ret = libvirtmod.virConnectNodeDeviceEventRegisterAny(self._o, dev._o, eventID, cbData) if ret == -1: - raise libvirtError('virConnectNodeDeviceEventRegisterAny() failed', conn=self) + raise libvirtError('virConnectNodeDeviceEventRegisterAny() failed') self.nodeDeviceEventCallbackID[ret] = opaque return ret

@@ -625,7 +625,7 @@ try: ret = libvirtmod.virConnectSecretEventDeregisterAny(self._o, callbackID) if ret == -1: - raise libvirtError('virConnectSecretEventDeregisterAny() failed', conn=self) + raise libvirtError('virConnectSecretEventDeregisterAny() failed') del self.secretEventCallbackID[callbackID] except AttributeError: pass @@ -646,7 +646,7 @@ else: ret = libvirtmod.virConnectSecretEventRegisterAny(self._o, secret._o, eventID, cbData) if ret == -1: - raise libvirtError('virConnectSecretEventRegisterAny() failed', conn=self) + raise libvirtError('virConnectSecretEventRegisterAny() failed') self.secretEventCallbackID[ret] = opaque return ret

@@ -656,7 +656,7 @@ """List all domains and returns a list of domain objects""" ret = libvirtmod.virConnectListAllDomains(self._o, flags) if ret is None: - raise libvirtError("virConnectListAllDomains() failed", conn=self) + raise libvirtError("virConnectListAllDomains() failed")

retlist = list() for domptr in ret: @@ -670,7 +670,7 @@ """Returns a list of storage pool objects""" ret = libvirtmod.virConnectListAllStoragePools(self._o, flags) if ret is None: - raise libvirtError("virConnectListAllStoragePools() failed", conn=self) + raise libvirtError("virConnectListAllStoragePools() failed")

retlist = list() for poolptr in ret: @@ -684,7 +684,7 @@ """Returns a list of network objects""" ret = libvirtmod.virConnectListAllNetworks(self._o, flags) if ret is None: - raise libvirtError("virConnectListAllNetworks() failed", conn=self) + raise libvirtError("virConnectListAllNetworks() failed")

retlist = list() for netptr in ret: @@ -698,7 +698,7 @@ """Returns a list of interface objects""" ret = libvirtmod.virConnectListAllInterfaces(self._o, flags) if ret is None: - raise libvirtError("virConnectListAllInterfaces() failed", conn=self) + raise libvirtError("virConnectListAllInterfaces() failed")

retlist = list() for ifaceptr in ret: @@ -712,7 +712,7 @@ """Returns a list of host node device objects""" ret = libvirtmod.virConnectListAllNodeDevices(self._o, flags) if ret is None: - raise libvirtError("virConnectListAllNodeDevices() failed", conn=self) + raise libvirtError("virConnectListAllNodeDevices() failed")

retlist = list() for devptr in ret: @@ -726,7 +726,7 @@ """Returns a list of network filter objects""" ret = libvirtmod.virConnectListAllNWFilters(self._o, flags) if ret is None: - raise libvirtError("virConnectListAllNWFilters() failed", conn=self) + raise libvirtError("virConnectListAllNWFilters() failed")

retlist = list() for filter_ptr in ret: @@ -740,7 +740,7 @@ """Returns a list of network filter binding objects""" ret = libvirtmod.virConnectListAllNWFilterBindings(self._o, flags) if ret is None: - raise libvirtError("virConnectListAllNWFilterBindings() failed", conn=self) + raise libvirtError("virConnectListAllNWFilterBindings() failed")

retlist = list() for filter_ptr in ret: @@ -754,7 +754,7 @@ """Returns a list of secret objects""" ret = libvirtmod.virConnectListAllSecrets(self._o, flags) if ret is None: - raise libvirtError("virConnectListAllSecrets() failed", conn=self) + raise libvirtError("virConnectListAllSecrets() failed")

retlist = list() for secret_ptr in ret: @@ -777,7 +777,7 @@ """Removes a close event callback""" ret = libvirtmod.virConnectUnregisterCloseCallback(self._o) if ret == -1: - raise libvirtError('virConnectUnregisterCloseCallback() failed', conn=self) + raise libvirtError('virConnectUnregisterCloseCallback() failed')

def registerCloseCallback(self, cb, # type: Callable @@ -788,7 +788,7 @@ cbData = {"cb": cb, "conn": self, "opaque": opaque} ret = libvirtmod.virConnectRegisterCloseCallback(self._o, cbData) if ret == -1: - raise libvirtError('virConnectRegisterCloseCallback() failed', conn=self) + raise libvirtError('virConnectRegisterCloseCallback() failed') return ret

def createXMLWithFiles(self, @@ -822,7 +822,7 @@ block attempts at migration, save-to-file, or snapshots. """ ret = libvirtmod.virDomainCreateXMLWithFiles(self._o, xmlDesc, files, flags) if ret is None: - raise libvirtError('virDomainCreateXMLWithFiles() failed', conn=self) + raise libvirtError('virDomainCreateXMLWithFiles() failed') __tmp = virDomain(self, _obj=ret) return __tmp

@@ -873,7 +873,7 @@ VIR_CONNECT_GET_ALL_DOMAINS_STATS_OTHER for all other states. """ ret = libvirtmod.virConnectGetAllDomainStats(self._o, stats, flags) if ret is None: - raise libvirtError("virConnectGetAllDomainStats() failed", conn=self) + raise libvirtError("virConnectGetAllDomainStats() failed")

retlist = list() for elem in ret: @@ -918,13 +918,13 @@ domlist = list() for dom in doms: if not isinstance(dom, virDomain): - raise libvirtError("domain list contains non-domain elements", conn=self) + raise libvirtError("domain list contains non-domain elements")

domlist.append(dom._o)

ret = libvirtmod.virDomainListGetStats(self._o, domlist, stats, flags) if ret is None: - raise libvirtError("virDomainListGetStats() failed", conn=self) + raise libvirtError("virDomainListGetStats() failed")

retlist = list() for elem in ret: diff --git a/libvirt-override-virDomain.py b/libvirt-override-virDomain.py index 590d5c0..69310eb 100644 --- a/libvirt-override-virDomain.py +++ b/libvirt-override-virDomain.py @@ -4,7 +4,7 @@ """List all snapshots and returns a list of snapshot objects""" ret = libvirtmod.virDomainListAllSnapshots(self._o, flags) if ret is None: - raise libvirtError("virDomainListAllSnapshots() failed", conn=self) + raise libvirtError("virDomainListAllSnapshots() failed")

retlist = list() for snapptr in ret: @@ -50,7 +50,7 @@ file for this domain is discarded, and the domain boots from scratch. """ ret = libvirtmod.virDomainCreateWithFiles(self._o, files, flags) if ret == -1: - raise libvirtError('virDomainCreateWithFiles() failed', dom=self) + raise libvirtError('virDomainCreateWithFiles() failed') return ret

def fsFreeze(self, @@ -60,7 +60,7 @@ """Freeze specified filesystems within the guest """ ret = libvirtmod.virDomainFSFreeze(self._o, mountpoints, flags) if ret == -1: - raise libvirtError('virDomainFSFreeze() failed', dom=self) + raise libvirtError('virDomainFSFreeze() failed') return ret

def fsThaw(self, @@ -70,7 +70,7 @@ """Thaw specified filesystems within the guest """ ret = libvirtmod.virDomainFSThaw(self._o, mountpoints, flags) if ret == -1: - raise libvirtError('virDomainFSThaw() failed', dom=self) + raise libvirtError('virDomainFSThaw() failed') return ret

def getTime(self, @@ -79,7 +79,7 @@ """Extract information about guest time """ ret = libvirtmod.virDomainGetTime(self._o, flags) if ret == None: - raise libvirtError('virDomainGetTime() failed', dom=self) + raise libvirtError('virDomainGetTime() failed') return ret

def setTime(self, @@ -90,5 +90,5 @@ 'seconds' field for seconds and 'nseconds' field for nanoseconds """ ret = libvirtmod.virDomainSetTime(self._o, time, flags) if ret == -1: - raise libvirtError('virDomainSetTime() failed', dom=self) + raise libvirtError('virDomainSetTime() failed') return ret diff --git a/libvirt-override-virDomainSnapshot.py b/libvirt-override-virDomainSnapshot.py index 722cee3..3555813 100644 --- a/libvirt-override-virDomainSnapshot.py +++ b/libvirt-override-virDomainSnapshot.py @@ -12,7 +12,7 @@ """List all child snapshots and returns a list of snapshot objects""" ret = libvirtmod.virDomainSnapshotListAllChildren(self._o, flags) if ret is None: - raise libvirtError("virDomainSnapshotListAllChildren() failed", conn=self) + raise libvirtError("virDomainSnapshotListAllChildren() failed")

retlist = list() for snapptr in ret: diff --git a/libvirt-override-virStoragePool.py b/libvirt-override-virStoragePool.py index 1f7c41e..5697ae0 100644 --- a/libvirt-override-virStoragePool.py +++ b/libvirt-override-virStoragePool.py @@ -4,7 +4,7 @@ """List all storage volumes and returns a list of storage volume objects""" ret = libvirtmod.virStoragePoolListAllVolumes(self._o, flags) if ret is None: - raise libvirtError("virStoragePoolListAllVolumes() failed", conn=self) + raise libvirtError("virStoragePoolListAllVolumes() failed")

retlist = list() for volptr in ret: diff --git a/libvirt-override.py b/libvirt-override.py index 61d4103..1a1ad43 100644 --- a/libvirt-override.py +++ b/libvirt-override.py @@ -26,12 +26,7 @@ except ImportError: # The root of all libvirt errors. class libvirtError(Exception): def __init__(self, - defmsg, # type: str - conn=None, # type: Optional[virConnect] - dom=None, # type: Optional[virDomain] - net=None, # type: Optional[virNetwork] - pool=None, # type: Optional[virStoragePool] - vol=None # type: Optional[virStorageVol] + defmsg # type: str ): # type: (...) -> None

# Never call virConnGetLastError(). -- 2.11.0

-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list

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