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