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@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
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 :|
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
