Re: [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 
> ---
>  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, , ) < 0)
>  goto cleanup;
>  
> -for (i = 0; i < nparams; i++) {
> -virTypedParameterPtr param = [i];
> -enabled = params->value.b;
> -type = virPerfEventTypeFromString(param->field);
> +if (def) {
> +for (i = 0; i < nparams; i++) {
> +virTypedParameterPtr param = [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 = [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)))
> +

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

[Michal Privoznik]

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 
---
 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)
 return -1;
@@ -10071,31 +10072,37 @@ qemuDomainSetPerfEvents(virDomainPtr dom,
 if (virDomainObjGetDefs(vm, flags, , ) < 0)
 goto cleanup;
 
-for (i = 0; i < nparams; i++) {
-virTypedParameterPtr param = [i];
-enabled = params->value.b;
-type = virPerfEventTypeFromString(param->field);
+if (def) {
+for (i = 0; i < nparams; i++) {
+virTypedParameterPtr param = [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++) {
+virTypedParameterPtr param = [i];
+enabled = params->value.b;
+type = virPerfEventTypeFromString(param->field);
+
 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;
 
 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, ,
+) < 0)
+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);
+
 if (virTypedParamsAddBoolean(, , ,
  virPerfEventTypeToString(i),
- virPerfEventIsEnabled(priv->perf, i)) < 
0) {
+ perf_enabled) < 0) {
 virTypedParamsFree(par, npar);
 goto cleanup;
 }
@@ -10145,6 +10174,7 @@