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