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;
> }
>
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list