[libvirt] [PATCH 1/2] qemu: Resolve issue with virsh schedinfo for non running domain

[Martin Kletzander]

On 05/30/2013 02:24 PM, John Ferlan wrote:
> Since commit '632f78ca' the 'virsh schedinfo <domain>' command returns:
> 
> Scheduler      : Unknown
> error: Requested operation is not valid: cgroup CPU controller is not mounted
> 
> Prior to that change a non running domain would return:
> 
> Scheduler      : posix
> cpu_shares     : 0
> vcpu_period    : 0
> vcpu_quota     : 0
> emulator_period: 0
> emulator_quota : 0
> 
> This change will result in the following:
> 
> Scheduler      : posix
> cpu_shares     : 0
> 
> The code sequence is a 'virGetSchedulerType()' which returns the "*params"
> for a subsequent 'virDomainGetSchedulerParameters[Flags]()' call. The latter
> requires at least 1 'nparam' to be provided/returned.

I can't find where the nparams is required to be >= 1, but that might
changed since you've posted this patch.  I changed the '*nparams = 1' to
'0' and it works perfectly (returns only the scheduler type, so it's
visible that nothing is set).

> ---
>  src/qemu/qemu_driver.c | 11 ++++++++++-
>  1 file changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 4a76f14..7292149 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -6899,12 +6899,20 @@ static char *qemuDomainGetSchedulerType(virDomainPtr dom,
>      }
>      priv = vm->privateData;
>  
> +    /* Domain not running or no cgroup */
> +    if (!priv->cgroup) {
> +        if (nparams)
> +            *nparams = 1;
> +        goto cleanup;
> +    }
> +
>      if (!virCgroupHasController(priv->cgroup, VIR_CGROUP_CONTROLLER_CPU)) {
>          virReportError(VIR_ERR_OPERATION_INVALID,
>                         "%s", _("cgroup CPU controller is not mounted"));
>          goto cleanup;
>      }
>  
> +
>      if (nparams) {
>          rc = qemuGetCpuBWStatus(priv->cgroup);
>          if (rc < 0)
> @@ -6915,11 +6923,12 @@ static char *qemuDomainGetSchedulerType(virDomainPtr dom,
>              *nparams = 5;
>      }
>  
> +cleanup:
>      ignore_value(VIR_STRDUP(ret, "posix"));
>  

You can get to here even when the domain doesn't exist and in that case
NULL should be returned to indicate an error.  It would also skip an
error on problem in qemuGetCpuBWStatus(), but there is none printed, so
at least we would return the scheduler type.

> -cleanup:
>      if (vm)
>          virObjectUnlock(vm);
> +
>      return ret;
>  }
>  
> 

This patch fixes the problem, but may create a new one.  Also 'virsh
schedinfo --config <domain>' is different for running and shutoff
domain, but I'm willing to overlook that since it is less problematic
than what happened before.

Martin