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

[Philipp Hahn]

Hi,

while working on the Python type annotations for the Python libvirt
binding I noticed the following code in

"""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().
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?

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

Philipp
--
Philipp Hahn
Open Source Software Engineer

Univention GmbH
be open.
Mary-Somerville-Str. 1
D-28359 Bremen
Tel.: [phone]
Fax : [phone]
***@univention.de

http://www.univention.de/
GeschÀftsfÌhrer: Peter H. Ganten
HRB 20755 Amtsgericht Bremen
Steuer-Nr.: 71-597-02876

[Michal Privoznik]

Post by Philipp Hahn
Hi,
while working on the Python type annotations for the Python libvirt
binding I noticed the following code in

"""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().
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.

Post by Philipp Hahn
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?)

Michal

[Philipp Hahn]

Hello,

Post by Michal Privoznik

Post by Philipp Hahn
while working on the Python type annotations for the Python libvirt
binding I noticed the following code in

"""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().
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

Post by Michal Privoznik
142 /**
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.

Post by Michal Privoznik

Post by Philipp Hahn
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

[Daniel P. Berrangé]

Post by Philipp Hahn
Hello,

Post by Michal Privoznik

Post by Philipp Hahn
while working on the Python type annotations for the Python libvirt
binding I noticed the following code in

"""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().
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

