[libvirt] [PATCH 0/7] qemu: Job handling fixes
While reviewing Martin's reference counting series I've noticed a few qemu API impls that don't properly handle jobs. Peter Krempa (7): qemu: Fix job handling in qemuDomainPinVcpuFlags qemu: Fix job handling in qemuDomainPinEmulator qemu: Fix job handling in qemuDomainSetAutostart qemu: Fix job handling in qemuDomainSetMemoryParameters qemu: Fix job handling in qemuDomainSetSchedulerParametersFlags qemu: Fix job type in qemuDomainGetBlockIoTune qemu: Fix job handling in qemuDomainSetMetadata src/qemu/qemu_driver.c | 135 +++++++++++++++++++++++++++++++------------------ 1 file changed, 87 insertions(+), 48 deletions(-) -- 2.2.1
The domain modifies the domain configuration but doesn't take a MODIFY type job to do it. --- src/qemu/qemu_driver.c | 32 +++++++++++++++++++------------- 1 file changed, 19 insertions(+), 13 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 5994558..4882cab 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -4825,24 +4825,27 @@ qemuDomainPinVcpuFlags(virDomainPtr dom, if (virBitmapIsAllSet(pcpumap)) doReset = true; + if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0) + goto cleanup; + if (flags & VIR_DOMAIN_AFFECT_LIVE) { if (priv->vcpupids == NULL) { virReportError(VIR_ERR_OPERATION_INVALID, "%s", _("cpu affinity is not supported")); - goto cleanup; + goto endjob; } if (vm->def->cputune.vcpupin) { newVcpuPin = virDomainVcpuPinDefCopy(vm->def->cputune.vcpupin, vm->def->cputune.nvcpupin); if (!newVcpuPin) - goto cleanup; + goto endjob; newVcpuPinNum = vm->def->cputune.nvcpupin; } else { if (VIR_ALLOC(newVcpuPin) < 0) - goto cleanup; + goto endjob; newVcpuPinNum = 0; } @@ -4850,25 +4853,25 @@ qemuDomainPinVcpuFlags(virDomainPtr dom, virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("failed to update vcpupin")); virDomainVcpuPinDefArrayFree(newVcpuPin, newVcpuPinNum); - goto cleanup; + goto endjob; } /* Configure the corresponding cpuset cgroup before set affinity. */ if (virCgroupHasController(priv->cgroup, VIR_CGROUP_CONTROLLER_CPUSET)) { if (virCgroupNewVcpu(priv->cgroup, vcpu, false, &cgroup_vcpu) < 0) - goto cleanup; + goto endjob; if (qemuSetupCgroupVcpuPin(cgroup_vcpu, newVcpuPin, newVcpuPinNum, vcpu) < 0) { virReportError(VIR_ERR_OPERATION_INVALID, _("failed to set cpuset.cpus in cgroup" " for vcpu %d"), vcpu); - goto cleanup; + goto endjob; } } else { if (virProcessSetAffinity(priv->vcpupids[vcpu], pcpumap) < 0) { virReportError(VIR_ERR_SYSTEM_ERROR, _("failed to set cpu affinity for vcpu %d"), vcpu); - goto cleanup; + goto endjob; } } @@ -4887,17 +4890,17 @@ qemuDomainPinVcpuFlags(virDomainPtr dom, virDomainVcpuPinDefArrayFree(newVcpuPin, newVcpuPinNum); if (virDomainSaveStatus(driver->xmlopt, cfg->stateDir, vm) < 0) - goto cleanup; + goto endjob; if (snprintf(paramField, VIR_TYPED_PARAM_FIELD_LENGTH, VIR_DOMAIN_TUNABLE_CPU_VCPUPIN, vcpu) < 0) { - goto cleanup; + goto endjob; } str = virBitmapFormat(pcpumap); if (virTypedParamsAddString(&eventParams, &eventNparams, &eventMaxparams, paramField, str) < 0) - goto cleanup; + goto endjob; event = virDomainEventTunableNewFromDom(dom, eventParams, eventNparams); } @@ -4909,7 +4912,7 @@ qemuDomainPinVcpuFlags(virDomainPtr dom, } else { if (!persistentDef->cputune.vcpupin) { if (VIR_ALLOC(persistentDef->cputune.vcpupin) < 0) - goto cleanup; + goto endjob; persistentDef->cputune.nvcpupin = 0; } if (virDomainVcpuPinAdd(&persistentDef->cputune.vcpupin, @@ -4920,16 +4923,19 @@ qemuDomainPinVcpuFlags(virDomainPtr dom, virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("failed to update or add vcpupin xml of " "a persistent domain")); - goto cleanup; + goto endjob; } } ret = virDomainSaveConfig(cfg->configDir, persistentDef); - goto cleanup; + goto endjob; } ret = 0; + endjob: + qemuDomainObjEndJob(driver, vm); + cleanup: if (cgroup_vcpu) virCgroupFree(&cgroup_vcpu); -- 2.2.1
The code modifies the domain configuration but doesn't take a MODIFY type job to do so. --- src/qemu/qemu_driver.c | 30 ++++++++++++++++++------------ 1 file changed, 18 insertions(+), 12 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 4882cab..fa2259a 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -5116,17 +5116,20 @@ qemuDomainPinEmulator(virDomainPtr dom, pid = vm->pid; + if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0) + goto cleanup; + if (flags & VIR_DOMAIN_AFFECT_LIVE) { if (priv->vcpupids != NULL) { if (VIR_ALLOC(newVcpuPin) < 0) - goto cleanup; + goto endjob; if (virDomainVcpuPinAdd(&newVcpuPin, &newVcpuPinNum, cpumap, maplen, -1) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("failed to update vcpupin")); virDomainVcpuPinDefArrayFree(newVcpuPin, newVcpuPinNum); - goto cleanup; + goto endjob; } if (virCgroupHasController(priv->cgroup, @@ -5135,20 +5138,20 @@ qemuDomainPinEmulator(virDomainPtr dom, * Configure the corresponding cpuset cgroup. */ if (virCgroupNewEmulator(priv->cgroup, false, &cgroup_emulator) < 0) - goto cleanup; + goto endjob; if (qemuSetupCgroupEmulatorPin(cgroup_emulator, newVcpuPin[0]->cpumask) < 0) { virReportError(VIR_ERR_OPERATION_INVALID, "%s", _("failed to set cpuset.cpus in cgroup" " for emulator threads")); - goto cleanup; + goto endjob; } } else { if (virProcessSetAffinity(pid, pcpumap) < 0) { virReportError(VIR_ERR_SYSTEM_ERROR, "%s", _("failed to set cpu affinity for " "emulator threads")); - goto cleanup; + goto endjob; } } @@ -5157,7 +5160,7 @@ qemuDomainPinEmulator(virDomainPtr dom, virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("failed to delete emulatorpin xml of " "a running domain")); - goto cleanup; + goto endjob; } } else { virDomainVcpuPinDefFree(vm->def->cputune.emulatorpin); @@ -5170,18 +5173,18 @@ qemuDomainPinEmulator(virDomainPtr dom, } else { virReportError(VIR_ERR_OPERATION_INVALID, "%s", _("cpu affinity is not supported")); - goto cleanup; + goto endjob; } if (virDomainSaveStatus(driver->xmlopt, cfg->stateDir, vm) < 0) - goto cleanup; + goto endjob; str = virBitmapFormat(pcpumap); if (virTypedParamsAddString(&eventParams, &eventNparams, &eventMaxparams, VIR_DOMAIN_TUNABLE_CPU_EMULATORPIN, str) < 0) - goto cleanup; + goto endjob; event = virDomainEventTunableNewFromDom(dom, eventParams, eventNparams); } @@ -5193,23 +5196,26 @@ qemuDomainPinEmulator(virDomainPtr dom, virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("failed to delete emulatorpin xml of " "a persistent domain")); - goto cleanup; + goto endjob; } } else { if (virDomainEmulatorPinAdd(persistentDef, cpumap, maplen) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("failed to update or add emulatorpin xml " "of a persistent domain")); - goto cleanup; + goto endjob; } } ret = virDomainSaveConfig(cfg->configDir, persistentDef); - goto cleanup; + goto endjob; } ret = 0; + endjob: + qemuDomainObjEndJob(driver, vm); + cleanup: if (cgroup_emulator) virCgroupFree(&cgroup_emulator); -- 2.2.1
The code modifies the domain configuration but doesn't take a MODIFY type job to do so. This patch also fixes a few very long lines of code around the touched parts. --- src/qemu/qemu_driver.c | 24 +++++++++++++++++------- 1 file changed, 17 insertions(+), 7 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index fa2259a..ecb3693 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -8040,35 +8040,45 @@ static int qemuDomainSetAutostart(virDomainPtr dom, autostart = (autostart != 0); if (vm->autostart != autostart) { - if ((configFile = virDomainConfigFile(cfg->configDir, vm->def->name)) == NULL) - goto cleanup; - if ((autostartLink = virDomainConfigFile(cfg->autostartDir, vm->def->name)) == NULL) + if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0) goto cleanup; + if (!(configFile = virDomainConfigFile(cfg->configDir, vm->def->name))) + goto endjob; + + if (!(autostartLink = virDomainConfigFile(cfg->autostartDir, + vm->def->name))) + goto endjob; + if (autostart) { if (virFileMakePath(cfg->autostartDir) < 0) { virReportSystemError(errno, _("cannot create autostart directory %s"), cfg->autostartDir); - goto cleanup; + goto endjob; } if (symlink(configFile, autostartLink) < 0) { virReportSystemError(errno, _("Failed to create symlink '%s to '%s'"), autostartLink, configFile); - goto cleanup; + goto endjob; } } else { - if (unlink(autostartLink) < 0 && errno != ENOENT && errno != ENOTDIR) { + if (unlink(autostartLink) < 0 && + errno != ENOENT && + errno != ENOTDIR) { virReportSystemError(errno, _("Failed to delete symlink '%s'"), autostartLink); - goto cleanup; + goto endjob; } } vm->autostart = autostart; + + endjob: + qemuDomainObjEndJob(driver, vm); } ret = 0; -- 2.2.1
On 01/22/2015 10:20 AM, Peter Krempa wrote:
The code modifies the domain configuration but doesn't take a MODIFY type job to do so.
This patch also fixes a few very long lines of code around the touched parts. --- src/qemu/qemu_driver.c | 24 +++++++++++++++++------- 1 file changed, 17 insertions(+), 7 deletions(-)
ACK Jan
The code modifies the domain configuration but doesn't take a MODIFY type job to do so. --- src/qemu/qemu_driver.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index ecb3693..f6b2967 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -9043,6 +9043,9 @@ qemuDomainSetMemoryParameters(virDomainPtr dom, } } + if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0) + goto cleanup; + #define QEMU_SET_MEM_PARAMETER(FUNC, VALUE) \ if (set_ ## VALUE) { \ if (flags & VIR_DOMAIN_AFFECT_LIVE) { \ @@ -9050,7 +9053,7 @@ qemuDomainSetMemoryParameters(virDomainPtr dom, virReportSystemError(-rc, _("unable to set memory %s tunable"), \ #VALUE); \ \ - goto cleanup; \ + goto endjob; \ } \ vm->def->mem.VALUE = VALUE; \ } \ @@ -9078,10 +9081,13 @@ qemuDomainSetMemoryParameters(virDomainPtr dom, if (flags & VIR_DOMAIN_AFFECT_CONFIG && virDomainSaveConfig(cfg->configDir, persistentDef) < 0) - goto cleanup; + goto endjob; ret = 0; + endjob: + qemuDomainObjEndJob(driver, vm); + cleanup: qemuDomObjEndAPI(&vm); virObjectUnref(caps); -- 2.2.1
The code modifies the domain configuration but doesn't take a MODIFY type job to do so. --- src/qemu/qemu_driver.c | 32 +++++++++++++++++++------------- 1 file changed, 19 insertions(+), 13 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index f6b2967..c52821a 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -9679,6 +9679,9 @@ qemuDomainSetSchedulerParametersFlags(virDomainPtr dom, } } + if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0) + goto cleanup; + for (i = 0; i < nparams; i++) { virTypedParameterPtr param = ¶ms[i]; value_ul = param->value.ul; @@ -9688,10 +9691,10 @@ qemuDomainSetSchedulerParametersFlags(virDomainPtr dom, if (flags & VIR_DOMAIN_AFFECT_LIVE) { unsigned long long val; if (virCgroupSetCpuShares(priv->cgroup, value_ul) < 0) - goto cleanup; + goto endjob; if (virCgroupGetCpuShares(priv->cgroup, &val) < 0) - goto cleanup; + goto endjob; vm->def->cputune.shares = val; vm->def->cputune.sharesSpecified = true; @@ -9700,7 +9703,7 @@ qemuDomainSetSchedulerParametersFlags(virDomainPtr dom, &eventMaxNparams, VIR_DOMAIN_TUNABLE_CPU_CPU_SHARES, val) < 0) - goto cleanup; + goto endjob; } if (flags & VIR_DOMAIN_AFFECT_CONFIG) { @@ -9715,7 +9718,7 @@ qemuDomainSetSchedulerParametersFlags(virDomainPtr dom, if (flags & VIR_DOMAIN_AFFECT_LIVE && value_ul) { if ((rc = qemuSetVcpusBWLive(vm, priv->cgroup, value_ul, 0))) - goto cleanup; + goto endjob; vm->def->cputune.period = value_ul; @@ -9723,7 +9726,7 @@ qemuDomainSetSchedulerParametersFlags(virDomainPtr dom, &eventMaxNparams, VIR_DOMAIN_TUNABLE_CPU_VCPU_PERIOD, value_ul) < 0) - goto cleanup; + goto endjob; } if (flags & VIR_DOMAIN_AFFECT_CONFIG) @@ -9735,7 +9738,7 @@ qemuDomainSetSchedulerParametersFlags(virDomainPtr dom, if (flags & VIR_DOMAIN_AFFECT_LIVE && value_l) { if ((rc = qemuSetVcpusBWLive(vm, priv->cgroup, 0, value_l))) - goto cleanup; + goto endjob; vm->def->cputune.quota = value_l; @@ -9743,7 +9746,7 @@ qemuDomainSetSchedulerParametersFlags(virDomainPtr dom, &eventMaxNparams, VIR_DOMAIN_TUNABLE_CPU_VCPU_QUOTA, value_l) < 0) - goto cleanup; + goto endjob; } if (flags & VIR_DOMAIN_AFFECT_CONFIG) @@ -9756,7 +9759,7 @@ qemuDomainSetSchedulerParametersFlags(virDomainPtr dom, if (flags & VIR_DOMAIN_AFFECT_LIVE && value_ul) { if ((rc = qemuSetEmulatorBandwidthLive(vm, priv->cgroup, value_ul, 0))) - goto cleanup; + goto endjob; vm->def->cputune.emulator_period = value_ul; @@ -9764,7 +9767,7 @@ qemuDomainSetSchedulerParametersFlags(virDomainPtr dom, &eventMaxNparams, VIR_DOMAIN_TUNABLE_CPU_EMULATOR_PERIOD, value_ul) < 0) - goto cleanup; + goto endjob; } if (flags & VIR_DOMAIN_AFFECT_CONFIG) @@ -9777,7 +9780,7 @@ qemuDomainSetSchedulerParametersFlags(virDomainPtr dom, if (flags & VIR_DOMAIN_AFFECT_LIVE && value_l) { if ((rc = qemuSetEmulatorBandwidthLive(vm, priv->cgroup, 0, value_l))) - goto cleanup; + goto endjob; vm->def->cputune.emulator_quota = value_l; @@ -9785,7 +9788,7 @@ qemuDomainSetSchedulerParametersFlags(virDomainPtr dom, &eventMaxNparams, VIR_DOMAIN_TUNABLE_CPU_EMULATOR_QUOTA, value_l) < 0) - goto cleanup; + goto endjob; } if (flags & VIR_DOMAIN_AFFECT_CONFIG) @@ -9794,7 +9797,7 @@ qemuDomainSetSchedulerParametersFlags(virDomainPtr dom, } if (virDomainSaveStatus(driver->xmlopt, cfg->stateDir, vm) < 0) - goto cleanup; + goto endjob; if (eventNparams) { event = virDomainEventTunableNewFromDom(dom, eventParams, eventNparams); @@ -9806,7 +9809,7 @@ qemuDomainSetSchedulerParametersFlags(virDomainPtr dom, if (flags & VIR_DOMAIN_AFFECT_CONFIG) { rc = virDomainSaveConfig(cfg->configDir, vmdef); if (rc < 0) - goto cleanup; + goto endjob; virDomainObjAssignDef(vm, vmdef, false, NULL); vmdef = NULL; @@ -9814,6 +9817,9 @@ qemuDomainSetSchedulerParametersFlags(virDomainPtr dom, ret = 0; + endjob: + qemuDomainObjEndJob(driver, vm); + cleanup: virDomainDefFree(vmdef); qemuDomObjEndAPI(&vm); -- 2.2.1
The function just queries status so there's no need for a MODIFY type job. --- src/qemu/qemu_driver.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index c52821a..2c3c3e0 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -17041,7 +17041,7 @@ qemuDomainGetBlockIoTune(virDomainPtr dom, if (!(caps = virQEMUDriverGetCapabilities(driver, false))) goto cleanup; - if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0) + if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_QUERY) < 0) goto cleanup; if (virDomainLiveConfigHelperMethod(caps, driver->xmlopt, vm, &flags, -- 2.2.1
On 01/22/2015 10:20 AM, Peter Krempa wrote:
The function just queries status so there's no need for a MODIFY type job. --- src/qemu/qemu_driver.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
ACK Jan
The code modifies the domain configuration but doesn't take a MODIFY type job to do so. --- src/qemu/qemu_driver.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 2c3c3e0..ab65e9b 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -17309,10 +17309,15 @@ qemuDomainSetMetadata(virDomainPtr dom, if (!(caps = virQEMUDriverGetCapabilities(driver, false))) goto cleanup; + if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0) + goto cleanup; + ret = virDomainObjSetMetadata(vm, type, metadata, key, uri, caps, driver->xmlopt, cfg->stateDir, cfg->configDir, flags); + qemuDomainObjEndJob(driver, vm); + cleanup: qemuDomObjEndAPI(&vm); virObjectUnref(caps); -- 2.2.1
On 01/22/2015 10:20 AM, Peter Krempa wrote:
The code modifies the domain configuration but doesn't take a MODIFY type job to do so. --- src/qemu/qemu_driver.c | 5 +++++ 1 file changed, 5 insertions(+)
ACK After getting the job, the virDomainLiveConfigHelperMethod is called inside virDomainObjSetMetadata checking if the domain is alive again. Jan
On 01/22/2015 10:20 AM, Peter Krempa wrote:
While reviewing Martin's reference counting series I've noticed a few qemu API impls that don't properly handle jobs.
Peter Krempa (7): qemu: Fix job handling in qemuDomainPinVcpuFlags qemu: Fix job handling in qemuDomainPinEmulator qemu: Fix job handling in qemuDomainSetAutostart qemu: Fix job handling in qemuDomainSetMemoryParameters qemu: Fix job handling in qemuDomainSetSchedulerParametersFlags qemu: Fix job type in qemuDomainGetBlockIoTune qemu: Fix job handling in qemuDomainSetMetadata
src/qemu/qemu_driver.c | 135 +++++++++++++++++++++++++++++++------------------ 1 file changed, 87 insertions(+), 48 deletions(-)
In patches 1-3,5 it's necessary to check if the domain is still alive after getting the job. I suggest moving the BeginJob between the ACL check and virDomainLiveConfigHelperMethod, which does check for domain liveness. ACK with that fix. Jan
On Thu, Jan 22, 2015 at 15:23:03 +0100, Ján Tomko wrote:
On 01/22/2015 10:20 AM, Peter Krempa wrote:
While reviewing Martin's reference counting series I've noticed a few qemu API impls that don't properly handle jobs.
Peter Krempa (7): qemu: Fix job handling in qemuDomainPinVcpuFlags qemu: Fix job handling in qemuDomainPinEmulator qemu: Fix job handling in qemuDomainSetAutostart qemu: Fix job handling in qemuDomainSetMemoryParameters qemu: Fix job handling in qemuDomainSetSchedulerParametersFlags qemu: Fix job type in qemuDomainGetBlockIoTune qemu: Fix job handling in qemuDomainSetMetadata
src/qemu/qemu_driver.c | 135 +++++++++++++++++++++++++++++++------------------ 1 file changed, 87 insertions(+), 48 deletions(-)
In patches 1-3,5 it's necessary to check if the domain is still alive after getting the job.
I suggest moving the BeginJob between the ACL check and virDomainLiveConfigHelperMethod, which does check for domain liveness.
ACK with that fix.
I've moved the job acquisition to the place you've suggested and pushed the series now that the release is out. Thanks Peter
participants (2)
-
Ján Tomko -
Peter Krempa