libxl: dispose libxl_dominfo after libxl_domain_info()

Commit Message

Jim Fehlig Jan. 21, 2016, 1:41 a.m. UTC

Joao Martins wrote:
> As suggested in a previous thread [0] this patch adds some missing calls
> to libxl_dominfo_dispose when doing some of the libxl_domain_info
> operations which would otherwise lead to memory leaks.
> 
> [0]
> https://www.redhat.com/archives/libvir-list/2015-September/msg00519.html
> 
> Signed-off-by: Joao Martins <[e-mail]>
> ---
>  src/libxl/libxl_driver.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
> index 73ed448..a449730 100644
> --- a/src/libxl/libxl_driver.c
> +++ b/src/libxl/libxl_driver.c
> @@ -358,6 +358,8 @@ libxlReconnectDomain(virDomainObjPtr vm,
>  
>      virObjectLock(vm);
>  
> +    libxl_dominfo_init(&d_info);
> +
>      /* Does domain still exist? */
>      rc = libxl_domain_info(cfg->ctx, &d_info, vm->def->id);
>      if (rc == ERROR_INVAL) {
> @@ -389,11 +391,13 @@ libxlReconnectDomain(virDomainObjPtr vm,
>      /* Enable domain death events */
>      libxl_evenable_domain_death(cfg->ctx, vm->def->id, 0, &priv->deathW);
>  
> +    libxl_dominfo_dispose(&d_info);
>      virObjectUnlock(vm);
>      virObjectUnref(cfg);
>      return 0;
>  
>   out:
> +    libxl_dominfo_dispose(&d_info);
>      libxlDomainCleanup(driver, vm);
>      if (!vm->persistent)
>          virDomainObjListRemoveLocked(driver->domains, vm);
> @@ -1598,6 +1602,8 @@ libxlDomainGetInfo(virDomainPtr dom, virDomainInfoPtr info)
>          info->cpuTime = d_info.cpu_time;
>          info->memory = d_info.current_memkb;
>          info->maxMem = d_info.max_memkb;
> +
> +        libxl_dominfo_dispose(&d_info);

There should be a corresponding libxl_dominfo_init(). While looking at this, I
audited all uses of libxl_domain_info() and found another missing _init(). With
the below diff squashed in, I think this code is finally correct :-). Do you agree?

Regards,
Jim

Comments

Joao Martins Jan. 21, 2016, 10:21 a.m. UTC | #1

On 01/21/2016 01:41 AM, Jim Fehlig wrote:
> Joao Martins wrote:
>> As suggested in a previous thread [0] this patch adds some missing calls
>> to libxl_dominfo_dispose when doing some of the libxl_domain_info
>> operations which would otherwise lead to memory leaks.
>>
>> [0]
>> https://www.redhat.com/archives/libvir-list/2015-September/msg00519.html
>>
>> Signed-off-by: Joao Martins <[e-mail]>
>> ---
>>  src/libxl/libxl_driver.c | 6 ++++++
>>  1 file changed, 6 insertions(+)
>>
>> diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
>> index 73ed448..a449730 100644
>> --- a/src/libxl/libxl_driver.c
>> +++ b/src/libxl/libxl_driver.c
>> @@ -358,6 +358,8 @@ libxlReconnectDomain(virDomainObjPtr vm,
>>  
>>      virObjectLock(vm);
>>  
>> +    libxl_dominfo_init(&d_info);
>> +
>>      /* Does domain still exist? */
>>      rc = libxl_domain_info(cfg->ctx, &d_info, vm->def->id);
>>      if (rc == ERROR_INVAL) {
>> @@ -389,11 +391,13 @@ libxlReconnectDomain(virDomainObjPtr vm,
>>      /* Enable domain death events */
>>      libxl_evenable_domain_death(cfg->ctx, vm->def->id, 0, &priv->deathW);
>>  
>> +    libxl_dominfo_dispose(&d_info);
>>      virObjectUnlock(vm);
>>      virObjectUnref(cfg);
>>      return 0;
>>  
>>   out:
>> +    libxl_dominfo_dispose(&d_info);
>>      libxlDomainCleanup(driver, vm);
>>      if (!vm->persistent)
>>          virDomainObjListRemoveLocked(driver->domains, vm);
>> @@ -1598,6 +1602,8 @@ libxlDomainGetInfo(virDomainPtr dom, virDomainInfoPtr info)
>>          info->cpuTime = d_info.cpu_time;
>>          info->memory = d_info.current_memkb;
>>          info->maxMem = d_info.max_memkb;
>> +
>> +        libxl_dominfo_dispose(&d_info);
> 
> There should be a corresponding libxl_dominfo_init(). While looking at this, I
> audited all uses of libxl_domain_info() and found another missing _init(). With
> the below diff squashed in, I think this code is finally correct :-). Do you agree?
> 
Ah, missed the dominfo_init() calls. Yeah, I definitively agree. I just sent it
over v2.

Joao

> Regards,
> Jim
> 
> diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
> index 21c7610..4a9134e 100644
> --- a/src/libxl/libxl_driver.c
> +++ b/src/libxl/libxl_driver.c
> @@ -1593,6 +1593,8 @@ libxlDomainGetInfo(virDomainPtr dom, virDomainInfoPtr info)
>          info->memory = vm->def->mem.cur_balloon;
>          info->maxMem = virDomainDefGetMemoryActual(vm->def);
>      } else {
> +        libxl_dominfo_init(&d_info);
> +
>          if (libxl_domain_info(cfg->ctx, &d_info, vm->def->id) != 0) {
>              virReportError(VIR_ERR_INTERNAL_ERROR,
>                             _("libxl_domain_info failed for domain '%d'"),
> @@ -4797,6 +4799,7 @@ libxlDomainMemoryStats(virDomainPtr dom,
> 
>      virCheckFlags(0, -1);
> 
> +    libxl_dominfo_init(&d_info);
>      cfg = libxlDriverConfigGet(driver);
> 
>      if (!(vm = libxlDomObjFromDomain(dom)))
> @@ -4828,13 +4831,12 @@ libxlDomainMemoryStats(virDomainPtr dom,
> 
>      ret = i;
> 
> -    libxl_dominfo_dispose(&d_info);
> -
>   endjob:
>      if (!libxlDomainObjEndJob(driver, vm))
>          vm = NULL;
> 
>   cleanup:
> +    libxl_dominfo_dispose(&d_info);
>      if (vm)
>          virObjectUnlock(vm);
>      virObjectUnref(cfg);
>

Patch

diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
index 21c7610..4a9134e 100644
--- a/src/libxl/libxl_driver.c
+++ b/src/libxl/libxl_driver.c
@@ -1593,6 +1593,8 @@  libxlDomainGetInfo(virDomainPtr dom, virDomainInfoPtr info)
         info->memory = vm->def->mem.cur_balloon;
         info->maxMem = virDomainDefGetMemoryActual(vm->def);
     } else {
+        libxl_dominfo_init(&d_info);
+
         if (libxl_domain_info(cfg->ctx, &d_info, vm->def->id) != 0) {
             virReportError(VIR_ERR_INTERNAL_ERROR,
                            _("libxl_domain_info failed for domain '%d'"),
@@ -4797,6 +4799,7 @@  libxlDomainMemoryStats(virDomainPtr dom,

     virCheckFlags(0, -1);

+    libxl_dominfo_init(&d_info);
     cfg = libxlDriverConfigGet(driver);

     if (!(vm = libxlDomObjFromDomain(dom)))
@@ -4828,13 +4831,12 @@  libxlDomainMemoryStats(virDomainPtr dom,

     ret = i;

-    libxl_dominfo_dispose(&d_info);
-
  endjob:
     if (!libxlDomainObjEndJob(driver, vm))
         vm = NULL;

  cleanup:
+    libxl_dominfo_dispose(&d_info);
     if (vm)
         virObjectUnlock(vm);
     virObjectUnref(cfg);