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

On 09/10/2014 06:08 AM, Pavel Hrdina wrote: > Signed-off-by: Pavel Hrdina <phrdina(a)redhat.com&gt; > --- > 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?