Post by Michal Privoznik
142 /**
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.

Post by Philipp Hahn
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.

Post by Michal Privoznik

Post by Philipp Hahn
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
Date: Wed, 21 Nov 2018 07:55:57 +0100
Subject: [PATCH libvirt-python] *: Remove legacy libvirtError arguments
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
---
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
classes.write(
- " if ret is None:raise libvirtError('%s() failed', conn=self)\n" %
+ " if ret is None:raise libvirtError('%s() failed')\n" %
(name))
classes.write(
- " if ret is None:raise libvirtError('%s() failed', dom=self)\n" %
+ " if ret is None:raise libvirtError('%s() failed')\n" %
(name))
classes.write(
- " if ret is None:raise libvirtError('%s() failed', net=self)\n" %
+ " if ret is None:raise libvirtError('%s() failed')\n" %
(name))
classes.write(
- " if ret is None:raise libvirtError('%s() failed', net=self)\n" %
+ " if ret is None:raise libvirtError('%s() failed')\n" %
(name))
classes.write(
- " if ret is None:raise libvirtError('%s() failed', pool=self)\n" %
+ " if ret is None:raise libvirtError('%s() failed')\n" %
(name))
classes.write(
- " if ret is None:raise libvirtError('%s() failed', vol=self)\n" %
+ " if ret is None:raise libvirtError('%s() failed')\n" %
(name))
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))
classes.write(
@@ -1657,27 +1657,27 @@ def buildWrappers(module # type: str
test = functions_int_default_test
classes.write((" if " + test +
- ": raise libvirtError('%s() failed', conn=self)\n") %
+ ": raise libvirtError('%s() failed')\n") %
("ret", name))
classes.write((" if " + test +
- ": raise libvirtError('%s() failed', dom=self)\n") %
+ ": raise libvirtError('%s() failed')\n") %
("ret", name))
classes.write((" if " + test +
- ": raise libvirtError('%s() failed', net=self)\n") %
+ ": raise libvirtError('%s() failed')\n") %
("ret", name))
classes.write((" if " + test +
- ": raise libvirtError('%s() failed', net=self)\n") %
+ ": raise libvirtError('%s() failed')\n") %
("ret", name))
classes.write((" if " + test +
- ": raise libvirtError('%s() failed', pool=self)\n") %
+ ": raise libvirtError('%s() failed')\n") %
("ret", name))
classes.write((" if " + test +
- ": raise libvirtError('%s() failed', vol=self)\n") %
+ ": raise libvirtError('%s() failed')\n") %
("ret", name))
classes.write((" if " + test +
@@ -1690,27 +1690,27 @@ def buildWrappers(module # type: str
classes.write((" if %s is None" +
- ": raise libvirtError('%s() failed', conn=self)\n") %
+ ": raise libvirtError('%s() failed')\n") %
("ret", name))
classes.write((" if %s is None" +
- ": raise libvirtError('%s() failed', dom=self)\n") %
+ ": raise libvirtError('%s() failed')\n") %
("ret", name))
classes.write((" if %s is None" +
- ": raise libvirtError('%s() failed', net=self)\n") %
+ ": raise libvirtError('%s() failed')\n") %
("ret", name))
classes.write((" if %s is None" +
- ": raise libvirtError('%s() failed', net=self)\n") %
+ ": raise libvirtError('%s() failed')\n") %
("ret", name))
classes.write((" if %s is None" +
- ": raise libvirtError('%s() failed', pool=self)\n") %
+ ": raise libvirtError('%s() failed')\n") %
("ret", name))
classes.write((" if %s is None" +
- ": raise libvirtError('%s() failed', vol=self)\n") %
+ ": raise libvirtError('%s() failed')\n") %
("ret", name))
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)
- raise libvirtError('virConnectDomainEventDeregister() failed', conn=self)
+ raise libvirtError('virConnectDomainEventDeregister() failed')
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)
- raise libvirtError('virConnectDomainEventRegister() failed', conn=self)
+ raise libvirtError('virConnectDomainEventRegister() failed')
def _dispatchDomainEventCallbacks(self,
dom, # type: virDomain
@@ -397,7 +397,7 @@
ret = libvirtmod.virConnectDomainEventDeregisterAny(self._o, callbackID)
- raise libvirtError('virConnectDomainEventDeregisterAny() failed', conn=self)
+ raise libvirtError('virConnectDomainEventDeregisterAny() failed')
del self.domainEventCallbackID[callbackID]
pass
@@ -424,7 +424,7 @@
ret = libvirtmod.virConnectNetworkEventDeregisterAny(self._o, callbackID)
- raise libvirtError('virConnectNetworkEventDeregisterAny() failed', conn=self)
+ raise libvirtError('virConnectNetworkEventDeregisterAny() failed')
del self.networkEventCallbackID[callbackID]
pass
@@ -445,7 +445,7 @@
ret = libvirtmod.virConnectNetworkEventRegisterAny(self._o, net._o, eventID, cbData)
- raise libvirtError('virConnectNetworkEventRegisterAny() failed', conn=self)
+ raise libvirtError('virConnectNetworkEventRegisterAny() failed')
self.networkEventCallbackID[ret] = opaque
return ret
@@ -465,7 +465,7 @@
ret = libvirtmod.virConnectDomainEventRegisterAny(self._o, dom._o, eventID, cbData)
- raise libvirtError('virConnectDomainEventRegisterAny() failed', conn=self)
+ raise libvirtError('virConnectDomainEventRegisterAny() failed')
self.domainEventCallbackID[ret] = opaque
return ret
@@ -505,7 +505,7 @@
ret = libvirtmod.virConnectStoragePoolEventDeregisterAny(self._o, callbackID)
- raise libvirtError('virConnectStoragePoolEventDeregisterAny() failed', conn=self)
+ raise libvirtError('virConnectStoragePoolEventDeregisterAny() failed')
del self.storagePoolEventCallbackID[callbackID]
pass
@@ -526,7 +526,7 @@
ret = libvirtmod.virConnectStoragePoolEventRegisterAny(self._o, pool._o, eventID, cbData)
- raise libvirtError('virConnectStoragePoolEventRegisterAny() failed', conn=self)
+ raise libvirtError('virConnectStoragePoolEventRegisterAny() failed')
self.storagePoolEventCallbackID[ret] = opaque
return ret
@@ -566,7 +566,7 @@
ret = libvirtmod.virConnectNodeDeviceEventDeregisterAny(self._o, callbackID)
- raise libvirtError('virConnectNodeDeviceEventDeregisterAny() failed', conn=self)
+ raise libvirtError('virConnectNodeDeviceEventDeregisterAny() failed')
del self.nodeDeviceEventCallbackID[callbackID]
pass
@@ -587,7 +587,7 @@
ret = libvirtmod.virConnectNodeDeviceEventRegisterAny(self._o, dev._o, eventID, cbData)
- raise libvirtError('virConnectNodeDeviceEventRegisterAny() failed', conn=self)
+ raise libvirtError('virConnectNodeDeviceEventRegisterAny() failed')
self.nodeDeviceEventCallbackID[ret] = opaque
return ret
@@ -625,7 +625,7 @@
ret = libvirtmod.virConnectSecretEventDeregisterAny(self._o, callbackID)
- raise libvirtError('virConnectSecretEventDeregisterAny() failed', conn=self)
+ raise libvirtError('virConnectSecretEventDeregisterAny() failed')
del self.secretEventCallbackID[callbackID]
pass
@@ -646,7 +646,7 @@
ret = libvirtmod.virConnectSecretEventRegisterAny(self._o, secret._o, eventID, cbData)
- 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)
- raise libvirtError("virConnectListAllDomains() failed", conn=self)
+ raise libvirtError("virConnectListAllDomains() failed")
retlist = list()
@@ -670,7 +670,7 @@
"""Returns a list of storage pool objects"""
ret = libvirtmod.virConnectListAllStoragePools(self._o, flags)
- raise libvirtError("virConnectListAllStoragePools() failed", conn=self)
+ raise libvirtError("virConnectListAllStoragePools() failed")
retlist = list()
@@ -684,7 +684,7 @@
"""Returns a list of network objects"""
ret = libvirtmod.virConnectListAllNetworks(self._o, flags)
- raise libvirtError("virConnectListAllNetworks() failed", conn=self)
+ raise libvirtError("virConnectListAllNetworks() failed")
retlist = list()
@@ -698,7 +698,7 @@
"""Returns a list of interface objects"""
ret = libvirtmod.virConnectListAllInterfaces(self._o, flags)
- raise libvirtError("virConnectListAllInterfaces() failed", conn=self)
+ raise libvirtError("virConnectListAllInterfaces() failed")
retlist = list()
@@ -712,7 +712,7 @@
"""Returns a list of host node device objects"""
ret = libvirtmod.virConnectListAllNodeDevices(self._o, flags)
- raise libvirtError("virConnectListAllNodeDevices() failed", conn=self)
+ raise libvirtError("virConnectListAllNodeDevices() failed")
retlist = list()
@@ -726,7 +726,7 @@
"""Returns a list of network filter objects"""
ret = libvirtmod.virConnectListAllNWFilters(self._o, flags)
- raise libvirtError("virConnectListAllNWFilters() failed", conn=self)
+ raise libvirtError("virConnectListAllNWFilters() failed")
retlist = list()
@@ -740,7 +740,7 @@
"""Returns a list of network filter binding objects"""
ret = libvirtmod.virConnectListAllNWFilterBindings(self._o, flags)
- raise libvirtError("virConnectListAllNWFilterBindings() failed", conn=self)
+ raise libvirtError("virConnectListAllNWFilterBindings() failed")
retlist = list()
@@ -754,7 +754,7 @@
"""Returns a list of secret objects"""
ret = libvirtmod.virConnectListAllSecrets(self._o, flags)
- raise libvirtError("virConnectListAllSecrets() failed", conn=self)
+ raise libvirtError("virConnectListAllSecrets() failed")
retlist = list()
@@ -777,7 +777,7 @@
"""Removes a close event callback"""
ret = libvirtmod.virConnectUnregisterCloseCallback(self._o)
- 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)
- 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)
- 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)
- raise libvirtError("virConnectGetAllDomainStats() failed", conn=self)
+ raise libvirtError("virConnectGetAllDomainStats() failed")
retlist = list()
@@ -918,13 +918,13 @@
domlist = list()
- 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)
- raise libvirtError("virDomainListGetStats() failed", conn=self)
+ raise libvirtError("virDomainListGetStats() failed")
retlist = list()
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)
- raise libvirtError("virDomainListAllSnapshots() failed", conn=self)
+ raise libvirtError("virDomainListAllSnapshots() failed")
retlist = list()
@@ -50,7 +50,7 @@
file for this domain is discarded, and the domain boots from scratch. """
ret = libvirtmod.virDomainCreateWithFiles(self._o, files, flags)
- 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)
- 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)
- 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)
- 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)
- 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)
- raise libvirtError("virDomainSnapshotListAllChildren() failed", conn=self)
+ raise libvirtError("virDomainSnapshotListAllChildren() failed")
retlist = list()
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)
- raise libvirtError("virStoragePoolListAllVolumes() failed", conn=self)
+ raise libvirtError("virStoragePoolListAllVolumes() failed")
retlist = list()
diff --git a/libvirt-override.py b/libvirt-override.py
index 61d4103..1a1ad43 100644
--- a/libvirt-override.py
+++ b/libvirt-override.py
# The root of all libvirt errors.
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
https://www.redhat.com/mailman/listinfo/libvir-list

Regards,
Daniel

[Philipp Hahn]

Hello,

Post by Daniel P. Berrangé

Post by Philipp Hahn

Post by Michal Privoznik

Post by Philipp Hahn
while working on the Python type annotations for the Python libvirt
binding I noticed the following code in

...

Post by Daniel P. Berrangé

Post by Philipp Hahn

Post by Michal Privoznik

Post by Philipp Hahn
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.

...

Post by Daniel P. Berrangé

Post by Philipp Hahn
The fields have been deprecated with
git:f60dc0bc09f09c6817d6706a9edb1579a3e2b2b8 in "libvirt" and there is

Post by Michal Privoznik
142 /**
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.
20 # The root of all libvirt errors.
23
24 # Never call virConnGetLastError().
25 # virGetLastError() is now thread local
26 err = libvirtmod.virGetLastError()
28 msg = defmsg
30 msg = err[2]
31
32 Exception.__init__(self, msg)
33
34 self.err = err

conn, dom, net, pool, vol are arguments to the __init__() function, but
they are *nowhere* referenced in that function. There is *no* code like

self.conn = conn
self.dom = dom
self.net = net
self.pool = pool
self.vol = vol

so the function arguments are *not* used at all!

There is no other class inheriting from libvirtError and the generated
code should be the only one "raising libvirtError" directly.

So why do we pass those information if we don't need them?

And we already have broken code passing instances of the wrong type (see
original email with those 3 patches). I only found then when adding type
annotations and the question remains, if we should add new unused
arguments or if we should delete them now and be done with them.

I find it very confusing to pass the object, where the error occurred,
and to not have an accessor to get that information back.

I'm also not keen to add new arguments for each new vir* type like
- virNodeDevice
- virSecret
- virNWFilterBinding
- virStream
- virDomainSnapshot

I would prefer to document the existing arguments as "unused" and to
never add any new arguments for new types.

Just my 2¢

Philipp