[libvirt] [PATCH 24/34] qemu: Refactor qemuDomainHotplugVcpus

[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 also moves the auditing and setting of the new vCPU count
> right to the place where the hotplug happens, since it's possible that
> the hotplug succeeds and adds a cpu while other stuff fails.
> 
> Lastly, failures of qemuMonitorGetCPUInfo are now reported rather than
> ignored. The function retuns 0 if it "successfully" detected 0 threads.
> ---
>  src/qemu/qemu_driver.c | 54 +++++++++++++++++++++++---------------------------
>  1 file changed, 25 insertions(+), 29 deletions(-)
> 

Damn - should have peeked ahead...

> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 9f0e3a3..9e0e334 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -4698,41 +4698,46 @@ qemuDomainHotplugVcpus(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;
>      virCgroupPtr cgroup_vcpu = NULL;
>      char *mem_mask = NULL;
>      virDomainNumatuneMemMode mem_mode;
> 
>      qemuDomainObjEnterMonitor(driver, vm);
> 
> -    if (qemuMonitorSetCPU(priv->mon, vcpu, true) < 0)
> -        goto exit_monitor;
> -
> -    vcpus++;
> +    rc = qemuMonitorSetCPU(priv->mon, vcpu, true);
> 
> -    /* 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 (rc == 0)
> +        ncpupids = qemuMonitorGetCPUInfo(priv->mon, &cpupids);
> 
>      if (qemuDomainObjExitMonitor(driver, vm) < 0)

This could overwrite a qemuMonitorGetCPUInfo error, but that's no
different than we current could do (though we don't ResetLast on
GetCPUInfo failure).

>          goto cleanup;

But if we leave here, then we know we're still active thus the
adjustment from cleanup to call SetVcpus only if still active is
satisfied...

> 
> -    if (ncpupids != vcpus) {
> +    virDomainAuditVcpu(vm, oldvcpus, oldvcpus + 1, "update", rc == 0);

If the ExitMonitor fails, then we won't Audit

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

Why not just :

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

I would *hope* that we don't fail SetVcpus at this point - at least we
can avoid the pointless ignore_value

ACK - as long as we can audit on failure to ExitMonitor... Whether you
feel the GetCPUInfo error is worth saving/sending is up to you. The last
comment is purely an observation.

John
> +
> +    if (ncpupids < 0)
> +        goto cleanup;
> +
> +    /* failure to re-detect vCPU pids after hotplug due to lack of support was
> +     * historically deemed not fatal. We need to skip the rest of the steps though. */
> +    if (ncpupids == 0) {
> +        ret = 0;
> +        goto cleanup;
> +    }
> +
> +    if (ncpupids != oldvcpus + 1) {
>          virReportError(VIR_ERR_INTERNAL_ERROR,
>                         _("got wrong number of vCPU pids from QEMU monitor. "
>                           "got %d, wanted %d"),
> -                       ncpupids, vcpus);
> -        vcpus = oldvcpus;
> +                       ncpupids, oldvcpus + 1);
>          goto cleanup;
>      }
> 
> @@ -4781,17 +4786,8 @@ qemuDomainHotplugVcpus(virQEMUDriverPtr driver,
>   cleanup:
>      VIR_FREE(cpupids);
>      VIR_FREE(mem_mask);
> -    if (virDomainObjIsActive(vm) &&
> -        virDomainDefSetVCpus(vm->def, vcpus) < 0)
> -        ret = -1;
> -    virDomainAuditVcpu(vm, oldvcpus, vcpus, "update", ret == 0);
> -    if (cgroup_vcpu)
> -        virCgroupFree(&cgroup_vcpu);
> +    virCgroupFree(&cgroup_vcpu);
>      return ret;
> -
> - exit_monitor:
> -    ignore_value(qemuDomainObjExitMonitor(driver, vm));
> -    goto cleanup;
>  }
> 
>