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