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:
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
Am 26.11.18 um 16:28 schrieb Michal Privoznik: 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