[libvirt] [PATCH] conf: resctrl object is not properly handled

[Michal Privoznik]

On 8/20/19 3:18 PM, Daniel Henrique Barboza wrote:
> 
> 
> On 8/20/19 7:06 AM, Wang Huaqiang wrote:
>> resctrl object stored in def->resctrls is shared by cachetune and
>> memorytune. The domain xml configuration is parsed firstly for
>> cachetune then memorytune, and the resctrl object will not be created
>> in parsing settings for memorytune once it found sharing exists.
>>
>> But resctrl is improperly freed when sharing happens.
>>
>> Signed-off-by: Wang Huaqiang <huaqiang.wang at intel.com>
>> ---
>>   src/conf/domain_conf.c                    | 12 +++++-------
>>   tests/genericxml2xmlindata/memorytune.xml |  4 ++++
>>   2 files changed, 9 insertions(+), 7 deletions(-)
>>
>> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
>> index 617ccac..604e006 100644
>> --- a/src/conf/domain_conf.c
>> +++ b/src/conf/domain_conf.c
>> @@ -19590,7 +19590,6 @@ virDomainMemorytuneDefParse(virDomainDefPtr def,
>>       VIR_AUTOUNREF(virResctrlAllocPtr) alloc = NULL;
>>       ssize_t i = 0;
>>       int n;
>> -    int ret = -1;
>>       ctxt->node = node;
>> @@ -19632,14 +19631,13 @@ virDomainMemorytuneDefParse(virDomainDefPtr 
>> def,
>>           if (!(resctrl = virDomainResctrlNew(node, alloc, vcpus, 
>> flags)))
>>               return -1;
>> -        if (VIR_APPEND_ELEMENT(def->resctrls, def->nresctrls, 
>> resctrl) < 0)
>> -            goto cleanup;
>> +        if (VIR_APPEND_ELEMENT(def->resctrls, def->nresctrls, 
>> resctrl) < 0) {
>> +            virDomainResctrlDefFree(resctrl);
>> +            return -1;
>> +        }
>>       }
>> -    ret = 0;
>> - cleanup:
>> -    virDomainResctrlDefFree(resctrl);
>> -    return ret;
>> +    return 0;
>>   }
> 
> This could also be fixed by putting the free into a conditional:
> 
> if (resctrl)
>      virDomainRescrtlDefFree(resctrl);

Not really. We want to free @resctrl only if it's a newly allocated 
struct. In case the pointer was obtained via virDomainResctrlVcpuMatch() 
then we must not free it (even though it is not NULL).

Reviewed-by: Michal Privoznik <mprivozn at redhat.com>

And pushed. Thanks for fixing this.

Michal