[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH v2] python: Fix bindings for virDomainSnapshotGet{Domain, Connect}



On 01/25/2013 11:35 AM, Jiri Denemark wrote:
> On Thu, Jan 24, 2013 at 17:37:31 -0500, Cole Robinson wrote:
>> On 01/23/2013 06:26 AM, Jiri Denemark wrote:
>>> https://bugzilla.redhat.com/show_bug.cgi?id=895882
>>>
>>> virDomainSnapshot.getDomain() and virDomainSnapshot.getConnect()
>>> wrappers around virDomainSnapshotGet{Domain,Connect} were not supposed
>>> to be ever implemented. The class should contain proper domain() and
>>> connect() accessors that fetch python objects stored internally within
>>> the class. While domain() was already provided, connect() was missing.
>>>
>>> This patch adds connect() method to virDomainSnapshot class and
>>> reimplements getDomain() and getConnect() methods as aliases to domain()
>>> and connect() for backward compatibility.
>>> ---
>>>  python/generator.py                          |  4 +++-
>>>  python/libvirt-override-virDomainSnapshot.py |  8 ++++++++
>>>  src/libvirt.c                                | 10 ++++++++--
>>>  3 files changed, 19 insertions(+), 3 deletions(-)
>>
>>
>>> diff --git a/python/libvirt-override-virDomainSnapshot.py b/python/libvirt-override-virDomainSnapshot.py
>>> index 3da7bfd..bf708a5 100644
>>> --- a/python/libvirt-override-virDomainSnapshot.py
>>> +++ b/python/libvirt-override-virDomainSnapshot.py
>>> @@ -1,3 +1,11 @@
>>> +    def getConnect(self):
>>> +        """Get the connection that owns the domain that a snapshot was created for"""
>>> +        return self.connect()
>>> +
>>> +    def getDomain(self):
>>> +        """Get the domain that a snapshot was created for"""
>>> +        return self.domain()
>>> +
>>>      def listAllChildren(self, flags):
>>>          """List all child snapshots and returns a list of snapshot objects"""
>>>          ret = libvirtmod.virDomainSnapshotListAllChildren(self._o, flags)
>>
>> Not a big deal, but I think this chunk should be reverted. None of the other
>> classes provide getConnect, and it is inconsistent with how virDomainGetInfo()
>> is converted to virDomain.info()
> 
> You are right, but we have getConnect and getDomain methods in
> virDomainSnapshot since 0.9.5 (although their implementation was not
> exactly correct) and thus we need to keep them for backward
> compatibility.

I'm pretty sure using getConnect and getDomain would actually cause crashes,
at least that was my experience when I patched out other getConnect instances
in the past. If that's the case then they were never usable anyways and IMO
can just be dropped, but it's a minor point.

Thanks,
Cole


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]