Re: [Libvir] [PATCH] Do check the UUID in __virGetDomain()

25 Jun 2007

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


    