[libvirt] [PATCH] numatune: setting --mode does not work well

[Erik Skultety]

I see your point, yes, you are right, a leak might occur and I'll fix it
in the cleanup phase. Just to be correct, it is not related to the
error-check you pointed out. It is because up until now, numatune was
only a local duplicate pointer to an allocated memory which was
referenced by *numatunePtr (passed as argument). And this was the core
of the bug, working directly with a pointer passed to the function. If
any problem occures, that pointer should stay untouched, that's why I
slightly changed the allocation, thus unfortunately creating a
possibility for the leak. So I'm putting another 'if(create)' clause
inside the cleanup phase in PATCHv2.

Regards,
Erik

On 08/20/2014 02:50 PM, Ján Tomko wrote:
> On 08/20/2014 12:49 PM, Erik Skultety wrote:
>> When trying to set numatune mode directly using virsh numatune command,
>> correct error is raised, however numatune structure was not deallocated,
>> thus resulting in creating an empty numatune element in the guest XML,
>> if none was present before. Running the same command aftewards results
>> in a successful change with broken XML structure. Patch fixes the
>> deallocation problem as well as checking for invalid attribute
>> combination VIR_DOMAIN_NUMATUNE_PLACEMENT_AUTO + a nonempty nodeset.
>>
>> Resolves https://bugzilla.redhat.com/show_bug.cgi?id=1129998
>> ---
>>   src/conf/numatune_conf.c | 28 +++++++++++++++++++---------
>>   1 file changed, 19 insertions(+), 9 deletions(-)
>>
>> diff --git a/src/conf/numatune_conf.c b/src/conf/numatune_conf.c
>> index 48d1d04..45bf0cb 100644
>> --- a/src/conf/numatune_conf.c
>> +++ b/src/conf/numatune_conf.c
>> @@ -439,7 +439,7 @@ virDomainNumatuneSet(virDomainNumatunePtr *numatunePtr,
>>   {
>>       bool create = !*numatunePtr;  /* Whether we are creating new struct */
>>       int ret = -1;
>> -    virDomainNumatunePtr numatune = NULL;
>> +    virDomainNumatunePtr numatune = *numatunePtr;
>>
>>       /* No need to do anything in this case */
>>       if (mode == -1 && placement == -1 && !nodeset)
>> @@ -461,9 +461,15 @@ virDomainNumatuneSet(virDomainNumatunePtr *numatunePtr,
>>           goto cleanup;
>>       }
>>
>> -    if (create && VIR_ALLOC(*numatunePtr) < 0)
>> +    if (placement_static && !nodeset) {
>> +        virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
>> +                       _("nodeset for NUMA memory tuning must be set "
>> +                         "if 'placement' is 'static'"));
>> +        goto cleanup;
>
> You moved this error above the allocation, but there is another 'goto cleanup'
> possible after the successful allocation of 'numatune':
>
>       if (nodeset) {
>           virBitmapFree(numatune->memory.nodeset);
>           numatune->memory.nodeset = virBitmapNewCopy(nodeset);
>           if (!numatune->memory.nodeset)
>               goto cleanup;
>           if (placement == -1)
>               placement = VIR_DOMAIN_NUMATUNE_PLACEMENT_STATIC;
>       }
>
> If virBitmapNewCopy fails and numatune was allocated by this function, it's
> leaked.
>
> Jan
>
>
>
> --
> libvir-list mailing list
> libvir-list at redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
>