[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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



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
> 


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]