Re: [libvirt] [PATCH 24/34] qemu: Refactor qemuDomainHotplugVcpus
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;
> }
>
>