Hi Dan I noticed that I misunderstood your suggestion. --> I heard the point that was not good of this patch which Daniel Veillard thought from Saori Fukuta. Thanks Daniel and Saori. I misunderstood that the point that is missing a call of virGetDomain() exists before this patch is applied. But your indication is that the point that is missing a call of virGetDomain() emerges after this patch is applied. Therefore, I remake this patch. This patch changes as follows. - the argument of virFreeDomain() doesn't change. - virFreeDomainInternal() is added as the function that is called only from hash.c. Thanks Masayuki Sunou -------------------------------------------------------------------------------- Index: src/hash.c =================================================================== RCS file: /data/cvs/libvirt/src/hash.c,v retrieving revision 1.22 diff -u -p -r1.22 hash.c --- src/hash.c 8 May 2007 10:53:27 -0000 1.22 +++ src/hash.c 22 Jun 2007 05:45:30 -0000 @@ -731,6 +731,71 @@ virFreeConnect(virConnectPtr conn) { } /** + * virFreeDomainInternal: + * @conn: the hypervisor connection + * @domain: the domain to release + * @domain: the flag to execute xmlMutexLock + * + * Release the given domain, if the reference count drops to zero, then + * the domain is really freed. + * This function is called only from hash.c. + * + * Returns the reference count or -1 in case of failure. + */ +int +virFreeDomainInternal(virConnectPtr conn, virDomainPtr domain, int lock) { + int ret = 0; + + if ((!VIR_IS_CONNECT(conn)) || (!VIR_IS_CONNECTED_DOMAIN(domain)) || + (domain->conn != conn) || (conn->hashes_mux == NULL)) { + virHashError(conn, VIR_ERR_INVALID_ARG, __FUNCTION__); + return(-1); + } + if (lock) + xmlMutexLock(conn->hashes_mux); + + /* + * decrement the count for the domain + */ + domain->uses--; + ret = domain->uses; + if (ret > 0) + goto done; + + /* TODO search by UUID first as they are better differenciators */ + + if (virHashRemoveEntry(conn->domains, domain->name, NULL) < 0) { + virHashError(conn, VIR_ERR_INTERNAL_ERROR, + _("domain missing from connection hash table")); + goto done; + } + domain->magic = -1; + domain->id = -1; + if (domain->name) + free(domain->name); + free(domain); + + /* + * decrement the count for the connection + */ + conn->uses--; + if (conn->uses > 0) + goto done; + + if (conn->domains != NULL) + virHashFree(conn->domains, (virHashDeallocator) virDomainFreeName); + if (conn->hashes_mux != NULL && lock) + xmlFreeMutex(conn->hashes_mux); + free(conn); + return(0); + +done: + if (lock) + xmlMutexUnlock(conn->hashes_mux); + return(ret); +} + +/** * virGetDomain: * @conn: the hypervisor connection * @name: pointer to the domain name @@ -758,8 +823,14 @@ __virGetDomain(virConnectPtr conn, const ret = (virDomainPtr) virHashLookup(conn->domains, name); if (ret != NULL) { - /* TODO check the UUID */ - goto done; + if (memcmp(ret->uuid, uuid, 16)) { + if (virFreeDomainInternal(conn, ret, 0) < 0) { + xmlMutexUnlock(conn->hashes_mux); + return(NULL); + } + } else { + goto done; + } } /* @@ -814,53 +885,7 @@ error: */ int virFreeDomain(virConnectPtr conn, virDomainPtr domain) { - int ret = 0; - - if ((!VIR_IS_CONNECT(conn)) || (!VIR_IS_CONNECTED_DOMAIN(domain)) || - (domain->conn != conn) || (conn->hashes_mux == NULL)) { - virHashError(conn, VIR_ERR_INVALID_ARG, __FUNCTION__); - return(-1); - } - xmlMutexLock(conn->hashes_mux); - - /* - * decrement the count for the domain - */ - domain->uses--; - ret = domain->uses; - if (ret > 0) - goto done; - - /* TODO search by UUID first as they are better differenciators */ - - if (virHashRemoveEntry(conn->domains, domain->name, NULL) < 0) { - virHashError(conn, VIR_ERR_INTERNAL_ERROR, - _("domain missing from connection hash table")); - goto done; - } - domain->magic = -1; - domain->id = -1; - if (domain->name) - free(domain->name); - free(domain); - - /* - * decrement the count for the connection - */ - conn->uses--; - if (conn->uses > 0) - goto done; - - if (conn->domains != NULL) - virHashFree(conn->domains, (virHashDeallocator) virDomainFreeName); - if (conn->hashes_mux != NULL) - xmlFreeMutex(conn->hashes_mux); - free(conn); - return(0); - -done: - xmlMutexUnlock(conn->hashes_mux); - return(ret); + return virFreeDomainInternal(conn, domain, 1); } /** -------------------------------------------------------------------------------- In message <200706150931.BAD60924.2E73K9NG(a)aa.jp.fujitsu.com&gt; "Re: [Libvir] [PATCH] Do check the UUID in __virGetDomain()" "Masayuki Sunou <fj1826dm(a)aa.jp.fujitsu.com&gt;&quot; wrote:

