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

[Philipp Hahn]

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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Remove-legacy-libvirtError-arguments.patch
Type: text/x-patch
Size: 23925 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20181126/ea8a2209/attachment-0001.bin>