[libvirt] [PATCH 25/34] qemu: refactor qemuDomainHotunplugVcpus

[John Ferlan]

On 11/20/2015 10:22 AM, Peter Krempa wrote:
> Refactor the code flow so that 'exit_monitor:' can be removed.
> 
> This patch moves the auditing functions into places where it's certain
> that hotunplug was or was not successful and reports errors from
> qemuMonitorGetCPUInfo properly.
> ---
>  src/qemu/qemu_driver.c | 50 +++++++++++++++-----------------------------------
>  1 file changed, 15 insertions(+), 35 deletions(-)
> 
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 9e0e334..614c7f8 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -4798,48 +4798,36 @@ qemuDomainHotunplugVcpus(virQEMUDriverPtr driver,
>  {
>      qemuDomainObjPrivatePtr priv = vm->privateData;
>      int ret = -1;
> +    int rc;
>      int oldvcpus = virDomainDefGetVCpus(vm->def);
> -    int vcpus = oldvcpus;
>      pid_t *cpupids = NULL;
> -    int ncpupids;
> +    int ncpupids = 0;
> 
>      qemuDomainObjEnterMonitor(driver, vm);
> 
> -    if (qemuMonitorSetCPU(priv->mon, vcpu, false) < 0)
> -        goto exit_monitor;
> +    rc = qemuMonitorSetCPU(priv->mon, vcpu, false);
> 
> -    vcpus--;
> +    if (rc == 0)
> +        ncpupids = qemuMonitorGetCPUInfo(priv->mon, &cpupids);
> 
> -    /* After hotplugging the CPUs we need to re-detect threads corresponding
> -     * to the virtual CPUs. Some older versions don't provide the thread ID
> -     * or don't have the "info cpus" command (and they don't support multiple
> -     * CPUs anyways), so errors in the re-detection will not be treated
> -     * fatal */
> -    if ((ncpupids = qemuMonitorGetCPUInfo(priv->mon, &cpupids)) <= 0) {
> -        virResetLastError();
> -        ret = 0;
> -        goto exit_monitor;
> -    }
>      if (qemuDomainObjExitMonitor(driver, vm) < 0)
>          goto cleanup;
> 
> -    /* check if hotunplug has failed */
> -    if (ncpupids == oldvcpus) {
> -        virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
> -                       _("qemu didn't unplug the vCPUs properly"));
> -        vcpus = oldvcpus;
> +    virDomainAuditVcpu(vm, oldvcpus, oldvcpus - 1, "update",
> +                       rc == 0 && ncpupids == oldvcpus -1);
> +

Similar comments to 24 w/r/t ExitMonitor failure and lack of Audit and
overwritten last message possible from GetCPUInfo failure.

w/r/t "&& ncpupids == oldvcpus - 1" in the audit message - if ncpupids
== 0 here, then unless we've dropped to zero vcpu's, this will always
trip strangely.

IOW: The ncpupids == 0 has been lost...

> +    if (rc < 0 || ncpupids < 0)
>          goto cleanup;
> -    }
> 
> -    if (ncpupids != vcpus) {
> -        virReportError(VIR_ERR_INTERNAL_ERROR,
> -                       _("got wrong number of vCPU pids from QEMU monitor. "
> -                         "got %d, wanted %d"),
> -                       ncpupids, vcpus);
> -        vcpus = oldvcpus;
> +    /* check if hotunplug has failed */
> +    if (ncpupids != oldvcpus - 1) {
> +        virReportError(VIR_ERR_OPERATION_UNSUPPORTED,
> +                       _("qemu didn't unplug vCPU '%u' properly"), vcpu);
>          goto cleanup;
>      }
> 
> +    ignore_value(virDomainDefSetVCpus(vm->def, oldvcpus - 1));
> +

Again - I would hope it wouldn't fail and not sure why ignore_value
should be used... I would think we'd have after the (rc < 0 || ncpupids
< 0) check (e.g. similar to Hotplug order):

    if (virDomainDefSetVCpus(vm->def, oldvcpus - 1) < 0)
        goto cleanup;

    /* Gratuitous comment here ... */
    if (ncpupids == 0) {
        ret = 0;
        goto cleanup;
    }

I'm sure you'll figure out a better order for an ACK...

John
>      if (qemuDomainDelCgroupForThread(priv->cgroup,
>                                       VIR_CGROUP_THREAD_VCPU, vcpu) < 0)
>          goto cleanup;
> @@ -4858,15 +4846,7 @@ qemuDomainHotunplugVcpus(virQEMUDriverPtr driver,
> 
>   cleanup:
>      VIR_FREE(cpupids);
> -    if (virDomainObjIsActive(vm) &&
> -        virDomainDefSetVCpus(vm->def, vcpus) < 0)
> -        ret = -1;
> -    virDomainAuditVcpu(vm, oldvcpus, vcpus, "update", ret == 0);
>      return ret;
> -
> - exit_monitor:
> -    ignore_value(qemuDomainObjExitMonitor(driver, vm));
> -    goto cleanup;
>  }
> 
>