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
Attachment:
signature.asc
Description: Digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list