Re: [libvirt] [BUG][PATCH][RRFC][libvirt-python] libvirtError(..., conn=, dom=. vol=, pool=, snap=)
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