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

Re: [Libvir] Remote daemon & virDomainFree interaction



On Tue, Jul 24, 2007 at 02:38:40AM +0100, Daniel P. Berrange wrote:
> In looking at a problem with domain object cleanup in virt-manager I came 
> across a problem in the remote driver, well the internal driver API itself
> actually. Specifically the implmenetation of virDomainFree() never calls
> into the driver API - it simply uses virFreeDomain() release the memory
> associated with the virDomainPtr object. 
> 
> Couple this with the remote driver though, and virDomainPtr objects in the
> remote daemon never get released, because the virDomainFree call is never
> propagated over the wire to the server.
> 
> Its quite easy to see this in practice. Simply add a printf to the impl
> of virDomainLookupByName which prints out the ref count. Then run either
> virsh or virt-manager for a while
> 
>   Get info QEMUGuest1 69 c7a5fdbd-edaf-9455-926a-d65c16db1809
>   Get info QEMUGuest1 70 c7a5fdbd-edaf-9455-926a-d65c16db1809
>   Get info QEMUGuest1 71 c7a5fdbd-edaf-9455-926a-d65c16db1809
> 
> We need to make virDomainFree call into the driver API, and also make sure
> that the remote driver implements it.

Actually I was wrong in this. It is not just that the virDomainLookupByName 
which cause virDomainPtr objects to be allocated. Every single API call into
the daemon which uses get_nonnull_domain() causes a virDomainPtr object
to have its ref count increased. So we don't want to pass the virDomainFree
API through the driver API at all.

Instead in the server side of the remote driver, any API call which returns 
a virDomainPtr *and* any call which accepts a virDomainPtr needs to have a
corresponding call to virDomainFree to reduce the ref-count in all of its
exit paths. The same goes for virNetwork objects & its calls.

I'm attaching a patch which I believe hits all the neccessary codepaths.
With a few prints in virGetDomain, I can see the refcount correctly being
incremented & decremented per API call, rather than growing without bound.

Regards,
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  -=| 
Index: qemud/remote.c
===================================================================
RCS file: /data/cvs/libvirt/qemud/remote.c,v
retrieving revision 1.5
diff -u -p -r1.5 remote.c
--- qemud/remote.c	26 Jun 2007 23:48:47 -0000	1.5
+++ qemud/remote.c	24 Jul 2007 02:33:17 -0000
@@ -545,10 +545,14 @@ remoteDispatchDomainGetSchedulerType (st
     }
 
     type = virDomainGetSchedulerType (dom, &nparams);
-    if (type == NULL) return -1;
+    if (type == NULL) {
+        virDomainFree(dom);
+        return -1;
+    }
 
     ret->type = type;
     ret->nparams = nparams;
+    virDomainFree(dom);
     return 0;
 }
 
@@ -584,6 +588,7 @@ remoteDispatchDomainGetSchedulerParamete
 
     r = virDomainGetSchedulerParameters (dom, params, &nparams);
     if (r == -1) {
+        virDomainFree(dom);
         free (params);
         return -1;
     }
@@ -593,6 +598,7 @@ remoteDispatchDomainGetSchedulerParamete
     ret->params.params_val = malloc (sizeof (struct remote_sched_param)
                                      * nparams);
     if (ret->params.params_val == NULL) {
+        virDomainFree(dom);
         free (params);
         remoteDispatchError (client, req,
                              "out of memory allocating return array");
@@ -603,6 +609,7 @@ remoteDispatchDomainGetSchedulerParamete
         // remoteDispatchClientRequest will free this:
         ret->params.params_val[i].field = strdup (params[i].field);
         if (ret->params.params_val[i].field == NULL) {
+            virDomainFree(dom);
             free (params);
             remoteDispatchError (client, req,
                                  "out of memory allocating return array");
@@ -623,11 +630,13 @@ remoteDispatchDomainGetSchedulerParamete
         case VIR_DOMAIN_SCHED_FIELD_BOOLEAN:
             ret->params.params_val[i].value.remote_sched_param_value_u.b = params[i].value.b; break;
         default:
+            virDomainFree(dom);
             free (params);
             remoteDispatchError (client, req, "unknown type");
             return -2;
         }
     }
