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

[Eric Blake]

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?

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 539 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20140912/62bd66dc/attachment-0001.sig>