[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [BUG][PATCH][RRFC][libvirt-python] libvirtError(..., conn=, dom=. vol=, pool=, snap=)



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


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]