+    virDomainFree(dom);
     free (params);
 
     return 0;
@@ -686,6 +695,7 @@ remoteDispatchDomainSetSchedulerParamete
     }
 
     r = virDomainSetSchedulerParameters (dom, params, nparams);
+    virDomainFree(dom);
     free (params);
     if (r == -1) return -1;
 
@@ -707,9 +717,11 @@ remoteDispatchDomainAttachDevice (struct
         return -2;
     }
 
-    if (virDomainAttachDevice (dom, args->xml) == -1)
+    if (virDomainAttachDevice (dom, args->xml) == -1) {
+        virDomainFree(dom);
         return -1;
-
+    }
+    virDomainFree(dom);
     return 0;
 }
 
@@ -728,9 +740,11 @@ remoteDispatchDomainCreate (struct qemud
         return -2;
     }
 
-    if (virDomainCreate (dom) == -1)
+    if (virDomainCreate (dom) == -1) {
+        virDomainFree(dom);
         return -1;
-
+    }
+    virDomainFree(dom);
     return 0;
 }
 
@@ -747,6 +761,7 @@ remoteDispatchDomainCreateLinux (struct 
     if (dom == NULL) return -1;
 
     make_nonnull_domain (&ret->dom, dom);
+    virDomainFree(dom);
 
     return 0;
 }
@@ -764,6 +779,7 @@ remoteDispatchDomainDefineXml (struct qe
     if (dom == NULL) return -1;
 
     make_nonnull_domain (&ret->dom, dom);
+    virDomainFree(dom);
 
     return 0;
 }
@@ -785,7 +801,7 @@ remoteDispatchDomainDestroy (struct qemu
 
     if (virDomainDestroy (dom) == -1)
         return -1;
-
+    /* No need to free dom - destroy does it for us */
     return 0;
 }
 
@@ -804,9 +820,12 @@ remoteDispatchDomainDetachDevice (struct
         return -2;
     }
 
-    if (virDomainDetachDevice (dom, args->xml) == -1)
+    if (virDomainDetachDevice (dom, args->xml) == -1) {
+        virDomainFree(dom);
         return -1;
+    }
 
+    virDomainFree(dom);
     return 0;
 }
 
@@ -827,8 +846,11 @@ remoteDispatchDomainDumpXml (struct qemu
 
     /* remoteDispatchClientRequest will free this. */
     ret->xml = virDomainGetXMLDesc (dom, args->flags);
-    if (!ret->xml) return -1;
-
+    if (!ret->xml) {
+            virDomainFree(dom);
+            return -1;
+    }
+    virDomainFree(dom);
     return 0;
 }
 
@@ -847,9 +869,11 @@ remoteDispatchDomainGetAutostart (struct
         return -2;
     }
 
-    if (virDomainGetAutostart (dom, &ret->autostart) == -1)
+    if (virDomainGetAutostart (dom, &ret->autostart) == -1) {
+        virDomainFree(dom);
         return -1;
-
+    }
+    virDomainFree(dom);
     return 0;
 }
 
@@ -869,8 +893,10 @@ remoteDispatchDomainGetInfo (struct qemu
         return -2;
     }
 
-    if (virDomainGetInfo (dom, &info) == -1)
+    if (virDomainGetInfo (dom, &info) == -1) {
+        virDomainFree(dom);
         return -1;
+    }
 
     ret->state = info.state;
     ret->max_mem = info.maxMem;
@@ -878,6 +904,8 @@ remoteDispatchDomainGetInfo (struct qemu
     ret->nr_virt_cpu = info.nrVirtCpu;
     ret->cpu_time = info.cpuTime;
 
+    virDomainFree(dom);
+
     return 0;
 }
 
@@ -897,8 +925,11 @@ remoteDispatchDomainGetMaxMemory (struct
     }
 
     ret->memory = virDomainGetMaxMemory (dom);
