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

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.

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

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