[libvirt] [PATCH v2 07/21] qemu: Reuse qemuDomainDetectVcpuPids in cpu hot(un)plug

[Ján Tomko]

On Fri, Jan 29, 2016 at 05:02:02PM +0100, Peter Krempa wrote:
> Now that qemuDomainDetectVcpuPids is able to refresh the vCPU pid
> information it can be reused in the hotplug and hotunplug code paths
> rather than open-coding a very similar algorithm.
> 
> A slight algoirithm change is necessary for unplug since the vCPU needs

*algorithm, as John pointed out in the review of v1.

> to be marked offline prior to calling the thread detector function and
> eventually rolled back if something fails.
> ---
> 
> Notes:
>     v2: fix bugs regarding error codes from redetection
> 
>  src/qemu/qemu_driver.c | 86 +++++++++++++++++---------------------------------
>  1 file changed, 29 insertions(+), 57 deletions(-)

> @@ -4887,49 +4865,43 @@ qemuDomainHotplugDelVcpu(virQEMUDriverPtr driver,
>          return -1;
>      }
> 
> +    vcpuinfo->online = false;
> +
>      qemuDomainObjEnterMonitor(driver, vm);
> 
>      rc = qemuMonitorSetCPU(priv->mon, vcpu, false);
> 
> -    if (rc == 0)
> -        ncpupids = qemuMonitorGetCPUInfo(priv->mon, &cpupids);
> -
>      if (qemuDomainObjExitMonitor(driver, vm) < 0)
>          goto cleanup;
> 
> -    virDomainAuditVcpu(vm, oldvcpus, oldvcpus - 1, "update",
> -                       rc == 0 && ncpupids == oldvcpus -1);
> +    if (rc < 0)
> +        rc = qemuDomainDetectVcpuPids(driver, vm, QEMU_ASYNC_JOB_NONE);

I would expect 'goto cleanup' if SetCPU failed and re-detecting the CPUs
if it succeeded.

> -    if (rc < 0 || ncpupids < 0)
> -        goto cleanup;
> +    virDomainAuditVcpu(vm, oldvcpus, oldvcpus - 1, "update", rc >= 0);
> +

If SetCPU failed, this would audit the return value of DetectVcpuPids.

> +    /* Free vcpupin setting */
> +    virDomainPinDel(&vm->def->cputune.vcpupin,
> +                    &vm->def->cputune.nvcpupin,
> +                    vcpu);

And if the domain crashed while DetectVcpuPids was in the monitor,
vm->def would be a stale pointer here.

> +
> +    if (rc <= 0) {
> +        if (rc == 0)
> +            ret = 0;
> 
> -    /* check if hotunplug has failed */
> -    if (ncpupids != oldvcpus - 1) {
> -        virReportError(VIR_ERR_OPERATION_UNSUPPORTED,
> -                       _("qemu didn't unplug vCPU '%u' properly"), vcpu);
>          goto cleanup;
>      }
> 
> -    vcpuinfo->online = false;
> -
>      if (qemuDomainDelCgroupForThread(priv->cgroup,
>                                       VIR_CGROUP_THREAD_VCPU, vcpu) < 0)
>          goto cleanup;
> 
> -    /* Free vcpupin setting */
> -    virDomainPinDel(&vm->def->cputune.vcpupin,
> -                    &vm->def->cputune.nvcpupin,
> -                    vcpu);
> -
> -    priv->nvcpupids = ncpupids;
> -    VIR_FREE(priv->vcpupids);
> -    priv->vcpupids = cpupids;
> -    cpupids = NULL;
> -
>      ret = 0;
> 
>   cleanup:
> -    VIR_FREE(cpupids);
> +    /* rollback the cpu state */
> +    if (ret < 0)
> +        vcpuinfo->online = true;
> +

vcpuinfo points into vm->def. It might be freed if the domain crashed.

Also, should the vcpu be marked as online if we got here because
DelCgroupForThread failed?

Jan
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20160201/ce1185fd/attachment-0001.sig>