-    if (ret->memory == 0) return -1;
-
+    if (ret->memory == 0) {
+        virDomainFree(dom);
+        return -1;
+    }
+    virDomainFree(dom);
     return 0;
 }
 
@@ -918,8 +949,11 @@ remoteDispatchDomainGetMaxVcpus (struct 
     }
 
     ret->num = virDomainGetMaxVcpus (dom);
-    if (ret->num == -1) return -1;
-
+    if (ret->num == -1) {
+        virDomainFree(dom);
+        return -1;
+    }
+    virDomainFree(dom);
     return 0;
 }
 
@@ -940,8 +974,11 @@ remoteDispatchDomainGetOsType (struct qe
 
     /* remoteDispatchClientRequest will free this */
     ret->type = virDomainGetOSType (dom);
-    if (ret->type == NULL) return -1;
-
+    if (ret->type == NULL) {
+            virDomainFree(dom);
+            return -1;
+    }
+    virDomainFree(dom);
     return 0;
 }
 
@@ -964,11 +1001,13 @@ remoteDispatchDomainGetVcpus (struct qem
     }
 
     if (args->maxinfo > REMOTE_VCPUINFO_MAX) {
+        virDomainFree(dom);
         remoteDispatchError (client, req, "maxinfo > REMOTE_VCPUINFO_MAX");
         return -2;
     }
 
     if (args->maxinfo * args->maplen > REMOTE_CPUMAPS_MAX) {
+        virDomainFree(dom);
         remoteDispatchError (client, req, "maxinfo * maplen > REMOTE_CPUMAPS_MAX");
         return -2;
     }
@@ -980,7 +1019,10 @@ remoteDispatchDomainGetVcpus (struct qem
     info_len = virDomainGetVcpus (dom,
                                   info, args->maxinfo,
                                   cpumaps, args->maplen);
-    if (info_len == -1) return -1;
+    if (info_len == -1) {
+        virDomainFree(dom);
+        return -1;
+    }
 
     /* Allocate the return buffer for info. */
     ret->info.info_len = info_len;
@@ -1000,6 +1042,7 @@ remoteDispatchDomainGetVcpus (struct qem
     ret->cpumaps.cpumaps_len = args->maxinfo * args->maplen;
     ret->cpumaps.cpumaps_val = (char *) cpumaps;
 
+    virDomainFree(dom);
     return 0;
 }
 
@@ -1041,7 +1084,7 @@ remoteDispatchDomainLookupById (struct q
     if (dom == NULL) return -1;
 
     make_nonnull_domain (&ret->dom, dom);
-
+    virDomainFree(dom);
     return 0;
 }
 
@@ -1058,7 +1101,7 @@ remoteDispatchDomainLookupByName (struct
     if (dom == NULL) return -1;
 
     make_nonnull_domain (&ret->dom, dom);
-
+    virDomainFree(dom);
     return 0;
 }
 
@@ -1075,7 +1118,7 @@ remoteDispatchDomainLookupByUuid (struct
     if (dom == NULL) return -1;
 
     make_nonnull_domain (&ret->dom, dom);
-
+    virDomainFree(dom);
     return 0;
 }
 
@@ -1110,6 +1153,7 @@ remoteDispatchDomainPinVcpu (struct qemu
     }
 
     if (args->cpumap.cpumap_len > REMOTE_CPUMAP_MAX) {
+        virDomainFree(dom);
         remoteDispatchError (client, req, "cpumap_len > REMOTE_CPUMAP_MAX");
         return -2;
     }
@@ -1117,8 +1161,11 @@ remoteDispatchDomainPinVcpu (struct qemu
     rv = virDomainPinVcpu (dom, args->vcpu,
                            (unsigned char *) args->cpumap.cpumap_val,
                            args->cpumap.cpumap_len);
-    if (rv == -1) return -1;
-
+    if (rv == -1) {
+        virDomainFree(dom);
+        return -1;
+    }
+    virDomainFree(dom);
     return 0;
 }
 
@@ -1137,9 +1184,11 @@ remoteDispatchDomainReboot (struct qemud
         return -2;
     }
 
-    if (virDomainReboot (dom, args->flags) == -1)
+    if (virDomainReboot (dom, args->flags) == -1) {
+        virDomainFree(dom);
         return -1;
-
+    }
+    virDomainFree(dom);
     return 0;
 }
 
