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

[Philipp Hahn]

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