[libvirt] using public vir*Free() (instead of virUnref*()) from vir*FreeName() destructors.
I just noticed in datatypes.c that the vir(Network|StoragePool|StorageVol)FreeName destructors call the public API vir*Free() functions rather than the local virUnref*(). vir*Free() all clear any errors, which seems like it might not be the right thing to do (eg, if we're cleaning out the hashtables as part of some error-handling path, and haven't yet logged the error). On the other hand, the public API function does some sanity checking on the object to make sure it really is what we think it is before calling the unref. So it's got that going for it. (5 points to anyone who catches the reference without going to Google) But I don't know enough to know whether or not that's important in this case. So which is more important: the extra sanity check, or not clearing existing error codes?
On Thu, May 07, 2009 at 12:22:56PM -0400, Laine Stump wrote:
I just noticed in datatypes.c that the vir(Network|StoragePool|StorageVol)FreeName destructors call the public API vir*Free() functions rather than the local virUnref*(). vir*Free() all clear any errors, which seems like it might not be the right thing to do (eg, if we're cleaning out the hashtables as part of some error-handling path, and haven't yet logged the error).
On the other hand, the public API function does some sanity checking on the object to make sure it really is what we think it is before calling the unref. So it's got that going for it. (5 points to anyone who catches the reference without going to Google) But I don't know enough to know whether or not that's important in this case.
So which is more important: the extra sanity check, or not clearing existing error codes?
IMHO the existing code is wrong, because these functions are invokved from the hash table destructor, which is invokved from the code that destroys the virConnectPtr. It really isn't safe to call back to the public APIs like this from the middle of the virCOnnectPtr destructor. So we should change them to directly call Unref. Regards, Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
Something like this? Do you think that the virUnref*() functions should continue to error out if the conn is invalid? Or should they maybe just refrain from grabbing the conn mutex, but still decrement the ref count and release the object when necessary? (P.S. Is this patch mail formatted correctly? I'm doing this as much to get the procedure correct as to fix the code...) --- src/datatypes.c | 8 ++++---- 1 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/datatypes.c b/src/datatypes.c index b1013f2..eceb839 100644 --- a/src/datatypes.c +++ b/src/datatypes.c @@ -49,7 +49,7 @@ static int virDomainFreeName(virDomainPtr domain, const char *name ATTRIBUTE_UNUSED) { - return (virDomainFree(domain)); + return (virUnrefDomain(domain)); } /** @@ -63,7 +63,7 @@ virDomainFreeName(virDomainPtr domain, const char *name ATTRIBUTE_UNUSED) static int virNetworkFreeName(virNetworkPtr network, const char *name ATTRIBUTE_UNUSED) { - return (virNetworkFree(network)); + return (virUnrefNetwork(network)); } /** @@ -77,7 +77,7 @@ virNetworkFreeName(virNetworkPtr network, const char *name ATTRIBUTE_UNUSED) static int virStoragePoolFreeName(virStoragePoolPtr pool, const char *name ATTRIBUTE_UNUSED) { - return (virStoragePoolFree(pool)); + return (virUnrefStoragePool(pool)); } /** @@ -91,7 +91,7 @@ virStoragePoolFreeName(virStoragePoolPtr pool, const char *name ATTRIBUTE_UNUSED static int virStorageVolFreeName(virStorageVolPtr vol, const char *name ATTRIBUTE_UNUSED) { - return (virStorageVolFree(vol)); + return (virUnrefStorageVol(vol)); } /** -- 1.6.0.6
On Thu, May 07, 2009 at 02:05:06PM -0400, Laine Stump wrote:
Something like this?
Do you think that the virUnref*() functions should continue to error out if the conn is invalid? Or should they maybe just refrain from grabbing the conn mutex, but still decrement the ref count and release the object when necessary?
I think this patch is fine. The reason the public APIs do error checking on the connection object, is because we dont entirely trust the source. In this scenario, the objects are coming straight out of our internal hash table, and if we can't trust that then we're so badly doomed we've got bigger things to worry about :-) So, ACK to this patch
(P.S. Is this patch mail formatted correctly? I'm doing this as much to get the procedure correct as to fix the code...)
Looks fine to me. I prefer inline patches :-)
--- src/datatypes.c | 8 ++++---- 1 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/src/datatypes.c b/src/datatypes.c index b1013f2..eceb839 100644 --- a/src/datatypes.c +++ b/src/datatypes.c @@ -49,7 +49,7 @@ static int virDomainFreeName(virDomainPtr domain, const char *name ATTRIBUTE_UNUSED) { - return (virDomainFree(domain)); + return (virUnrefDomain(domain)); }
/** @@ -63,7 +63,7 @@ virDomainFreeName(virDomainPtr domain, const char *name ATTRIBUTE_UNUSED) static int virNetworkFreeName(virNetworkPtr network, const char *name ATTRIBUTE_UNUSED) { - return (virNetworkFree(network)); + return (virUnrefNetwork(network)); }
/** @@ -77,7 +77,7 @@ virNetworkFreeName(virNetworkPtr network, const char *name ATTRIBUTE_UNUSED) static int virStoragePoolFreeName(virStoragePoolPtr pool, const char *name ATTRIBUTE_UNUSED) { - return (virStoragePoolFree(pool)); + return (virUnrefStoragePool(pool)); }
/** @@ -91,7 +91,7 @@ virStoragePoolFreeName(virStoragePoolPtr pool, const char *name ATTRIBUTE_UNUSED static int virStorageVolFreeName(virStorageVolPtr vol, const char *name ATTRIBUTE_UNUSED) { - return (virStorageVolFree(vol)); + return (virUnrefStorageVol(vol)); }
/** -- 1.6.0.6
-- Libvir-list mailing list Libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
-- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
participants (2)
-
Daniel P. Berrange -
Laine Stump