@@ -1172,9 +1221,11 @@ remoteDispatchDomainResume (struct qemud
         return -2;
     }
 
-    if (virDomainResume (dom) == -1)
+    if (virDomainResume (dom) == -1) {
+        virDomainFree(dom);
         return -1;
-
+    }
+    virDomainFree(dom);
     return 0;
 }
 
@@ -1193,9 +1244,11 @@ remoteDispatchDomainSave (struct qemud_c
         return -2;
     }
 
-    if (virDomainSave (dom, args->to) == -1)
+    if (virDomainSave (dom, args->to) == -1) {
+        virDomainFree(dom);
         return -1;
-
+    }
+    virDomainFree(dom);
     return 0;
 }
 
@@ -1214,9 +1267,11 @@ remoteDispatchDomainCoreDump (struct qem
         return -2;
     }
 
-    if (virDomainCoreDump (dom, args->to, args->flags) == -1)
+    if (virDomainCoreDump (dom, args->to, args->flags) == -1) {
+        virDomainFree(dom);
         return -1;
-
+    }
+    virDomainFree(dom);
     return 0;
 }
 
@@ -1235,9 +1290,11 @@ remoteDispatchDomainSetAutostart (struct
         return -2;
     }
 
-    if (virDomainSetAutostart (dom, args->autostart) == -1)
+    if (virDomainSetAutostart (dom, args->autostart) == -1) {
+        virDomainFree(dom);
         return -1;
-
+    }
+    virDomainFree(dom);
     return 0;
 }
 
@@ -1256,9 +1313,11 @@ remoteDispatchDomainSetMaxMemory (struct
         return -2;
     }
 
-    if (virDomainSetMaxMemory (dom, args->memory) == -1)
+    if (virDomainSetMaxMemory (dom, args->memory) == -1) {
+        virDomainFree(dom);
         return -1;
-
+    }
+    virDomainFree(dom);
     return 0;
 }
 
@@ -1277,9 +1336,11 @@ remoteDispatchDomainSetMemory (struct qe
         return -2;
     }
 
-    if (virDomainSetMemory (dom, args->memory) == -1)
+    if (virDomainSetMemory (dom, args->memory) == -1) {
+        virDomainFree(dom);
         return -1;
-
+    }
+    virDomainFree(dom);
     return 0;
 }
 
@@ -1298,9 +1359,11 @@ remoteDispatchDomainSetVcpus (struct qem
         return -2;
     }
 
-    if (virDomainSetVcpus (dom, args->nvcpus) == -1)
+    if (virDomainSetVcpus (dom, args->nvcpus) == -1) {
+        virDomainFree(dom);
         return -1;
-
+    }
+    virDomainFree(dom);
     return 0;
 }
 
@@ -1319,9 +1382,11 @@ remoteDispatchDomainShutdown (struct qem
         return -2;
     }
 
-    if (virDomainShutdown (dom) == -1)
+    if (virDomainShutdown (dom) == -1) {
+        virDomainFree(dom);
         return -1;
-
+    }
+    virDomainFree(dom);
     return 0;
 }
 
@@ -1340,9 +1405,11 @@ remoteDispatchDomainSuspend (struct qemu
         return -2;
     }
 
-    if (virDomainSuspend (dom) == -1)
+    if (virDomainSuspend (dom) == -1) {
+        virDomainFree(dom);
         return -1;
-
+    }
+    virDomainFree(dom);
     return 0;
 }
 
@@ -1361,9 +1428,11 @@ remoteDispatchDomainUndefine (struct qem
         return -2;
     }
 
