[libvirt] [PATCH v2 5/5] cputune_event: queue the event for cputune updates
Pavel Hrdina
phrdina at redhat.com
Mon Sep 15 13:21:05 UTC 2014
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
More information about the libvir-list
mailing list