[libvirt] [PATCH v2 5/5] cputune_event: queue the event for cputune updates

[Pavel Hrdina]

On 09/13/2014 12:12 AM, Eric Blake wrote:
> On 09/10/2014 06:08 AM, Pavel Hrdina wrote:
>> Signed-off-by: Pavel Hrdina <phrdina at redhat.com>
>> ---
>>   src/qemu/qemu_cgroup.c | 18 ++++++++++++-
>>   src/qemu/qemu_driver.c | 73 ++++++++++++++++++++++++++++++++++++++++++++++++++
>>   2 files changed, 90 insertions(+), 1 deletion(-)
>>
>
>> @@ -676,6 +677,10 @@ static int
>>   qemuSetupCpuCgroup(virDomainObjPtr vm)
>>   {
>>       qemuDomainObjPrivatePtr priv = vm->privateData;
>> +    virObjectEventPtr event = NULL;
>> +    virTypedParameterPtr eventParams;
>> +    int eventNparams = 0;
>> +    int eventMaxparams = 0;
>>
>>       if (!virCgroupHasController(priv->cgroup, VIR_CGROUP_CONTROLLER_CPU)) {
>>          if (vm->def->cputune.sharesSpecified) {
>> @@ -694,7 +699,18 @@ qemuSetupCpuCgroup(virDomainObjPtr vm)
>>
>>           if (virCgroupGetCpuShares(priv->cgroup, &val) < 0)
>>               return -1;
>> -        vm->def->cputune.shares = val;
>> +        if (vm->def->cputune.shares != val) {
>> +            vm->def->cputune.shares = val;
>> +            if (virTypedParamsAddULLong(&eventParams, &eventNparams,
>> +                                        &eventMaxparams, "shares",
>> +                                        val) < 0)
>> +                return -1;
>> +
>> +            event = virDomainEventCputuneNewFromObj(vm, eventParams, eventNparams);
>> +        }
>> +
>> +        if (event)
>> +            qemuDomainEventQueue(vm->privateData, event);
>>       }
>
> Continuing my comments from 3/5: Memory leak - if
> virDomainEventCputuneNewFromObj() fails, nothing frees eventParams; but
> since this particular event was exactly one typed parameter, at least
> you have no other problems.  Similar leaks for other single-element lists.
>
> But then we have this:
>
>> @@ -9054,6 +9092,10 @@ qemuDomainSetSchedulerParametersFlags(virDomainPtr dom,
>>       virQEMUDriverConfigPtr cfg = NULL;
>>       virCapsPtr caps = NULL;
>>       qemuDomainObjPrivatePtr priv;
>> +    virObjectEventPtr event = NULL;
>> +    virTypedParameterPtr eventParams = NULL;
>> +    int eventNparams = 0;
>> +    int eventMaxNparams = 0;
>>
>>       virCheckFlags(VIR_DOMAIN_AFFECT_LIVE |
>>                     VIR_DOMAIN_AFFECT_CONFIG, -1);
>> @@ -9124,6 +9166,11 @@ qemuDomainSetSchedulerParametersFlags(virDomainPtr dom,
>>
>>                   vm->def->cputune.shares = val;
>>                   vm->def->cputune.sharesSpecified = true;
>> +
>> +                if (virTypedParamsAddULLong(&eventParams, &eventNparams,
>> +                                            &eventMaxNparams, "shares",
>> +                                            val) < 0)
>> +                    goto cleanup;
>>               }
>>
>>               if (flags & VIR_DOMAIN_AFFECT_CONFIG) {
>> @@ -9141,6 +9188,11 @@ qemuDomainSetSchedulerParametersFlags(virDomainPtr dom,
>>                       goto cleanup;
>>
>>                   vm->def->cputune.period = value_ul;
>> +
>> +                if (virTypedParamsAddULLong(&eventParams, &eventNparams,
>> +                                            &eventMaxNparams, "period",
>> +                                            value_ul) < 0)
>> +                    goto cleanup;
>>               }
>
> Hmm.  Now you have an event with more than one list member.  Suppose
> that you successfully add "shares" to the list, then run out of memory
> on virTypedParamsAddULLong() for "period".  What happens to the
> partially-allocated list? ...
>
>> @@ -9205,12 +9272,18 @@ qemuDomainSetSchedulerParametersFlags(virDomainPtr dom,
>>           vmdef = NULL;
>>       }
>>
>> +
>>       ret = 0;
>>
>>    cleanup:
>>       virDomainDefFree(vmdef);
>>       if (vm)
>>           virObjectUnlock(vm);
>> +    if (eventNparams) {
>> +        event = virDomainEventCputuneNewFromDom(dom, eventParams, eventNparams);
>
> ...oh.  This says you are attempting to create the event anyways, in
> spite of having hit OOM on the attempt to add the second list member.
> So I suppose that if you do transfer semantics, even the partial list
> will eventually get cleaned up by the attempt to create the event.  But
> the fact that you failed to create a full list makes it all the more
> likely that you will also fail to create the event, and probably have a
> leak on your hands.  Is it even worth trying to create the event if the
> reason that we jumped to cleanup was due to an OOM condition in
> populating the list of parameters the event would include?
>

I'll change the behavior to trigger the event only when the live update 
succeed and for other case I'll free the eventParams.

Thanks,
Pavel