On 07/31/2012 10:58 AM, Daniel P. Berrange wrote:
From: "Daniel P. Berrange" <berrange@redhat.com>
This converts the following public API datatypes to use the virObject infrastructure:
virConnectPtr virDomainPtr virDomainSnapshotPtr virInterfacePtr virNetworkPtr virNodeDevicePtr virNWFilterPtr virSecretPtr virStreamPtr virStorageVolPtr virStoragePoolPtr
Given recent changes, this now fails to apply cleanly. Would you mind refreshing the series, to make it easier to review?
16 files changed, 448 insertions(+), 1086 deletions(-)
Love the diffstat! I still spotted some nits, even going solely by code inspection:
@@ -225,64 +217,21 @@ virGetDomain(virConnectPtr conn, const char *name, const unsigned char *uuid) { * which may also be released if its ref count hits zero. */ static void -virReleaseDomain(virDomainPtr domain) { - virConnectPtr conn = domain->conn; +virDomainDispose(void *obj) +{
- - if (conn) { - VIR_DEBUG("unref connection %p %d", conn, conn->refs); - conn->refs--; - if (conn->refs == 0) { - virReleaseConnect(conn); - /* Already unlocked mutex */ - return; - } - virMutexUnlock(&conn->lock); - } + if (domain->conn) + virObjectUnref(domain->conn);
Technically, we have a bug if we ever have domain->conn==NULL when we get here. Besides, you coded virObjectUnref(NULL) to be a graceful no-op, so that means you should add virObjectUnref to cfg.mk's list of free-like functions and drop the conditional. Oh, that reminds me - we DO have a bug in our RPC code where we fail to check for NULL after strdup and such in the various make_nonnull_* functions, and therefore I suppose that is an instance where we really _could_ end up with domain->conn being NULL at the moment - I've been meaning to spend some time on fixing that.
@@ -349,60 +289,17 @@ virGetNetwork(virConnectPtr conn, const char *name, const unsigned char *uuid) { * which may also be released if its ref count hits zero. */ static void -virReleaseNetwork(virNetworkPtr network) { - virConnectPtr conn = network->conn; +virNetworkDispose(void *obj) +{ + virNetworkPtr network = obj; char uuidstr[VIR_UUID_STRING_BUFLEN];
+ if (network->conn) + virObjectUnref(network->conn);
Another place where you can drop the conditional.
@@ -484,58 +366,15 @@ virGetInterface(virConnectPtr conn, const char *name, const char *mac) { * which may also be released if its ref count hits zero. */ static void -virReleaseInterface(virInterfacePtr iface) { - virConnectPtr conn = iface->conn; +virInterfaceDispose(void *obj) +{ + virInterfacePtr iface = obj; + if (iface->conn) + virObjectUnref(iface->conn);
and again
@@ -608,60 +438,17 @@ error: * which may also be released if its ref count hits zero. */ static void -virReleaseStoragePool(virStoragePoolPtr pool) { - virConnectPtr conn = pool->conn; +virStoragePoolDispose(void *obj) +{ + virStoragePoolPtr pool = obj; char uuidstr[VIR_UUID_STRING_BUFLEN];
+ if (pool->conn) + virObjectUnref(pool->conn);
I'm turning into a broken record...
@@ -748,59 +516,16 @@ error: * which may also be released if its ref count hits zero. */ static void -virReleaseStorageVol(virStorageVolPtr vol) { - virConnectPtr conn = vol->conn; +virStorageVolDispose(void *obj) +{ + virStorageVolPtr vol = obj;
+ if (vol->conn) + virObjectUnref(vol->conn);
Need I say it? :) I'll quit mentioning it (again, a cfg.mk rule will make it easier to find all instances).
+++ b/src/datatypes.h
+# define VIR_IS_CONNECT(obj) \ + (virObjectIsClass((virObjectPtr)(obj), virConnectClass))
Why the extra cast here? The compiler can convert obj to 'void*' without your help.
+ +# define VIR_IS_DOMAIN(obj) \ + (virObjectIsClass((virObjectPtr)(obj), virDomainClass))
Likewise throughout these macros.
+++ b/src/libvirt.c
@@ -1420,14 +1420,16 @@ error: * matching virConnectClose, and all other references will be released * after the corresponding operation completes. * - * Returns the number of remaining references on success - * (positive implies that some other call still has a reference open, - * 0 implies that no references remain and the connection is closed), - * or -1 on failure. It is possible for the last virConnectClose to - * return a positive value if some other object still has a temporary - * reference to the connection, but the application should not try to - * further use a connection after the virConnectClose that matches the - * initial open. + * Returns the a positive number if at least 1 reference remains on
s/the a/a/
--- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms +virSecretClass; +virStorageVolClass; +virStoragePoolClass;
Sorting.
+++ b/src/xen/xend_internal.c @@ -1237,7 +1237,7 @@ error: virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("failed to parse Xend domain information")); if (ret != NULL) - virUnrefDomain(ret); + virObjectUnref(ret);
Another useless if before free.
+++ b/tests/sexpr2xmltest.c @@ -72,7 +72,7 @@ testCompareFiles(const char *xml, const char *sexpr, int xendConfigVersion) VIR_FREE(gotxml); virDomainDefFree(def); if (conn) - virUnrefConnect(conn); + virObjectUnref(conn);
Oops, I promised not to point them out :) -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org