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

[Masayuki Sunou]

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 at redhat.com>
   "Re: [Libvir] [PATCH] Do check the UUID in __virGetDomain()"
   ""Richard W.M. Jones" <rjones at 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
>