[libvirt] [PATCH] qemu: report vcpu state in virDomainGetVcpus

[Peter Krempa]

On Fri, Jul 08, 2016 at 15:39:00 +0200, Boris Fiuczynski wrote:
> From: Viktor Mihajlovski <mihajlov at linux.vnet.ibm.com>
> 
> Currently, the virVcpuInfo returned by virDomainGetVcpus() will always
> report a state of VIR_VCPU_RUNNING for each defined domain vcpu even if
> the vcpu is currently in the halted state.
> 
> As the monitor interface is in fact reporting the accurate state, it is
> rather easy to transport this information with the existing API.

Could you please elaborat how you expect to use this info?

> This is done by
> - adding a new state of VIR_VCPU_HALTED
> - extending the monitor to pass back the halted state for the vcpus
> - adding a new array to the private domain object reflecting the halted
>   state for the vcpus and update it along with the vcpu pids array
> - modifying the driver code to report the vcpu state based on the halted
>   indicator
> - extending virsh vcpuinfo to also display the halted state
> - modifying the monitor_json testcase
> 
> The vcpu state is however not recorded in the internal XML format, since
> the state can change asynchronously (without notification).

virDomainGetVcpus does not call qemuDomainDetectVcpuPids nor does not
update the info in any way. qemuDomainDetectVcpuPids is called just at
startup of the qemu process. As of such this is reporting old data at
any time and thus doesn't really make much sense in the current state.

Please note that calling qemuDomainDetectVcpuPids is not desired in
virDomainGetVcpus. You would need to update just the vcpu states.

Still doing so doesn't seem to make much sense and thus I'd like an
elaboration why this is necessary.

> Signed-off-by: Viktor Mihajlovski <mihajlov at linux.vnet.ibm.com>
> Signed-off-by: Boris Fiuczynski <fiuczy at linux.vnet.ibm.com>
> Reviewed-by: Bjoern Walk <bwalk at linux.vnet.ibm.com>
> ---
>  include/libvirt/libvirt-domain.h |  1 +
>  src/qemu/qemu_domain.c           | 26 +++++++++++++++++++++++++-
>  src/qemu/qemu_domain.h           |  2 ++
>  src/qemu/qemu_driver.c           | 14 +++++++++++++-
>  src/qemu/qemu_monitor.c          |  7 ++++---
>  src/qemu/qemu_monitor.h          |  3 ++-
>  src/qemu/qemu_monitor_json.c     | 22 +++++++++++++++++++---
>  src/qemu/qemu_monitor_json.h     |  3 ++-
>  src/qemu/qemu_monitor_text.c     | 17 +++++++++++++++--
>  src/qemu/qemu_monitor_text.h     |  3 ++-
>  src/qemu/qemu_process.c          |  1 +
>  tests/qemumonitorjsontest.c      | 18 +++++++++++++++---
>  tools/virsh-domain.c             |  3 ++-
>  13 files changed, 103 insertions(+), 17 deletions(-)
> 
> diff --git a/include/libvirt/libvirt-domain.h b/include/libvirt/libvirt-domain.h
> index 7ea93aa..98b9420 100644
> --- a/include/libvirt/libvirt-domain.h
> +++ b/include/libvirt/libvirt-domain.h
> @@ -1656,6 +1656,7 @@ typedef enum {
>      VIR_VCPU_OFFLINE    = 0,    /* the virtual CPU is offline */
>      VIR_VCPU_RUNNING    = 1,    /* the virtual CPU is running */
>      VIR_VCPU_BLOCKED    = 2,    /* the virtual CPU is blocked on resource */
> +    VIR_VCPU_HALTED     = 3,    /* the virtual CPU is halted */
>  
>  # ifdef VIR_ENUM_SENTINELS
>      VIR_VCPU_LAST
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index 42b5511..3428605 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -1256,6 +1256,7 @@ qemuDomainObjPrivateFree(void *data)
>      virDomainChrSourceDefFree(priv->monConfig);
>      qemuDomainObjFreeJob(priv);
>      VIR_FREE(priv->vcpupids);

I'm currently refactoring the code to get rid of this structure. (See
https://www.redhat.com/archives/libvir-list/2016-July/msg00182.html ) So
this is going against that direction.

> +    VIR_FREE(priv->vcpuhalted);
>      VIR_FREE(priv->lockState);
>      VIR_FREE(priv->origname);

[...]

> diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
> index 098e654..0272fb4 100644
> --- a/src/qemu/qemu_monitor.c
> +++ b/src/qemu/qemu_monitor.c
> @@ -1656,14 +1656,15 @@ qemuMonitorSystemReset(qemuMonitorPtr mon)
>   */
>  int
>  qemuMonitorGetCPUInfo(qemuMonitorPtr mon,
> -                      int **pids)
> +                      int **pids,
> +                      bool **haltpids)

The name of the variable doesn't make sense. The array refers to vcpus
rather than pids.

>  {
>      QEMU_CHECK_MONITOR(mon);
>  
>      if (mon->json)
> -        return qemuMonitorJSONGetCPUInfo(mon, pids);
> +        return qemuMonitorJSONGetCPUInfo(mon, pids, haltpids);
>      else
> -        return qemuMonitorTextGetCPUInfo(mon, pids);
> +        return qemuMonitorTextGetCPUInfo(mon, pids, haltpids);

Peter