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
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list