[libvirt] [PATCH] python: Fix bindings for virDomainSnapshotGetDomain

[Eric Blake]

On 01/22/2013 11:47 AM, Jiri Denemark wrote:
> https://bugzilla.redhat.com/show_bug.cgi?id=895882
> 
> virDomainSnapshotGetDomain returns snapshot->domain without incrementing
> domain's reference counter.

Is that a bug in virDomainSnapshotGetDomain?  [looking...]
Hmm, we don't increment the reference count of a connection in
virDomainGetConnect, since a domain object can only exist as long as the
connection object also exists.  And since a snapshot object only exists
as long as the domain object exists, this sounds like the same approach.

Ah, but in libvirt.c, we explicitly document the non-reference-counting
properties of virDomainGetConnect and others such as
virSecretGetConnect; we should do the same for virDomainSnapshotGetName
and virDomainSnapshotGetConnect.

> The autogenerated python wrapper around this
> API did not honor this fact and created a new virDomain object from the
> snapshot's domain. When this object is deleted, it calls virDomainFree.
> This caused python client to crash when the domain object is accessed
> after it has been freed.
> ---
>  python/generator.py       |  1 +
>  python/libvirt-override.c | 24 ++++++++++++++++++++++++
>  src/libvirt.c             |  6 +++++-
>  3 files changed, 30 insertions(+), 1 deletion(-)

Do we even really want to expose virDomainSnapshotGetDomain in the
bindings?  After all, generator.py already states:

    # This functions shouldn't be called via the bindings (and even the docs
    # contain an explicit warning to that effect). The equivalent should be
    # implemented in pure python for each class
    "virDomainGetConnect",
    "virInterfaceGetConnect",
    "virNetworkGetConnect",
    "virSecretGetConnect",
    "virNWFilterGetConnect",
    "virStoragePoolGetConnect",
    "virStorageVolGetConnect",


> 
> diff --git a/python/generator.py b/python/generator.py
> index f853d77..7e76c2a 100755
> --- a/python/generator.py
> +++ b/python/generator.py
> @@ -379,6 +379,7 @@ skip_impl = (
>      'virConnectListDefinedInterfaces',
>      'virConnectListNWFilters',
>      'virDomainSnapshotListNames',
> +    'virDomainSnapshotGetDomain',

I'd rather hoist this line up to be next to the other GetConnect
counterparts that have the same problem, as well as adding
virDomainSnapshotGetConnect to the list of functions needing this treatment.

>      'virDomainSnapshotListChildrenNames',
>      'virConnGetLastError',
>      'virGetLastError',
> diff --git a/python/libvirt-override.c b/python/libvirt-override.c
> index 8154024..92dc939 100644
> --- a/python/libvirt-override.c
> +++ b/python/libvirt-override.c
> @@ -2212,6 +2212,29 @@ cleanup:
>  }
>  
>  static PyObject *
> +libvirt_virDomainSnapshotGetDomain(PyObject *self ATTRIBUTE_UNUSED,
> +                                   PyObject *args)
> +{

Seeing as how we do NOT have libvirt_virDomainGetConnect() in
libvirt-override.c, I think the better approach is to completely delete
this function as utterly broken and useless.  Instead, figure out how
libvirt.virDomain.connect() works, and implement
libvirt.virDomainSnapshot.domain() and
libvirt.virDomainSnapshot.connect() in the same manner (hmm, likes like
virDomainSnapshot.domain() already exists, but we are missing
virDomainSnapshot.connect(), when reading the generated libvirt.py file).

> +++ b/src/libvirt.c
> @@ -17850,7 +17850,11 @@ virDomainSnapshotGetName(virDomainSnapshotPtr snapshot)
>   * virDomainSnapshotGetDomain:
>   * @snapshot: a snapshot object
>   *
> - * Get the domain that a snapshot was created for
> + * Get the domain that a snapshot was created for. The caller must not call
> + * virDomainFree() on the returned domain unless it calls virDomainRef() first
> + * as this function does not increment domain's reference counter. The returned
> + * object will be automatically freed with the end of life of the snapshot
> + * object.

Please copy the wording in virDomainGetConnect() did, as well as touch
up virDomainSnapshotGetConnect().

Awaiting v2.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 621 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20130122/083c1719/attachment-0001.sig>