Hi Rich, Dan Thank you for a reviewing and detailed suggestion. I understand your suggestion as follows and remake a patch. - The reason that the virDomain object is not cleared is the garbage collector does not work well. And its cause is virsh.c misses the call of virDomainFree(). This patch fixes virDomainFree() and virNetworkFree() which virsh.c misses the call. However, I detect the problem that resembles this happens in virt-manager, and I can not fix it. Therefore I register it to Bugziila. https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=246198 Thanks, Masayuki Sunou ---------------------------------------------------------------------- Index: src/virsh.c =================================================================== RCS file: /data/cvs/libvirt/src/virsh.c,v retrieving revision 1.89 diff -u -p -r1.89 virsh.c --- src/virsh.c 26 Jun 2007 11:42:46 -0000 1.89 +++ src/virsh.c 29 Jun 2007 06:40:58 -0000 @@ -382,6 +382,7 @@ cmdAutostart(vshControl * ctl, vshCmd * else vshPrint(ctl, _("Domain %s unmarked as autostarted\n"), name); + virDomainFree(dom); return TRUE; } @@ -798,6 +799,7 @@ cmdCreate(vshControl * ctl, vshCmd * cmd if (dom != NULL) { vshPrint(ctl, _("Domain %s created from %s\n"), virDomainGetName(dom), from); + virDomainFree(dom); } else { vshError(ctl, FALSE, _("Failed to create domain from %s"), from); ret = FALSE; @@ -845,6 +847,7 @@ cmdDefine(vshControl * ctl, vshCmd * cmd if (dom != NULL) { vshPrint(ctl, _("Domain %s defined from %s\n"), virDomainGetName(dom), from); + virDomainFree(dom); } else { vshError(ctl, FALSE, _("Failed to define domain from %s"), from); ret = FALSE; @@ -887,6 +890,7 @@ cmdUndefine(vshControl * ctl, vshCmd * c ret = FALSE; } + virDomainFree(dom); return ret; } @@ -920,6 +924,7 @@ cmdStart(vshControl * ctl, vshCmd * cmd) if (virDomainGetID(dom) != (unsigned int)-1) { vshError(ctl, FALSE, _("Domain is already active")); + virDomainFree(dom); return FALSE; } @@ -931,6 +936,7 @@ cmdStart(vshControl * ctl, vshCmd * cmd) virDomainGetName(dom)); ret = FALSE; } + virDomainFree(dom); return ret; } @@ -1026,7 +1032,10 @@ cmdSchedinfo(vshControl * ctl, vshCmd * if (capfound) nr_inputparams++; params = vshMalloc(ctl, sizeof (virSchedParameter) * nr_inputparams); - if (params == NULL) return FALSE; + if (params == NULL){ + virDomainFree(dom); + return FALSE; + } if (weightfound) { strncpy(params[inputparams].field,str_weight,sizeof(str_weight)); @@ -1048,7 +1057,10 @@ cmdSchedinfo(vshControl * ctl, vshCmd * /* Set SchedulerParameters */ if (inputparams > 0) { ret = virDomainSetSchedulerParameters(dom, params, inputparams); - if (ret == -1) return FALSE; + if (ret == -1){ + virDomainFree(dom); + return FALSE; + } } free(params); @@ -1060,6 +1072,7 @@ cmdSchedinfo(vshControl * ctl, vshCmd * free(schedulertype); } else { vshPrint(ctl, "%-15s: %s\n", _("Scheduler"), _("Unknown")); + virDomainFree(dom); return FALSE; } @@ -1070,7 +1083,10 @@ cmdSchedinfo(vshControl * ctl, vshCmd * memset (params[i].field, 0, sizeof params[i].field); } ret = virDomainGetSchedulerParameters(dom, params, &nparams); - if (ret == -1) return FALSE; + if (ret == -1){ + virDomainFree(dom); + return FALSE; + } if(nparams){ for (i = 0; i < nparams; i++){ switch (params[i].type) { @@ -1098,6 +1114,7 @@ cmdSchedinfo(vshControl * ctl, vshCmd * } } free(params); + virDomainFree(dom); return TRUE; ---------------------------------------------------------------------- In message <467F99A0.8040806(a)redhat.com&gt; "Re: [Libvir] [PATCH] Do check the UUID in __virGetDomain()" ""Richard W.M. Jones" <rjones(a)redhat.com&gt;&quot; wrote: