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.
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@xxxxxxxxxxxxx>
From: Philipp Hahn <hahn@xxxxxxxxxxxxx>
Date: Wed, 21 Nov 2018 07:55:57 +0100
Subject: [PATCH libvirt-python] *: Remove legacy libvirtError arguments
To: libvir-list@xxxxxxxxxx
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@xxxxxxxxxxxxx>
---
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@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
