[libvirt] [PATCH 5/6] virDomain{Get, Set}PerfEvents: support --config --live --current

[Peter Krempa]

On Thu, Mar 31, 2016 at 07:29:00 +0200, Michal Privoznik wrote:
> Now that we have @flags we can support changing perf events just
> in active or inactive configuration regardless of the other.
> Previously, calling virDomainSetPerfEvents set events in both
> active and inactive configuration at once. Even though we allow
> users to set perf events that are to be enabled once domain is
> started up. The virDomainGetPerfEvents API was flawed too. It
> returned just runtime info.
> 
> Signed-off-by: Michal Privoznik <mprivozn at redhat.com>
> ---
>  src/qemu/qemu_driver.c | 76 +++++++++++++++++++++++++++++++++++---------------
>  tools/virsh-domain.c   | 14 ++++++++++
>  tools/virsh.pod        |  8 ++++++
>  3 files changed, 75 insertions(+), 23 deletions(-)
> 
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index cbd520b..56120ff 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -10054,7 +10054,8 @@ qemuDomainSetPerfEvents(virDomainPtr dom,
>      virPerfEventType type;
>      bool enabled;
>  
> -    virCheckFlags(0, -1);
> +    virCheckFlags(VIR_DOMAIN_AFFECT_LIVE |
> +                  VIR_DOMAIN_AFFECT_CONFIG, -1);
>  
>      if (virTypedParamsValidate(params, nparams, VIR_PERF_PARAMETERS) < 0)

VIR_PERF_PARAMETERS is unfortunately not required to be kept in sync
with the virPerfEvent enum ...

>          return -1;
> @@ -10071,31 +10072,37 @@ qemuDomainSetPerfEvents(virDomainPtr dom,
>      if (virDomainObjGetDefs(vm, flags, &def, &persistentDef) < 0)
>          goto cleanup;
>  
> -    for (i = 0; i < nparams; i++) {
> -        virTypedParameterPtr param = &params[i];
> -        enabled = params->value.b;
> -        type = virPerfEventTypeFromString(param->field);
> +    if (def) {
> +        for (i = 0; i < nparams; i++) {
> +            virTypedParameterPtr param = &params[i];
> +            enabled = params->value.b;
> +            type = virPerfEventTypeFromString(param->field);
>  
> -        if (!enabled && virPerfEventDisable(priv->perf, type))
> -            goto cleanup;
> -        if (enabled && virPerfEventEnable(priv->perf, type, vm->pid))
> -            goto cleanup;
> +            if (!enabled && virPerfEventDisable(priv->perf, type))
> +                goto cleanup;
> +            if (enabled && virPerfEventEnable(priv->perf, type, vm->pid))
> +                goto cleanup;
>  
> -        if (def) {
>              def->perf->events[type] = enabled ?
>                  VIR_TRISTATE_BOOL_YES : VIR_TRISTATE_BOOL_NO;
> -
> -            if (virDomainSaveStatus(driver->xmlopt, cfg->stateDir, vm, driver->caps) < 0)
> -                goto cleanup;
>          }
>  
> -        if (persistentDef) {
> +        if (virDomainSaveStatus(driver->xmlopt, cfg->stateDir, vm, driver->caps) < 0)
> +            goto cleanup;
> +    }
> +
> +    if (persistentDef) {
> +        for (i = 0; i < nparams; i++) {

You could go with one loop, that sets both the live and persistent defs
as it was previously. Yoy just need to move the section that is actually
enabling the perf events into 'if (def)'

> +            virTypedParameterPtr param = &params[i];
> +            enabled = params->value.b;
> +            type = virPerfEventTypeFromString(param->field);

... thus you probably should make sure that the returned data makes
sense. This can be fixed later though since it is currently in sync.


> +
>              persistentDef->perf->events[type] = enabled ?
>                  VIR_TRISTATE_BOOL_YES : VIR_TRISTATE_BOOL_NO;
> -
> -            if (virDomainSaveConfig(cfg->configDir, driver->caps, persistentDef) < 0)
> -                goto cleanup;
>          }
> +
> +        if (virDomainSaveConfig(cfg->configDir, driver->caps, persistentDef) < 0)
> +            goto cleanup;
>      }
>  
>      ret = 0;
> @@ -10112,28 +10119,50 @@ qemuDomainGetPerfEvents(virDomainPtr dom,
>                          int *nparams,
>                          unsigned int flags)
>  {
> -    size_t i;
> +    virQEMUDriverPtr driver = dom->conn->privateData;
>      virDomainObjPtr vm = NULL;
>      qemuDomainObjPrivatePtr priv;
> -    int ret = -1;
> +    virDomainDefPtr persistentDef;
>      virTypedParameterPtr par = NULL;
> +    virCapsPtr caps = NULL;
>      int maxpar = 0;
>      int npar = 0;
> +    size_t i;
> +    int ret = -1;
>  
> -    virCheckFlags(0, -1);
> +    virCheckFlags(VIR_DOMAIN_AFFECT_LIVE |
> +                  VIR_DOMAIN_AFFECT_CONFIG |
> +                  VIR_TYPED_PARAM_STRING_OKAY, -1);
> +
> +    /* We don't return strings, and thus trivially support this flag.  */
> +    flags &= ~VIR_TYPED_PARAM_STRING_OKAY;

I don't think we need this for APIs that were introduced after the
support for string typed params.

>  
>      if (!(vm = qemuDomObjFromDomain(dom)))
>          goto cleanup;
>  
> -    priv = vm->privateData;
> -
>      if (virDomainGetPerfEventsEnsureACL(dom->conn, vm->def) < 0)
>          goto cleanup;
>  
> +    if (!(caps = virQEMUDriverGetCapabilities(driver, false)))
> +        goto cleanup;
> +
> +    if (virDomainLiveConfigHelperMethod(caps, driver->xmlopt, vm, &flags,
> +                                        &persistentDef) < 0)

Don't use this function in the qemu driver. The suggested replacement is 
virDomainObjGetOneDef. With that function you don't need @caps or
@driver.

If you need @flags tweaked for the function below, then look into that
function for the helper.

> +        goto cleanup;
> +
> +    priv = vm->privateData;
> +
>      for (i = 0; i < VIR_PERF_EVENT_LAST; i++) {
> +        bool perf_enabled;
> +
> +        if (flags & VIR_DOMAIN_AFFECT_CONFIG)
> +            perf_enabled = persistentDef->perf->events[i] == VIR_TRISTATE_BOOL_YES;
> +        else
> +            perf_enabled = virPerfEventIsEnabled(priv->perf, i);

Sigh, why is that stored in 'priv' rather than 'def'? If we store it in
@def then this if won't be required.

> +
>          if (virTypedParamsAddBoolean(&par, &npar, &maxpar,
>                                       virPerfEventTypeToString(i),
> -                                     virPerfEventIsEnabled(priv->perf, i)) < 0) {
> +                                     perf_enabled) < 0) {
>              virTypedParamsFree(par, npar);
>              goto cleanup;
>          }

ACK iff you get rid of the usage of virDomainLiveConfigHelperMethod.

Other comments are optional.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20160331/c99da3d1/attachment-0001.sig>