-    if (virDomainUndefine (dom) == -1)
+    if (virDomainUndefine (dom) == -1) {
+        virDomainFree(dom);
         return -1;
-
+    }
+    virDomainFree(dom);
     return 0;
 }
 
@@ -1456,9 +1525,11 @@ remoteDispatchNetworkCreate (struct qemu
         return -2;
     }
 
-    if (virNetworkCreate (net) == -1)
+    if (virNetworkCreate (net) == -1) {
+        virNetworkFree(net);
         return -1;
-
+    }
+    virNetworkFree(net);
     return 0;
 }
 
@@ -1475,7 +1546,7 @@ remoteDispatchNetworkCreateXml (struct q
     if (net == NULL) return -1;
 
     make_nonnull_network (&ret->net, net);
-
+    virNetworkFree(net);
     return 0;
 }
 
@@ -1492,7 +1563,7 @@ remoteDispatchNetworkDefineXml (struct q
     if (net == NULL) return -1;
 
     make_nonnull_network (&ret->net, net);
-
+    virNetworkFree(net);
     return 0;
 }
 
@@ -1511,9 +1582,11 @@ remoteDispatchNetworkDestroy (struct qem
         return -2;
     }
 
-    if (virNetworkDestroy (net) == -1)
+    if (virNetworkDestroy (net) == -1) {
+        virNetworkFree(net);
         return -1;
-
+    }
+    virNetworkFree(net);
     return 0;
 }
 
@@ -1534,8 +1607,11 @@ remoteDispatchNetworkDumpXml (struct qem
 
     /* remoteDispatchClientRequest will free this. */
     ret->xml = virNetworkGetXMLDesc (net, args->flags);
-    if (!ret->xml) return -1;
-
+    if (!ret->xml) {
+        virNetworkFree(net);
+        return -1;
+    }
+    virNetworkFree(net);
     return 0;
 }
 
@@ -1554,9 +1630,11 @@ remoteDispatchNetworkGetAutostart (struc
         return -2;
     }
 
-    if (virNetworkGetAutostart (net, &ret->autostart) == -1)
+    if (virNetworkGetAutostart (net, &ret->autostart) == -1) {
+        virNetworkFree(net);
         return -1;
-
+    }
+    virNetworkFree(net);
     return 0;
 }
 
@@ -1577,8 +1655,11 @@ remoteDispatchNetworkGetBridgeName (stru
 
     /* remoteDispatchClientRequest will free this. */
     ret->name = virNetworkGetBridgeName (net);
-    if (!ret->name) return -1;
-
+    if (!ret->name) {
+        virNetworkFree(net);
+        return -1;
+    }
+    virNetworkFree(net);
     return 0;
 }
 
@@ -1595,7 +1676,7 @@ remoteDispatchNetworkLookupByName (struc
     if (net == NULL) return -1;
 
     make_nonnull_network (&ret->net, net);
-
+    virNetworkFree(net);
     return 0;
 }
 
@@ -1612,7 +1693,7 @@ remoteDispatchNetworkLookupByUuid (struc
     if (net == NULL) return -1;
 
     make_nonnull_network (&ret->net, net);
-
+    virNetworkFree(net);
     return 0;
 }
 
@@ -1631,9 +1712,11 @@ remoteDispatchNetworkSetAutostart (struc
         return -2;
     }
 
-    if (virNetworkSetAutostart (net, args->autostart) == -1)
+    if (virNetworkSetAutostart (net, args->autostart) == -1) {
+        virNetworkFree(net);
         return -1;
-
+    }
+    virNetworkFree(net);
     return 0;
 }
 
@@ -1652,9 +1735,11 @@ remoteDispatchNetworkUndefine (struct qe
         return -2;
     }
 
-    if (virNetworkUndefine (net) == -1)
+    if (virNetworkUndefine (net) == -1) {
+        virNetworkFree(net);
         return -1;
-
+    }
+    virNetworkFree(net);
     return 0;
 }
 

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