[Libvir] [PATCH] Do check the UUID in __virGetDomain()
Hi This patch adds checking the UUID in __virGetDomain(). Now, the UUID of domain is wrong in the following operations. 1. Start virsh in interactive mode. 2. Execute domuuid to the domain 3. Execute undefine to the domain which executed domuuid in 2. 4. Create the domain whose name is same as the domain that executed undefine. 5. Execute domuuid for the new domain This patch can fix it. Signed-off-by: Masayuki Sunou <fj1826dm@aa.jp.fujitsu.com> 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 7 Jun 2007 02:08:41 -0000 @@ -758,8 +758,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 (virFreeDomain(conn, ret, 0) < 0) { + xmlMutexUnlock(conn->hashes_mux); + return(NULL); + } + } else { + goto done; + } } /* @@ -813,7 +819,7 @@ error: * Returns the reference count or -1 in case of failure. */ int -virFreeDomain(virConnectPtr conn, virDomainPtr domain) { +virFreeDomain(virConnectPtr conn, virDomainPtr domain, int lock) { int ret = 0; if ((!VIR_IS_CONNECT(conn)) || (!VIR_IS_CONNECTED_DOMAIN(domain)) || @@ -821,7 +827,8 @@ virFreeDomain(virConnectPtr conn, virDom virHashError(conn, VIR_ERR_INVALID_ARG, __FUNCTION__); return(-1); } - xmlMutexLock(conn->hashes_mux); + if (lock) + xmlMutexLock(conn->hashes_mux); /* * decrement the count for the domain @@ -853,13 +860,14 @@ virFreeDomain(virConnectPtr conn, virDom if (conn->domains != NULL) virHashFree(conn->domains, (virHashDeallocator) virDomainFreeName); - if (conn->hashes_mux != NULL) + if (conn->hashes_mux != NULL && lock) xmlFreeMutex(conn->hashes_mux); free(conn); return(0); done: - xmlMutexUnlock(conn->hashes_mux); + if (lock) + xmlMutexUnlock(conn->hashes_mux); return(ret); } Index: src/internal.h =================================================================== RCS file: /data/cvs/libvirt/src/internal.h,v retrieving revision 1.41 diff -u -p -r1.41 internal.h --- src/internal.h 8 May 2007 10:53:27 -0000 1.41 +++ src/internal.h 7 Jun 2007 02:08:41 -0000 @@ -202,7 +202,7 @@ virDomainPtr __virGetDomain (virConnectP const char *name, const unsigned char *uuid); int virFreeDomain (virConnectPtr conn, - virDomainPtr domain); + virDomainPtr domain, int lock); virDomainPtr virGetDomainByID(virConnectPtr conn, int id); virNetworkPtr __virGetNetwork (virConnectPtr conn, Index: src/libvirt.c =================================================================== RCS file: /data/cvs/libvirt/src/libvirt.c,v retrieving revision 1.73 diff -u -p -r1.73 libvirt.c --- src/libvirt.c 5 Jun 2007 12:06:08 -0000 1.73 +++ src/libvirt.c 7 Jun 2007 02:08:41 -0000 @@ -730,7 +730,7 @@ virDomainFree(virDomainPtr domain) virLibDomainError(domain, VIR_ERR_INVALID_DOMAIN, __FUNCTION__); return (-1); } - if (virFreeDomain(domain->conn, domain) < 0) + if (virFreeDomain(domain->conn, domain, 1) < 0) return (-1); return(0); } Index: src/qemu_internal.c =================================================================== RCS file: /data/cvs/libvirt/src/qemu_internal.c,v retrieving revision 1.25 diff -u -p -r1.25 qemu_internal.c --- src/qemu_internal.c 5 Jun 2007 12:06:08 -0000 1.25 +++ src/qemu_internal.c 7 Jun 2007 02:08:42 -0000 @@ -906,7 +906,7 @@ static int qemuUndefine(virDomainPtr dom } cleanup: - if (virFreeDomain(dom->conn, dom) < 0) + if (virFreeDomain(dom->conn, dom, 1) < 0) ret = -1; return ret; Index: src/xend_internal.c =================================================================== RCS file: /data/cvs/libvirt/src/xend_internal.c,v retrieving revision 1.116 diff -u -p -r1.116 xend_internal.c --- src/xend_internal.c 5 Jun 2007 12:06:09 -0000 1.116 +++ src/xend_internal.c 7 Jun 2007 02:08:42 -0000 @@ -1900,7 +1900,7 @@ error: virXendError(conn, VIR_ERR_INTERNAL_ERROR, _("failed to parse Xend domain information")); if (ret != NULL) - virFreeDomain(conn, ret); + virFreeDomain(conn, ret, 1); return(NULL); } #endif /* !PROXY */ @@ -3055,7 +3055,7 @@ xenDaemonCreateLinux(virConnectPtr conn, /* Make sure we don't leave a still-born domain around */ if (dom != NULL) { xenDaemonDomainDestroy(dom); - virFreeDomain(dom->conn, dom); + virFreeDomain(dom->conn, dom, 1); } if (name != NULL) free(name);
On Thu, Jun 07, 2007 at 03:03:11PM +0900, Masayuki Sunou wrote:
Hi
This patch adds checking the UUID in __virGetDomain().
Now, the UUID of domain is wrong in the following operations.
1. Start virsh in interactive mode. 2. Execute domuuid to the domain 3. Execute undefine to the domain which executed domuuid in 2. 4. Create the domain whose name is same as the domain that executed undefine. 5. Execute domuuid for the new domain
Hmm, I strongly suspect one (or more) of the commands in this series of steps is missing a call for virDomainFree(). Every individual virsh command should be freeing all the objects it has open (aside fromthe virConnectPtr), so the cache of virDomainPtr objects ought to be empty for every individual command. The UUID chcek is still sensible, but we need to find which virsh command is not freeing objects too. Dan. -- |=- Red Hat, Engineering, Emerging Technologies, Boston. +1 978 392 2496 -=| |=- Perl modules: http://search.cpan.org/~danberr/ -=| |=- Projects: http://freshmeat.net/~danielpb/ -=| |=- GnuPG: 7D3B9505 F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 -=|
Hi Dan
Hmm, I strongly suspect one (or more) of the commands in this series of steps is missing a call for virDomainFree(). Every individual virsh command should be freeing all the objects it has open (aside fromthe virConnectPtr), so the cache of virDomainPtr objects ought to be empty for every individual command.
I think that it is not bad to have cache to make performance better. But I think that it is necessary to control it correctly.
The UUID chcek is still sensible, but we need to find which virsh command is not freeing objects too.
I think that we should remove the missing of freeing the object one by one. Thanks, Masayuki Sunou In message <20070607111427.GA12398@redhat.com> "Re: [Libvir] [PATCH] Do check the UUID in __virGetDomain()" ""Daniel P. Berrange" <berrange@redhat.com>" wrote:
On Thu, Jun 07, 2007 at 03:03:11PM +0900, Masayuki Sunou wrote:
Hi
This patch adds checking the UUID in __virGetDomain().
Now, the UUID of domain is wrong in the following operations.
1. Start virsh in interactive mode. 2. Execute domuuid to the domain 3. Execute undefine to the domain which executed domuuid in 2. 4. Create the domain whose name is same as the domain that executed undefine. 5. Execute domuuid for the new domain
Hmm, I strongly suspect one (or more) of the commands in this series of steps is missing a call for virDomainFree(). Every individual virsh command should be freeing all the objects it has open (aside fromthe virConnectPtr), so the cache of virDomainPtr objects ought to be empty for every individual command.
The UUID chcek is still sensible, but we need to find which virsh command is not freeing objects too.
Dan. -- |=- Red Hat, Engineering, Emerging Technologies, Boston. +1 978 392 2496 -=| |=- Perl modules: http://search.cpan.org/~danberr/ -=| |=- Projects: http://freshmeat.net/~danielpb/ -=| |=- GnuPG: 7D3B9505 F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 -=|
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@aa.jp.fujitsu.com> "Re: [Libvir] [PATCH] Do check the UUID in __virGetDomain()" "Masayuki Sunou <fj1826dm@aa.jp.fujitsu.com>" wrote:
Hi Dan
Hmm, I strongly suspect one (or more) of the commands in this series of steps is missing a call for virDomainFree(). Every individual virsh command should be freeing all the objects it has open (aside fromthe virConnectPtr), so the cache of virDomainPtr objects ought to be empty for every individual command.
I think that it is not bad to have cache to make performance better. But I think that it is necessary to control it correctly.
The UUID chcek is still sensible, but we need to find which virsh command is not freeing objects too.
I think that we should remove the missing of freeing the object one by one.
Thanks, Masayuki Sunou
In message <20070607111427.GA12398@redhat.com> "Re: [Libvir] [PATCH] Do check the UUID in __virGetDomain()" ""Daniel P. Berrange" <berrange@redhat.com>" wrote:
On Thu, Jun 07, 2007 at 03:03:11PM +0900, Masayuki Sunou wrote:
Hi
This patch adds checking the UUID in __virGetDomain().
Now, the UUID of domain is wrong in the following operations.
1. Start virsh in interactive mode. 2. Execute domuuid to the domain 3. Execute undefine to the domain which executed domuuid in 2. 4. Create the domain whose name is same as the domain that executed undefine. 5. Execute domuuid for the new domain
Hmm, I strongly suspect one (or more) of the commands in this series of steps is missing a call for virDomainFree(). Every individual virsh command should be freeing all the objects it has open (aside fromthe virConnectPtr), so the cache of virDomainPtr objects ought to be empty for every individual command.
The UUID chcek is still sensible, but we need to find which virsh command is not freeing objects too.
Dan. -- |=- Red Hat, Engineering, Emerging Technologies, Boston. +1 978 392 2496 -=| |=- Perl modules: http://search.cpan.org/~danberr/ -=| |=- Projects: http://freshmeat.net/~danielpb/ -=| |=- GnuPG: 7D3B9505 F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 -=|
-- Libvir-list mailing list Libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Masayuki Sunou wrote:
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.
Hello Masayuki, I think you're misunderstanding the original problem. It looks like src/virsh.c is missing (many) calls to virDomainFree. If we go back to your original email:
1. Start virsh in interactive mode. 2. Execute domuuid to the domain 3. Execute undefine to the domain which executed domuuid in 2. 4. Create the domain whose name is same as the domain that executed undefine. 5. Execute domuuid for the new domain
Now compare this to what src/virsh.c is doing for each of these calls: 2. In cmdDomuuid, we get a new virDomain object. This is allocated in src/hash.c. dom->uses == 1 3. In cmdUndefine, we do another virDomainsLookupBy* function. This returns the same domain object as step (2), but now dom->uses == 2. 4. In cmdDefine, we call virDomainDefineXML. Since this has the same name (see src/hash.c: __virGetDomain), it returns the same domain object as step (2) & (3), but now dom->uses == 3. etc. So I think we can see the problem: Only some of the command functions in src/virsh.c are freeing the domain object correctly. Some of them do it correctly (cmdResume), others not at all (cmdDomuuid), and no doubt there are some which only get it partially right because they don't consider error cases properly. I'm sure naysayers will be glad to know that the mlvirsh (OCaml virsh) which I wrote over the weekend also didn't get this right, although the solution there was simpler: just invoke the garbage collector manually between each interactive command so that unreachable domains are freed. (A finaliser was already in place to call virDomainFree, but with garbage collectors you can never be sure when finalisers will run. What we need are deterministic finalizers linked to object scope but no one's quite worked out how to implement that efficiently). Rich. -- Emerging Technologies, Red Hat - http://et.redhat.com/~rjones/ Registered Address: Red Hat UK Ltd, Amberley Place, 107-111 Peascod Street, Windsor, Berkshire, SL4 1TE, United Kingdom. Registered in England and Wales under Company Registration No. 03798903
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@redhat.com> "Re: [Libvir] [PATCH] Do check the UUID in __virGetDomain()" ""Richard W.M. Jones" <rjones@redhat.com>" wrote:
Masayuki Sunou wrote:
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.
Hello Masayuki,
I think you're misunderstanding the original problem. It looks like src/virsh.c is missing (many) calls to virDomainFree.
If we go back to your original email:
1. Start virsh in interactive mode. 2. Execute domuuid to the domain 3. Execute undefine to the domain which executed domuuid in 2. 4. Create the domain whose name is same as the domain that executed undefine. 5. Execute domuuid for the new domain
Now compare this to what src/virsh.c is doing for each of these calls:
2. In cmdDomuuid, we get a new virDomain object. This is allocated in src/hash.c. dom->uses == 1
3. In cmdUndefine, we do another virDomainsLookupBy* function. This returns the same domain object as step (2), but now dom->uses == 2.
4. In cmdDefine, we call virDomainDefineXML. Since this has the same name (see src/hash.c: __virGetDomain), it returns the same domain object as step (2) & (3), but now dom->uses == 3.
etc.
So I think we can see the problem: Only some of the command functions in src/virsh.c are freeing the domain object correctly. Some of them do it correctly (cmdResume), others not at all (cmdDomuuid), and no doubt there are some which only get it partially right because they don't consider error cases properly.
I'm sure naysayers will be glad to know that the mlvirsh (OCaml virsh) which I wrote over the weekend also didn't get this right, although the solution there was simpler: just invoke the garbage collector manually between each interactive command so that unreachable domains are freed. (A finaliser was already in place to call virDomainFree, but with garbage collectors you can never be sure when finalisers will run. What we need are deterministic finalizers linked to object scope but no one's quite worked out how to implement that efficiently).
Rich.
-- Emerging Technologies, Red Hat - http://et.redhat.com/~rjones/ Registered Address: Red Hat UK Ltd, Amberley Place, 107-111 Peascod Street, Windsor, Berkshire, SL4 1TE, United Kingdom. Registered in England and Wales under Company Registration No. 03798903
On Fri, Jun 29, 2007 at 05:36:44PM +0900, Masayuki Sunou 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.
That looks like the right fixes to me, thanks a lot !
However, I detect the problem that resembles this happens in virt-manager, and I can not fix it. Therefore I register it to Bugziila.
Hum, I remember looking at this a long time ago, we had the same effect (and IIRC that was because the update of the state change in xenstored was too slow, so there was a conflict there), but now that could be something completely different with the same effect. Daniel -- Red Hat Virtualization group http://redhat.com/virtualization/ Daniel Veillard | virtualization library http://libvirt.org/ veillard@redhat.com | libxml GNOME XML XSLT toolkit http://xmlsoft.org/ http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/
Masayuki Sunou wrote:
Hi Rich, Dan
+1. Rich. -- Emerging Technologies, Red Hat - http://et.redhat.com/~rjones/ Registered Address: Red Hat UK Ltd, Amberley Place, 107-111 Peascod Street, Windsor, Berkshire, SL4 1TE, United Kingdom. Registered in England and Wales under Company Registration No. 03798903
On Fri, Jun 29, 2007 at 05:36:44PM +0900, Masayuki Sunou 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.
Okay I applied the patch and commited, (well actually I made the chnage by hand because the transmission corrupted the patch please use attachments :-) thanks a lot ! Daniel -- Red Hat Virtualization group http://redhat.com/virtualization/ Daniel Veillard | virtualization library http://libvirt.org/ veillard@redhat.com | libxml GNOME XML XSLT toolkit http://xmlsoft.org/ http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/
participants (4)
-
Daniel P. Berrange -
Daniel Veillard -
Masayuki Sunou -
Richard W.M. Jones