1:17 a.m.
Hi,
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?
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.: +49 421 22232-0
Fax : +49 421 22232-99
hahn(a)univention.de
http://www.univention.de/
Geschäftsführer: Peter H. Ganten
HRB 20755 Amtsgericht Bremen
Steuer-Nr.: 71-597-02876
9:28 a.m.
On 11/21/18 8:17 AM, Philipp Hahn wrote:
Hi,
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.
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
10:12 a.m.
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
8:29 a.m.
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(a)univention.de>
From: Philipp Hahn <hahn(a)univention.de>
Date: Wed, 21 Nov 2018 07:55:57 +0100
Subject: [PATCH libvirt-python] *: Remove legacy libvirtError arguments
To: libvir-list(a)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(a)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(a)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 :|
8:56 a.m.
Hello,
Am 27.11.18 um 15:29 schrieb Daniel P. Berrangé:
On Mon, Nov 26, 2018 at 05:12:06PM +0100, Philipp Hahn wrote:
> 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:
...
>>> 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.
...
> 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.
Quoting from libvirt-override.py:
20 # The root of all libvirt errors.
21 class libvirtError(Exception):
22 def __init__(self, defmsg, conn=None, dom=None, net=None, pool=None,
vol=None):
23
24 # Never call virConnGetLastError().
25 # virGetLastError() is now thread local
26 err = libvirtmod.virGetLastError()
27 if err is None:
28 msg = defmsg
29 else:
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
2187
days inactive
2193
days old
4 comments
3 participants
participants (3)
-
Daniel P. Berrangé -
Michal Privoznik -
Philipp Hahn