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@xxxxxxxxxxxxxxxxx>
"Re: [PATCH] Do check the UUID in __virGetDomain()"
"Masayuki Sunou <fj1826dm@xxxxxxxxxxxxxxxxx>" 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@xxxxxxxxxx>
> "Re: [PATCH] Do check the UUID in __virGetDomain()"
> ""Daniel P. Berrange" <berrange@xxxxxxxxxx>" 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@xxxxxxxxxx
> https://www.redhat.com/mailman/listinfo/libvir-list
>