[libvirt] [PATCH 00/13] vCPU pinning and related refactors - Part 1.5
Few more fixes to the Part 1 and related stuff. This is a continuation and fix to stuff done in Part 1. Peter Krempa (13): conf: Fix virDomainObjGetDefs when getting persistent config on a live vm conf: Introduce helper to help getting correct def for getter functions qemu: Simplify qemuDomainGetInterfaceParameters by using virDomainObjGetOneDef qemu: Simplify qemuDomainGetNumaParameters by using virDomainObjGetOneDef qemu: Simplify qemuDomainGetVcpuPinInfo by using virDomainObjGetOneDef qemu: Simplify qemuDomainGetEmulatorPinInfo by using virDomainObjGetOneDef qemu: Simplify qemuDomainGetVcpusFlags by using virDomainObjGetOneDef qemu: Simplify qemuDomainSetInterfaceParameters by using virDomainObjGetDefs qemu: Refactor qemuDomainSetNumaParameters qemu: Refactor qemuDomainGetMemoryParameters qemu: Reuse virDomainObjGetDefs in qemuDomainGetMemoryParameters qemu: 'privileged' flag is not really configuration conf: Move vcpu info parsing code into a separate function src/conf/domain_conf.c | 174 ++++++++++++++++--------- src/conf/domain_conf.h | 1 + src/libvirt_private.syms | 1 + src/qemu/qemu_cgroup.c | 13 +- src/qemu/qemu_command.c | 9 +- src/qemu/qemu_conf.c | 7 +- src/qemu/qemu_conf.h | 5 +- src/qemu/qemu_domain.c | 4 +- src/qemu/qemu_driver.c | 332 ++++++++++++++--------------------------------- tests/qemuxml2argvtest.c | 4 +- 10 files changed, 241 insertions(+), 309 deletions(-) -- 2.4.1
If @flags contains only VIR_DOMAIN_AFFECT_CONFIG and @vm is active, the function would return the active config rather than the persistent one that it should return. This happened due to the fact that virDomainObjGetDefs was checking the updated flags which may not contain VIR_DOMAIN_AFFECT_LIVE if it is not requested even if @vm is active. Additionally the function would not take the flags into account when setting the pointers which was later used to determine whether the code needs to update the given configuration. The mistake was caught by the virt-test suite. --- Version 2 actually fixes the function. src/conf/domain_conf.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index ca55981..3cc182b 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -2933,11 +2933,11 @@ virDomainObjGetDefs(virDomainObjPtr vm, if (virDomainObjUpdateModificationImpact(vm, &flags) < 0) return -1; - if (flags & VIR_DOMAIN_AFFECT_LIVE) { - if (liveDef) + if (virDomainObjIsActive(vm)) { + if (liveDef && (flags & VIR_DOMAIN_AFFECT_LIVE)) *liveDef = vm->def; - if (persDef) + if (persDef && (flags & VIR_DOMAIN_AFFECT_CONFIG)) *persDef = vm->newDef; } else { if (persDef) -- 2.4.1
On 06/15/2015 03:47 PM, Peter Krempa wrote:
If @flags contains only VIR_DOMAIN_AFFECT_CONFIG and @vm is active, the function would return the active config rather than the persistent one that it should return. This happened due to the fact that virDomainObjGetDefs was checking the updated flags which may not contain VIR_DOMAIN_AFFECT_LIVE if it is not requested even if @vm is active.
Additionally the function would not take the flags into account when setting the pointers which was later used to determine whether the code needs to update the given configuration.
The mistake was caught by the virt-test suite. --- Version 2 actually fixes the function.
src/conf/domain_conf.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
ACK John
virDomainObjGetOneDef will help to retrieve the correct definition pointer from @vm in cases where VIR_DOMAIN_AFFECT_LIVE and VIR_DOMAIN_AFFECT_CONFIG are mutually exclusive. The function simply returns the correct pointer. This similarly to virDomainObjGetDefs will greatly simplify the code. --- src/conf/domain_conf.c | 36 ++++++++++++++++++++++++++++++++++++ src/conf/domain_conf.h | 1 + src/libvirt_private.syms | 1 + 3 files changed, 38 insertions(+) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 3cc182b..35e1cb4 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -2948,6 +2948,42 @@ virDomainObjGetDefs(virDomainObjPtr vm, } +/** + * virDomainObjGetOneDef: + * + * @vm: Domain object + * @flags: for virDomainModificationImpact + * + * Helper function to resolve @flags and return the correct domain pointer + * object. This function returns one of @vm->def or @vm->persistentDef + * according to @flags. This helper should be used only in APIs that guarantee + * that @flags contains exactly one of VIR_DOMAIN_AFFECT_LIVE, + * VIR_DOMAIN_AFFECT_CONFIG. + * + * Returns the correct definition pointer or NULL on error. + */ +virDomainDefPtr +virDomainObjGetOneDef(virDomainObjPtr vm, + unsigned int flags) +{ + if (flags & VIR_DOMAIN_AFFECT_LIVE && flags & VIR_DOMAIN_AFFECT_CONFIG) { + virReportInvalidArg(ctl, "%s", + _("Flags 'VIR_DOMAIN_AFFECT_LIVE' and " + "'VIR_DOMAIN_AFFECT_CONFIG' are mutually " + "exclusive")); + return NULL; + } + + if (virDomainObjUpdateModificationImpact(vm, &flags) < 0) + return NULL; + + if (virDomainObjIsActive(vm) && flags & VIR_DOMAIN_AFFECT_CONFIG) + return vm->newDef; + else + return vm->def; +} + + /* * The caller must hold a lock on the driver owning 'doms', * and must also have locked 'dom', to ensure no one else diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index ba17a8d..db49d46 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -2553,6 +2553,7 @@ int virDomainObjGetDefs(virDomainObjPtr vm, unsigned int flags, virDomainDefPtr *liveDef, virDomainDefPtr *persDef); +virDomainDefPtr virDomainObjGetOneDef(virDomainObjPtr vm, unsigned int flags); int virDomainLiveConfigHelperMethod(virCapsPtr caps, diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index dc8a52d..858c00f 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -385,6 +385,7 @@ virDomainObjEndAPI; virDomainObjFormat; virDomainObjGetDefs; virDomainObjGetMetadata; +virDomainObjGetOneDef; virDomainObjGetPersistentDef; virDomainObjGetState; virDomainObjListAdd; -- 2.4.1
On 06/15/2015 03:47 PM, Peter Krempa wrote:
virDomainObjGetOneDef will help to retrieve the correct definition pointer from @vm in cases where VIR_DOMAIN_AFFECT_LIVE and VIR_DOMAIN_AFFECT_CONFIG are mutually exclusive. The function simply returns the correct pointer. This similarly to virDomainObjGetDefs will greatly simplify the code. --- src/conf/domain_conf.c | 36 ++++++++++++++++++++++++++++++++++++ src/conf/domain_conf.h | 1 + src/libvirt_private.syms | 1 + 3 files changed, 38 insertions(+)
I dunno - I just have this little voice asking is there some corner case from the prior virDomainLiveConfigHelperMethod thruough possibly creating 'newDef' in virDomainObjSetDefTransient that this new code will miss... IOW - is there a condition where CONFIG is wanted, domain is running, but newDef is NULL. OTOH - one wonders if the previous code went through the patch into virDomainObjSetDefTransient All the callers check for NULL, so that's not an issue - just considering some strange corner case for a transient domain.
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 3cc182b..35e1cb4 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -2948,6 +2948,42 @@ virDomainObjGetDefs(virDomainObjPtr vm, }
+/** + * virDomainObjGetOneDef: + * + * @vm: Domain object + * @flags: for virDomainModificationImpact + * + * Helper function to resolve @flags and return the correct domain pointer + * object. This function returns one of @vm->def or @vm->persistentDef + * according to @flags. This helper should be used only in APIs that guarantee + * that @flags contains exactly one of VIR_DOMAIN_AFFECT_LIVE,
s/,/ or/
+ * VIR_DOMAIN_AFFECT_CONFIG.
and to be really redundant ;-) - couldn't resist. s/./ and not both./ ACK - although "GetOneDef" isn't my favorite, I don't have a better suggestion either. John
+ * + * Returns the correct definition pointer or NULL on error. + */ +virDomainDefPtr +virDomainObjGetOneDef(virDomainObjPtr vm, + unsigned int flags) +{ + if (flags & VIR_DOMAIN_AFFECT_LIVE && flags & VIR_DOMAIN_AFFECT_CONFIG) { + virReportInvalidArg(ctl, "%s", + _("Flags 'VIR_DOMAIN_AFFECT_LIVE' and " + "'VIR_DOMAIN_AFFECT_CONFIG' are mutually " + "exclusive")); + return NULL; + } + + if (virDomainObjUpdateModificationImpact(vm, &flags) < 0) + return NULL; + + if (virDomainObjIsActive(vm) && flags & VIR_DOMAIN_AFFECT_CONFIG) + return vm->newDef; + else + return vm->def; +} + + /* * The caller must hold a lock on the driver owning 'doms', * and must also have locked 'dom', to ensure no one else diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index ba17a8d..db49d46 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -2553,6 +2553,7 @@ int virDomainObjGetDefs(virDomainObjPtr vm, unsigned int flags, virDomainDefPtr *liveDef, virDomainDefPtr *persDef); +virDomainDefPtr virDomainObjGetOneDef(virDomainObjPtr vm, unsigned int flags);
int virDomainLiveConfigHelperMethod(virCapsPtr caps, diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index dc8a52d..858c00f 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -385,6 +385,7 @@ virDomainObjEndAPI; virDomainObjFormat; virDomainObjGetDefs; virDomainObjGetMetadata; +virDomainObjGetOneDef; virDomainObjGetPersistentDef; virDomainObjGetState; virDomainObjListAdd;
On Wed, Jun 17, 2015 at 16:53:14 -0400, John Ferlan wrote:
On 06/15/2015 03:47 PM, Peter Krempa wrote:
virDomainObjGetOneDef will help to retrieve the correct definition pointer from @vm in cases where VIR_DOMAIN_AFFECT_LIVE and VIR_DOMAIN_AFFECT_CONFIG are mutually exclusive. The function simply returns the correct pointer. This similarly to virDomainObjGetDefs will greatly simplify the code. --- src/conf/domain_conf.c | 36 ++++++++++++++++++++++++++++++++++++ src/conf/domain_conf.h | 1 + src/libvirt_private.syms | 1 + 3 files changed, 38 insertions(+)
I dunno - I just have this little voice asking is there some corner case from the prior virDomainLiveConfigHelperMethod thruough possibly creating 'newDef' in virDomainObjSetDefTransient that this new code will miss... IOW - is there a condition where CONFIG is wanted, domain is running, but newDef is NULL.
Well, up until now I've inspected the qemu driver and the test driver and both set the transient def upon start of a VM, so we should be fine using this in those. Honestly I think that all drivers should be fixed and virDomainLiveConfigHelperMethod should be killed with fire forever.
OTOH - one wonders if the previous code went through the patch into virDomainObjSetDefTransient
All the callers check for NULL, so that's not an issue - just considering some strange corner case for a transient domain.
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 3cc182b..35e1cb4 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -2948,6 +2948,42 @@ virDomainObjGetDefs(virDomainObjPtr vm, }
+/** + * virDomainObjGetOneDef: + * + * @vm: Domain object + * @flags: for virDomainModificationImpact + * + * Helper function to resolve @flags and return the correct domain pointer + * object. This function returns one of @vm->def or @vm->persistentDef + * according to @flags. This helper should be used only in APIs that guarantee + * that @flags contains exactly one of VIR_DOMAIN_AFFECT_LIVE,
s/,/ or/
+ * VIR_DOMAIN_AFFECT_CONFIG.
and to be really redundant ;-) - couldn't resist.
s/./ and not both./
ACK - although "GetOneDef" isn't my favorite, I don't have a better suggestion either.
Me neither but that can be changed in the future.
John
I've pushed this series with the suggested changes and I'll follow up with fixes to the endjob label and the double space. Thanks. Peter
--- src/qemu/qemu_driver.c | 16 +--------------- 1 file changed, 1 insertion(+), 15 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index d1f195c..dde94e8 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -11468,32 +11468,23 @@ qemuDomainGetInterfaceParameters(virDomainPtr dom, int *nparams, unsigned int flags) { - virQEMUDriverPtr driver = dom->conn->privateData; size_t i; virDomainObjPtr vm = NULL; virDomainDefPtr def = NULL; - virDomainDefPtr persistentDef = NULL; virDomainNetDefPtr net = NULL; int ret = -1; - virCapsPtr caps = NULL; virCheckFlags(VIR_DOMAIN_AFFECT_LIVE | VIR_DOMAIN_AFFECT_CONFIG | VIR_TYPED_PARAM_STRING_OKAY, -1); - flags &= ~VIR_TYPED_PARAM_STRING_OKAY; - if (!(vm = qemuDomObjFromDomain(dom))) return -1; if (virDomainGetInterfaceParametersEnsureACL(dom->conn, vm->def) < 0) goto cleanup; - if (!(caps = virQEMUDriverGetCapabilities(driver, false))) - goto cleanup; - - if (virDomainLiveConfigHelperMethod(caps, driver->xmlopt, vm, &flags, - &persistentDef) < 0) + if (!(def = virDomainObjGetOneDef(vm, flags))) goto cleanup; if ((*nparams) == 0) { @@ -11502,10 +11493,6 @@ qemuDomainGetInterfaceParameters(virDomainPtr dom, goto cleanup; } - def = persistentDef; - if (!def) - def = vm->def; - net = virDomainNetFind(def, device); if (!net) { virReportError(VIR_ERR_INVALID_ARG, @@ -11576,7 +11563,6 @@ qemuDomainGetInterfaceParameters(virDomainPtr dom, cleanup: virDomainObjEndAPI(&vm); - virObjectUnref(caps); return ret; } -- 2.4.1
--- src/qemu/qemu_driver.c | 20 +------------------- 1 file changed, 1 insertion(+), 19 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index dde94e8..e8b2be3 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -10277,36 +10277,24 @@ qemuDomainGetNumaParameters(virDomainPtr dom, int *nparams, unsigned int flags) { - virQEMUDriverPtr driver = dom->conn->privateData; size_t i; virDomainObjPtr vm = NULL; - virDomainDefPtr persistentDef = NULL; virDomainNumatuneMemMode tmpmode = VIR_DOMAIN_NUMATUNE_MEM_STRICT; char *nodeset = NULL; int ret = -1; - virCapsPtr caps = NULL; virDomainDefPtr def = NULL; virCheckFlags(VIR_DOMAIN_AFFECT_LIVE | VIR_DOMAIN_AFFECT_CONFIG | VIR_TYPED_PARAM_STRING_OKAY, -1); - /* We blindly return a string, and let libvirt.c and - * remote_driver.c do the filtering on behalf of older clients - * that can't parse it. */ - flags &= ~VIR_TYPED_PARAM_STRING_OKAY; - if (!(vm = qemuDomObjFromDomain(dom))) return -1; if (virDomainGetNumaParametersEnsureACL(dom->conn, vm->def) < 0) goto cleanup; - if (!(caps = virQEMUDriverGetCapabilities(driver, false))) - goto cleanup; - - if (virDomainLiveConfigHelperMethod(caps, driver->xmlopt, vm, &flags, - &persistentDef) < 0) + if (!(def = virDomainObjGetOneDef(vm, flags))) goto cleanup; if ((*nparams) == 0) { @@ -10315,11 +10303,6 @@ qemuDomainGetNumaParameters(virDomainPtr dom, goto cleanup; } - if (flags & VIR_DOMAIN_AFFECT_CONFIG) - def = persistentDef; - else - def = vm->def; - for (i = 0; i < QEMU_NB_NUMA_PARAM && i < *nparams; i++) { virMemoryParameterPtr param = ¶ms[i]; @@ -10357,7 +10340,6 @@ qemuDomainGetNumaParameters(virDomainPtr dom, cleanup: VIR_FREE(nodeset); virDomainObjEndAPI(&vm); - virObjectUnref(caps); return ret; } -- 2.4.1
virDomainObjGetOneDef is simpler to use than virDomainObjGetDefs --- src/qemu/qemu_driver.c | 14 +++++--------- 1 file changed, 5 insertions(+), 9 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index e8b2be3..f3c53f5 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -5213,7 +5213,6 @@ qemuDomainGetVcpuPinInfo(virDomainPtr dom, { virDomainObjPtr vm = NULL; virDomainDefPtr def; - virDomainDefPtr targetDef; int ret = -1; int hostcpus, vcpu; virBitmapPtr allcpumap = NULL; @@ -5227,12 +5226,9 @@ qemuDomainGetVcpuPinInfo(virDomainPtr dom, if (virDomainGetVcpuPinInfoEnsureACL(dom->conn, vm->def) < 0) goto cleanup; - if (virDomainObjGetDefs(vm, flags, &def, &targetDef) < 0) + if (!(def = virDomainObjGetOneDef(vm, flags))) goto cleanup; - if (def) - targetDef = def; - if ((hostcpus = nodeGetCPUCount()) < 0) goto cleanup; @@ -5242,8 +5238,8 @@ qemuDomainGetVcpuPinInfo(virDomainPtr dom, virBitmapSetAll(allcpumap); /* Clamp to actual number of vcpus */ - if (ncpumaps > targetDef->vcpus) - ncpumaps = targetDef->vcpus; + if (ncpumaps > def->vcpus) + ncpumaps = def->vcpus; if (ncpumaps < 1) goto cleanup; @@ -5252,8 +5248,8 @@ qemuDomainGetVcpuPinInfo(virDomainPtr dom, virDomainPinDefPtr pininfo; virBitmapPtr bitmap = NULL; - pininfo = virDomainPinFind(targetDef->cputune.vcpupin, - targetDef->cputune.nvcpupin, + pininfo = virDomainPinFind(def->cputune.vcpupin, + def->cputune.nvcpupin, vcpu); if (pininfo && pininfo->cpumask) -- 2.4.1
virDomainObjGetOneDef is simpler to use than virDomainObjGetDefs --- src/qemu/qemu_driver.c | 14 +++++--------- 1 file changed, 5 insertions(+), 9 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index f3c53f5..c878409 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -5400,7 +5400,6 @@ qemuDomainGetEmulatorPinInfo(virDomainPtr dom, { virDomainObjPtr vm = NULL; virDomainDefPtr def; - virDomainDefPtr targetDef; int ret = -1; int hostcpus; virBitmapPtr cpumask = NULL; @@ -5415,19 +5414,16 @@ qemuDomainGetEmulatorPinInfo(virDomainPtr dom, if (virDomainGetEmulatorPinInfoEnsureACL(dom->conn, vm->def) < 0) goto cleanup; - if (virDomainObjGetDefs(vm, flags, &def, &targetDef) < 0) + if (!(def = virDomainObjGetOneDef(vm, flags))) goto cleanup; - if (def) - targetDef = def; - if ((hostcpus = nodeGetCPUCount()) < 0) goto cleanup; - if (targetDef->cputune.emulatorpin) { - cpumask = targetDef->cputune.emulatorpin; - } else if (targetDef->cpumask) { - cpumask = targetDef->cpumask; + if (def->cputune.emulatorpin) { + cpumask = def->cputune.emulatorpin; + } else if (def->cpumask) { + cpumask = def->cpumask; } else { if (!(bitmap = virBitmapNew(hostcpus))) goto cleanup; -- 2.4.1
virDomainObjGetOneDef is simpler to use than virDomainObjGetDefs --- src/qemu/qemu_driver.c | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index c878409..2cb0215 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -5479,7 +5479,6 @@ qemuDomainGetVcpusFlags(virDomainPtr dom, unsigned int flags) qemuDomainObjPrivatePtr priv; virDomainObjPtr vm; virDomainDefPtr def; - virDomainDefPtr persistentDef; int ret = -1; qemuAgentCPUInfoPtr cpuinfo = NULL; int ncpuinfo = -1; @@ -5498,11 +5497,11 @@ qemuDomainGetVcpusFlags(virDomainPtr dom, unsigned int flags) if (virDomainGetVcpusFlagsEnsureACL(dom->conn, vm->def, flags) < 0) goto cleanup; - if (virDomainObjGetDefs(vm, flags, &def, &persistentDef) < 0) + if (!(def = virDomainObjGetOneDef(vm, flags))) goto cleanup; if (flags & VIR_DOMAIN_VCPU_GUEST) { - if (persistentDef) { + if (!virDomainObjIsActive(vm)) { virReportError(VIR_ERR_INVALID_ARG, "%s", _("vCPU count provided by the guest agent can only be " " requested for live domains")); @@ -5543,9 +5542,6 @@ qemuDomainGetVcpusFlags(virDomainPtr dom, unsigned int flags) ret++; } } else { - if (!def) - def = persistentDef; - if (flags & VIR_DOMAIN_VCPU_MAXIMUM) ret = def->maxvcpus; else -- 2.4.1
On 06/15/2015 03:47 PM, Peter Krempa wrote:
virDomainObjGetOneDef is simpler to use than virDomainObjGetDefs --- src/qemu/qemu_driver.c | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-)
ACK - 3/13 to 7/13 although there's an "unrelated", but since I was here NIT below. John
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index c878409..2cb0215 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -5479,7 +5479,6 @@ qemuDomainGetVcpusFlags(virDomainPtr dom, unsigned int flags) qemuDomainObjPrivatePtr priv; virDomainObjPtr vm; virDomainDefPtr def; - virDomainDefPtr persistentDef; int ret = -1; qemuAgentCPUInfoPtr cpuinfo = NULL; int ncpuinfo = -1; @@ -5498,11 +5497,11 @@ qemuDomainGetVcpusFlags(virDomainPtr dom, unsigned int flags) if (virDomainGetVcpusFlagsEnsureACL(dom->conn, vm->def, flags) < 0) goto cleanup;
- if (virDomainObjGetDefs(vm, flags, &def, &persistentDef) < 0) + if (!(def = virDomainObjGetOneDef(vm, flags))) goto cleanup;
if (flags & VIR_DOMAIN_VCPU_GUEST) { - if (persistentDef) { + if (!virDomainObjIsActive(vm)) { virReportError(VIR_ERR_INVALID_ARG, "%s", _("vCPU count provided by the guest agent can only be " " requested for live domains"));
Existing, but there will be a double space in the output message...
@@ -5543,9 +5542,6 @@ qemuDomainGetVcpusFlags(virDomainPtr dom, unsigned int flags) ret++; } } else { - if (!def) - def = persistentDef; - if (flags & VIR_DOMAIN_VCPU_MAXIMUM) ret = def->maxvcpus; else
--- src/qemu/qemu_driver.c | 40 ++++++++++++++++------------------------ 1 file changed, 16 insertions(+), 24 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 2cb0215..ceadc31 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -11246,12 +11246,12 @@ qemuDomainSetInterfaceParameters(virDomainPtr dom, virQEMUDriverPtr driver = dom->conn->privateData; size_t i; virDomainObjPtr vm = NULL; - virDomainDefPtr persistentDef = NULL; + virDomainDefPtr def; + virDomainDefPtr persistentDef; int ret = -1; virDomainNetDefPtr net = NULL, persistentNet = NULL; virNetDevBandwidthPtr bandwidth = NULL, newBandwidth = NULL; virQEMUDriverConfigPtr cfg = NULL; - virCapsPtr caps = NULL; bool inboundSpecified = false, outboundSpecified = false; virCheckFlags(VIR_DOMAIN_AFFECT_LIVE | @@ -11280,31 +11280,24 @@ qemuDomainSetInterfaceParameters(virDomainPtr dom, if (virDomainSetInterfaceParametersEnsureACL(dom->conn, vm->def, flags) < 0) goto cleanup; - if (!(caps = virQEMUDriverGetCapabilities(driver, false))) + if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0) goto cleanup; - if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0) + if (virDomainObjGetDefs(vm, flags, &def, &persistentDef) < 0) goto cleanup; - if (virDomainLiveConfigHelperMethod(caps, driver->xmlopt, vm, &flags, - &persistentDef) < 0) + if (def && + !(net = virDomainNetFind(vm->def, device))) { + virReportError(VIR_ERR_INVALID_ARG, + _("Can't find device %s"), device); goto endjob; - - if (flags & VIR_DOMAIN_AFFECT_LIVE) { - net = virDomainNetFind(vm->def, device); - if (!net) { - virReportError(VIR_ERR_INVALID_ARG, - _("Can't find device %s"), device); - goto endjob; - } } - if (flags & VIR_DOMAIN_AFFECT_CONFIG) { - persistentNet = virDomainNetFind(persistentDef, device); - if (!persistentNet) { - virReportError(VIR_ERR_INVALID_ARG, - _("Can't find device %s"), device); - goto endjob; - } + + if (persistentDef && + !(persistentNet = virDomainNetFind(persistentDef, device))) { + virReportError(VIR_ERR_INVALID_ARG, + _("Can't find device %s"), device); + goto endjob; } if ((VIR_ALLOC(bandwidth) < 0) || @@ -11340,7 +11333,7 @@ qemuDomainSetInterfaceParameters(virDomainPtr dom, if (!bandwidth->out->average) VIR_FREE(bandwidth->out); - if (flags & VIR_DOMAIN_AFFECT_LIVE) { + if (net) { if (VIR_ALLOC(newBandwidth) < 0) goto endjob; @@ -11392,7 +11385,7 @@ qemuDomainSetInterfaceParameters(virDomainPtr dom, goto endjob; } - if (flags & VIR_DOMAIN_AFFECT_CONFIG) { + if (persistentNet) { if (!persistentNet->bandwidth) { persistentNet->bandwidth = bandwidth; bandwidth = NULL; @@ -11426,7 +11419,6 @@ qemuDomainSetInterfaceParameters(virDomainPtr dom, virNetDevBandwidthFree(bandwidth); virNetDevBandwidthFree(newBandwidth); virDomainObjEndAPI(&vm); - virObjectUnref(caps); virObjectUnref(cfg); return ret; } -- 2.4.1
On 06/15/2015 03:47 PM, Peter Krempa wrote:
--- src/qemu/qemu_driver.c | 40 ++++++++++++++++------------------------ 1 file changed, 16 insertions(+), 24 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 2cb0215..ceadc31 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -11246,12 +11246,12 @@ qemuDomainSetInterfaceParameters(virDomainPtr dom, virQEMUDriverPtr driver = dom->conn->privateData; size_t i; virDomainObjPtr vm = NULL; - virDomainDefPtr persistentDef = NULL; + virDomainDefPtr def; + virDomainDefPtr persistentDef; int ret = -1; virDomainNetDefPtr net = NULL, persistentNet = NULL; virNetDevBandwidthPtr bandwidth = NULL, newBandwidth = NULL; virQEMUDriverConfigPtr cfg = NULL; - virCapsPtr caps = NULL; bool inboundSpecified = false, outboundSpecified = false;
virCheckFlags(VIR_DOMAIN_AFFECT_LIVE | @@ -11280,31 +11280,24 @@ qemuDomainSetInterfaceParameters(virDomainPtr dom, if (virDomainSetInterfaceParametersEnsureACL(dom->conn, vm->def, flags) < 0) goto cleanup;
- if (!(caps = virQEMUDriverGetCapabilities(driver, false))) + if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0) goto cleanup;
- if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0) + if (virDomainObjGetDefs(vm, flags, &def, &persistentDef) < 0) goto cleanup;
^^ goto endjob; NOTE: I see this is also an existing issue for qemuDomainPinIOThread - that is the failure to ObjGetDefs goes to cleanup after a BeginJob rather than going to endjob. I know - a separate issue, but it may as well be fixed in this series... ACK with the change here... John
- if (virDomainLiveConfigHelperMethod(caps, driver->xmlopt, vm, &flags, - &persistentDef) < 0) + if (def && + !(net = virDomainNetFind(vm->def, device))) { + virReportError(VIR_ERR_INVALID_ARG, + _("Can't find device %s"), device); goto endjob; - - if (flags & VIR_DOMAIN_AFFECT_LIVE) { - net = virDomainNetFind(vm->def, device); - if (!net) { - virReportError(VIR_ERR_INVALID_ARG, - _("Can't find device %s"), device); - goto endjob; - } } - if (flags & VIR_DOMAIN_AFFECT_CONFIG) { - persistentNet = virDomainNetFind(persistentDef, device); - if (!persistentNet) { - virReportError(VIR_ERR_INVALID_ARG, - _("Can't find device %s"), device); - goto endjob; - } + + if (persistentDef && + !(persistentNet = virDomainNetFind(persistentDef, device))) { + virReportError(VIR_ERR_INVALID_ARG, + _("Can't find device %s"), device); + goto endjob; }
if ((VIR_ALLOC(bandwidth) < 0) || @@ -11340,7 +11333,7 @@ qemuDomainSetInterfaceParameters(virDomainPtr dom, if (!bandwidth->out->average) VIR_FREE(bandwidth->out);
- if (flags & VIR_DOMAIN_AFFECT_LIVE) { + if (net) { if (VIR_ALLOC(newBandwidth) < 0) goto endjob;
@@ -11392,7 +11385,7 @@ qemuDomainSetInterfaceParameters(virDomainPtr dom, goto endjob; }
- if (flags & VIR_DOMAIN_AFFECT_CONFIG) { + if (persistentNet) { if (!persistentNet->bandwidth) { persistentNet->bandwidth = bandwidth; bandwidth = NULL; @@ -11426,7 +11419,6 @@ qemuDomainSetInterfaceParameters(virDomainPtr dom, virNetDevBandwidthFree(bandwidth); virNetDevBandwidthFree(newBandwidth); virDomainObjEndAPI(&vm); - virObjectUnref(caps); virObjectUnref(cfg); return ret; }
Use virDomainObjGetDefs and sanitize the control flow. --- src/qemu/qemu_driver.c | 64 ++++++++++++++++++++++---------------------------- 1 file changed, 28 insertions(+), 36 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index ceadc31..ca04d2c 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -10131,11 +10131,11 @@ qemuDomainSetNumaParameters(virDomainPtr dom, { virQEMUDriverPtr driver = dom->conn->privateData; size_t i; - virDomainDefPtr persistentDef = NULL; + virDomainDefPtr def; + virDomainDefPtr persistentDef; virDomainObjPtr vm = NULL; int ret = -1; virQEMUDriverConfigPtr cfg = NULL; - virCapsPtr caps = NULL; qemuDomainObjPrivatePtr priv; virBitmapPtr nodeset = NULL; virDomainNumatuneMemMode config_mode; @@ -10161,31 +10161,6 @@ qemuDomainSetNumaParameters(virDomainPtr dom, if (virDomainSetNumaParametersEnsureACL(dom->conn, vm->def, flags) < 0) goto cleanup; - if (!(caps = virQEMUDriverGetCapabilities(driver, false))) - goto cleanup; - - if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0) - goto cleanup; - - if (virDomainLiveConfigHelperMethod(caps, driver->xmlopt, vm, &flags, - &persistentDef) < 0) - goto endjob; - - if (!cfg->privileged && - flags & VIR_DOMAIN_AFFECT_LIVE) { - virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", - _("NUMA tuning is not available in session mode")); - goto endjob; - } - - if (flags & VIR_DOMAIN_AFFECT_LIVE) { - if (!virCgroupHasController(priv->cgroup, VIR_CGROUP_CONTROLLER_CPUSET)) { - virReportError(VIR_ERR_OPERATION_INVALID, "%s", - _("cgroup cpuset controller is not mounted")); - goto endjob; - } - } - for (i = 0; i < nparams; i++) { virTypedParameterPtr param = ¶ms[i]; @@ -10195,26 +10170,44 @@ qemuDomainSetNumaParameters(virDomainPtr dom, if (mode < 0 || mode >= VIR_DOMAIN_NUMATUNE_MEM_LAST) { virReportError(VIR_ERR_INVALID_ARG, _("unsupported numatune mode: '%d'"), mode); - goto endjob; + goto cleanup; } } else if (STREQ(param->field, VIR_DOMAIN_NUMA_NODESET)) { if (virBitmapParse(param->value.s, 0, &nodeset, VIR_DOMAIN_CPUMASK_LEN) < 0) - goto endjob; + goto cleanup; if (virBitmapIsAllClear(nodeset)) { virReportError(VIR_ERR_OPERATION_INVALID, _("Invalid nodeset of 'numatune': %s"), param->value.s); - goto endjob; + goto cleanup; } } } - if (flags & VIR_DOMAIN_AFFECT_LIVE) { + if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0) + goto cleanup; + + if (virDomainObjGetDefs(vm, flags, &def, &persistentDef) < 0) + goto endjob; + + if (def) { + if (!cfg->privileged) { + virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", + _("NUMA tuning is not available in session mode")); + goto endjob; + } + + if (!virCgroupHasController(priv->cgroup, VIR_CGROUP_CONTROLLER_CPUSET)) { + virReportError(VIR_ERR_OPERATION_INVALID, "%s", + _("cgroup cpuset controller is not mounted")); + goto endjob; + } + if (mode != -1 && - virDomainNumatuneGetMode(vm->def->numa, -1, &config_mode) == 0 && + virDomainNumatuneGetMode(def->numa, -1, &config_mode) == 0 && config_mode != mode) { virReportError(VIR_ERR_OPERATION_INVALID, "%s", _("can't change numatune mode for running domain")); @@ -10225,8 +10218,8 @@ qemuDomainSetNumaParameters(virDomainPtr dom, qemuDomainSetNumaParamsLive(vm, nodeset) < 0) goto endjob; - if (virDomainNumatuneSet(vm->def->numa, - vm->def->placement_mode == + if (virDomainNumatuneSet(def->numa, + def->placement_mode == VIR_DOMAIN_CPU_PLACEMENT_MODE_STATIC, -1, mode, nodeset) < 0) goto endjob; @@ -10235,7 +10228,7 @@ qemuDomainSetNumaParameters(virDomainPtr dom, goto endjob; } - if (flags & VIR_DOMAIN_AFFECT_CONFIG) { + if (persistentDef) { if (virDomainNumatuneSet(persistentDef->numa, persistentDef->placement_mode == VIR_DOMAIN_CPU_PLACEMENT_MODE_STATIC, @@ -10254,7 +10247,6 @@ qemuDomainSetNumaParameters(virDomainPtr dom, cleanup: virBitmapFree(nodeset); virDomainObjEndAPI(&vm); - virObjectUnref(caps); virObjectUnref(cfg); return ret; } -- 2.4.1
Replace the for loops with case inside with temp variables and a macro. --- src/qemu/qemu_driver.c | 99 +++++++++++++------------------------------------- 1 file changed, 25 insertions(+), 74 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index ca04d2c..f13e243 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -9919,6 +9919,13 @@ qemuDomainSetMemoryParameters(virDomainPtr dom, return ret; } + +#define QEMU_ASSIGN_MEM_PARAM(index, name, value) \ + if (index < *nparams && \ + virTypedParameterAssign(¶ms[index], name, VIR_TYPED_PARAM_ULLONG, \ + value) < 0) \ + goto cleanup + static int qemuDomainGetMemoryParameters(virDomainPtr dom, virTypedParameterPtr params, @@ -9926,13 +9933,13 @@ qemuDomainGetMemoryParameters(virDomainPtr dom, unsigned int flags) { virQEMUDriverPtr driver = dom->conn->privateData; - size_t i; virDomainObjPtr vm = NULL; virDomainDefPtr persistentDef = NULL; int ret = -1; virCapsPtr caps = NULL; qemuDomainObjPrivatePtr priv; virQEMUDriverConfigPtr cfg = NULL; + unsigned long long swap_hard_limit, mem_hard_limit, mem_soft_limit; virCheckFlags(VIR_DOMAIN_AFFECT_LIVE | VIR_DOMAIN_AFFECT_CONFIG | @@ -9979,85 +9986,28 @@ qemuDomainGetMemoryParameters(virDomainPtr dom, } if (flags & VIR_DOMAIN_AFFECT_CONFIG) { - for (i = 0; i < *nparams && i < QEMU_NB_MEM_PARAM; i++) { - virMemoryParameterPtr param = ¶ms[i]; - unsigned long long value; - - switch (i) { - case 0: /* fill memory hard limit here */ - value = persistentDef->mem.hard_limit; - if (virTypedParameterAssign(param, VIR_DOMAIN_MEMORY_HARD_LIMIT, - VIR_TYPED_PARAM_ULLONG, value) < 0) - goto cleanup; - break; - - case 1: /* fill memory soft limit here */ - value = persistentDef->mem.soft_limit; - if (virTypedParameterAssign(param, VIR_DOMAIN_MEMORY_SOFT_LIMIT, - VIR_TYPED_PARAM_ULLONG, value) < 0) - goto cleanup; - break; - - case 2: /* fill swap hard limit here */ - value = persistentDef->mem.swap_hard_limit; - if (virTypedParameterAssign(param, VIR_DOMAIN_MEMORY_SWAP_HARD_LIMIT, - VIR_TYPED_PARAM_ULLONG, value) < 0) - goto cleanup; - break; - - /* coverity[dead_error_begin] */ - default: - break; - /* should not hit here */ - } - } - goto out; - } - - for (i = 0; i < *nparams && i < QEMU_NB_MEM_PARAM; i++) { - virTypedParameterPtr param = ¶ms[i]; - unsigned long long val = 0; - - switch (i) { - case 0: /* fill memory hard limit here */ - if (virCgroupGetMemoryHardLimit(priv->cgroup, &val) < 0) - goto cleanup; - if (virTypedParameterAssign(param, - VIR_DOMAIN_MEMORY_HARD_LIMIT, - VIR_TYPED_PARAM_ULLONG, val) < 0) - goto cleanup; - break; + mem_hard_limit = persistentDef->mem.hard_limit; + mem_soft_limit = persistentDef->mem.soft_limit; + swap_hard_limit = persistentDef->mem.swap_hard_limit; + } else { + if (virCgroupGetMemoryHardLimit(priv->cgroup, &mem_hard_limit) < 0) + goto cleanup; - case 1: /* fill memory soft limit here */ - if (virCgroupGetMemorySoftLimit(priv->cgroup, &val) < 0) - goto cleanup; - if (virTypedParameterAssign(param, - VIR_DOMAIN_MEMORY_SOFT_LIMIT, - VIR_TYPED_PARAM_ULLONG, val) < 0) - goto cleanup; - break; + if (virCgroupGetMemorySoftLimit(priv->cgroup, &mem_soft_limit) < 0) + goto cleanup; - case 2: /* fill swap hard limit here */ - if (virCgroupGetMemSwapHardLimit(priv->cgroup, &val) < 0) { - if (!virLastErrorIsSystemErrno(ENOENT) && - !virLastErrorIsSystemErrno(EOPNOTSUPP)) - goto cleanup; - val = VIR_DOMAIN_MEMORY_PARAM_UNLIMITED; - } - if (virTypedParameterAssign(param, - VIR_DOMAIN_MEMORY_SWAP_HARD_LIMIT, - VIR_TYPED_PARAM_ULLONG, val) < 0) + if (virCgroupGetMemSwapHardLimit(priv->cgroup, &swap_hard_limit) < 0) { + if (!virLastErrorIsSystemErrno(ENOENT) && + !virLastErrorIsSystemErrno(EOPNOTSUPP)) goto cleanup; - break; - - /* coverity[dead_error_begin] */ - default: - break; - /* should not hit here */ + swap_hard_limit = VIR_DOMAIN_MEMORY_PARAM_UNLIMITED; } } - out: + QEMU_ASSIGN_MEM_PARAM(0, VIR_DOMAIN_MEMORY_HARD_LIMIT, mem_hard_limit); + QEMU_ASSIGN_MEM_PARAM(1, VIR_DOMAIN_MEMORY_SOFT_LIMIT, mem_soft_limit); + QEMU_ASSIGN_MEM_PARAM(2, VIR_DOMAIN_MEMORY_SWAP_HARD_LIMIT, swap_hard_limit); + if (QEMU_NB_MEM_PARAM < *nparams) *nparams = QEMU_NB_MEM_PARAM; ret = 0; @@ -10068,6 +10018,7 @@ qemuDomainGetMemoryParameters(virDomainPtr dom, virObjectUnref(cfg); return ret; } +#undef QEMU_ASSIGN_MEM_PARAM static int qemuDomainSetNumaParamsLive(virDomainObjPtr vm, -- 2.4.1
Simplify the code by restructuring control flow and reusing the better helper. --- src/qemu/qemu_driver.c | 27 ++++++++------------------- 1 file changed, 8 insertions(+), 19 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index f13e243..e09bb70 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -9936,7 +9936,6 @@ qemuDomainGetMemoryParameters(virDomainPtr dom, virDomainObjPtr vm = NULL; virDomainDefPtr persistentDef = NULL; int ret = -1; - virCapsPtr caps = NULL; qemuDomainObjPrivatePtr priv; virQEMUDriverConfigPtr cfg = NULL; unsigned long long swap_hard_limit, mem_hard_limit, mem_soft_limit; @@ -9945,9 +9944,6 @@ qemuDomainGetMemoryParameters(virDomainPtr dom, VIR_DOMAIN_AFFECT_CONFIG | VIR_TYPED_PARAM_STRING_OKAY, -1); - /* We don't return strings, and thus trivially support this flag. */ - flags &= ~VIR_TYPED_PARAM_STRING_OKAY; - if (!(vm = qemuDomObjFromDomain(dom))) return -1; @@ -9963,21 +9959,9 @@ qemuDomainGetMemoryParameters(virDomainPtr dom, goto cleanup; } - if (!(caps = virQEMUDriverGetCapabilities(driver, false))) + if (virDomainObjGetDefs(vm, flags, NULL, &persistentDef) < 0) goto cleanup; - if (virDomainLiveConfigHelperMethod(caps, driver->xmlopt, vm, &flags, - &persistentDef) < 0) - goto cleanup; - - if (flags & VIR_DOMAIN_AFFECT_LIVE) { - if (!virCgroupHasController(priv->cgroup, VIR_CGROUP_CONTROLLER_MEMORY)) { - virReportError(VIR_ERR_OPERATION_INVALID, - "%s", _("cgroup memory controller is not mounted")); - goto cleanup; - } - } - if ((*nparams) == 0) { /* Current number of memory parameters supported by cgroups */ *nparams = QEMU_NB_MEM_PARAM; @@ -9985,11 +9969,17 @@ qemuDomainGetMemoryParameters(virDomainPtr dom, goto cleanup; } - if (flags & VIR_DOMAIN_AFFECT_CONFIG) { + if (persistentDef) { mem_hard_limit = persistentDef->mem.hard_limit; mem_soft_limit = persistentDef->mem.soft_limit; swap_hard_limit = persistentDef->mem.swap_hard_limit; } else { + if (!virCgroupHasController(priv->cgroup, VIR_CGROUP_CONTROLLER_MEMORY)) { + virReportError(VIR_ERR_OPERATION_INVALID, + "%s", _("cgroup memory controller is not mounted")); + goto cleanup; + } + if (virCgroupGetMemoryHardLimit(priv->cgroup, &mem_hard_limit) < 0) goto cleanup; @@ -10014,7 +10004,6 @@ qemuDomainGetMemoryParameters(virDomainPtr dom, cleanup: virDomainObjEndAPI(&vm); - virObjectUnref(caps); virObjectUnref(cfg); return ret; } -- 2.4.1
On 06/15/2015 03:47 PM, Peter Krempa wrote:
Simplify the code by restructuring control flow and reusing the better helper. --- src/qemu/qemu_driver.c | 27 ++++++++------------------- 1 file changed, 8 insertions(+), 19 deletions(-)
ACK 9-11 John
The privileged flag will not change while the configuration might change. Make the 'privileged' flag member of the driver again and mark it immutable. Should that ever change add an accessor that will group reads of the state. --- src/qemu/qemu_cgroup.c | 13 ++++--------- src/qemu/qemu_command.c | 9 +++++---- src/qemu/qemu_conf.c | 7 ++++++- src/qemu/qemu_conf.h | 5 ++++- src/qemu/qemu_domain.c | 4 ++-- src/qemu/qemu_driver.c | 36 +++++++++++++----------------------- tests/qemuxml2argvtest.c | 4 ++-- 7 files changed, 36 insertions(+), 42 deletions(-) diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c index 7d1f009..8ed74ee 100644 --- a/src/qemu/qemu_cgroup.c +++ b/src/qemu/qemu_cgroup.c @@ -714,7 +714,7 @@ qemuInitCgroup(virQEMUDriverPtr driver, qemuDomainObjPrivatePtr priv = vm->privateData; virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); - if (!cfg->privileged) + if (!virQEMUDriverIsPrivileged(driver)) goto done; if (!virCgroupAvailable()) @@ -745,7 +745,7 @@ qemuInitCgroup(virQEMUDriverPtr driver, if (virCgroupNewMachine(vm->def->name, "qemu", - cfg->privileged, + true, vm->def->uuid, NULL, vm->pid, @@ -844,7 +844,7 @@ qemuConnectCgroup(virQEMUDriverPtr driver, qemuDomainObjPrivatePtr priv = vm->privateData; int ret = -1; - if (!cfg->privileged) + if (!virQEMUDriverIsPrivileged(driver)) goto done; if (!virCgroupAvailable()) @@ -1247,22 +1247,17 @@ qemuRemoveCgroup(virQEMUDriverPtr driver, virDomainObjPtr vm) { qemuDomainObjPrivatePtr priv = vm->privateData; - virQEMUDriverConfigPtr cfg; if (priv->cgroup == NULL) return 0; /* Not supported, so claim success */ - cfg = virQEMUDriverGetConfig(driver); - if (virCgroupTerminateMachine(vm->def->name, "qemu", - cfg->privileged) < 0) { + virQEMUDriverIsPrivileged(driver)) < 0) { if (!virCgroupNewIgnoreError()) VIR_DEBUG("Failed to terminate cgroup for %s", vm->def->name); } - virObjectUnref(cfg); - return virCgroupRemove(priv->cgroup); } diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 3886b4f..a51a3f6 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -354,7 +354,7 @@ qemuNetworkIfaceConnect(virDomainDefPtr def, if (net->backend.tap) { tunpath = net->backend.tap; - if (!cfg->privileged) { + if (!(virQEMUDriverIsPrivileged(driver))) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("cannot use custom tap device in session mode")); goto cleanup; @@ -381,7 +381,7 @@ qemuNetworkIfaceConnect(virDomainDefPtr def, tap_create_flags |= VIR_NETDEV_TAP_CREATE_VNET_HDR; } - if (cfg->privileged) { + if (virQEMUDriverIsPrivileged(driver)) { if (virNetDevTapCreateInBridgePort(brname, &net->ifname, &net->mac, def->uuid, tunpath, tapfd, *tapfdSize, virDomainNetGetActualVirtPortProfile(net), @@ -8284,7 +8284,8 @@ qemuBuildInterfaceCommandLine(virCommandPtr cmd, /* network and bridge use a tap device, and direct uses a * macvtap device */ - if (cfg->privileged && nicindexes && nnicindexes && net->ifname) { + if (virQEMUDriverIsPrivileged(driver) && nicindexes && nnicindexes && + net->ifname) { if (virNetDevGetIndex(net->ifname, &nicindex) < 0 || VIR_APPEND_ELEMENT(*nicindexes, *nnicindexes, nicindex) < 0) goto cleanup; @@ -8764,7 +8765,7 @@ qemuBuildCommandLine(virConnectPtr conn, emulator = def->emulator; - if (!cfg->privileged) { + if (!virQEMUDriverIsPrivileged(driver)) { /* If we have no cgroups then we can have no tunings that * require them */ diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index 16ae6ab..d521886 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -164,7 +164,6 @@ virQEMUDriverConfigPtr virQEMUDriverConfigNew(bool privileged) if (!(cfg = virObjectNew(virQEMUDriverConfigClass))) return NULL; - cfg->privileged = privileged; cfg->uri = privileged ? "qemu:///system" : "qemu:///session"; if (privileged) { @@ -873,6 +872,12 @@ virQEMUDriverConfigPtr virQEMUDriverGetConfig(virQEMUDriverPtr driver) return conf; } +bool +virQEMUDriverIsPrivileged(virQEMUDriverPtr driver) +{ + return driver->privileged; +} + virDomainXMLOptionPtr virQEMUDriverCreateXMLConf(virQEMUDriverPtr driver) { diff --git a/src/qemu/qemu_conf.h b/src/qemu/qemu_conf.h index 2ba4ce7..b74c283 100644 --- a/src/qemu/qemu_conf.h +++ b/src/qemu/qemu_conf.h @@ -81,7 +81,6 @@ typedef virQEMUDriverConfig *virQEMUDriverConfigPtr; struct _virQEMUDriverConfig { virObject parent; - bool privileged; const char *uri; uid_t user; @@ -198,6 +197,9 @@ struct _virQEMUDriver { /* Atomic inc/dec only */ unsigned int nactive; + /* Immutable value */ + bool privileged; + /* Immutable pointers. Caller must provide locking */ virStateInhibitCallback inhibitCallback; void *inhibitOpaque; @@ -273,6 +275,7 @@ int virQEMUDriverConfigLoadFile(virQEMUDriverConfigPtr cfg, const char *filename); virQEMUDriverConfigPtr virQEMUDriverGetConfig(virQEMUDriverPtr driver); +bool virQEMUDriverIsPrivileged(virQEMUDriverPtr driver); virCapsPtr virQEMUDriverCreateCapabilities(virQEMUDriverPtr driver); virCapsPtr virQEMUDriverGetCapabilities(virQEMUDriverPtr driver, diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 0682390..dcd4029 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -2045,7 +2045,7 @@ void qemuDomainObjCheckTaint(virQEMUDriverPtr driver, virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); qemuDomainObjPrivatePtr priv = obj->privateData; - if (cfg->privileged && + if (virQEMUDriverIsPrivileged(driver) && (!cfg->clearEmulatorCapabilities || cfg->user == 0 || cfg->group == 0)) @@ -2189,7 +2189,7 @@ qemuDomainCreateLog(virQEMUDriverPtr driver, virDomainObjPtr vm, oflags = O_CREAT | O_WRONLY; /* Only logrotate files in /var/log, so only append if running privileged */ - if (cfg->privileged || append) + if (virQEMUDriverIsPrivileged(driver) || append) oflags |= O_APPEND; else oflags |= O_TRUNC; diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index e09bb70..ca6f50f 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -421,7 +421,7 @@ qemuSecurityInit(virQEMUDriverPtr driver) mgr = NULL; } - if (cfg->privileged) { + if (virQEMUDriverIsPrivileged(driver)) { if (!(mgr = virSecurityManagerNewDAC(QEMU_DRIVER_NAME, cfg->user, cfg->group, @@ -652,6 +652,8 @@ qemuStateInitialize(bool privileged, /* Don't have a dom0 so start from 1 */ qemu_driver->nextvmid = 1; + qemu_driver->privileged = privileged; + if (!(qemu_driver->domains = virDomainObjListNew())) goto error; @@ -871,7 +873,7 @@ qemuStateInitialize(bool privileged, hugepagePath); goto error; } - if (cfg->privileged) { + if (privileged) { if (virFileUpdatePerm(cfg->hugetlbfs[i].mnt_dir, 0, S_IXGRP | S_IXOTH) < 0) goto error; @@ -1161,7 +1163,7 @@ static virDrvOpenStatus qemuConnectOpen(virConnectPtr conn, goto cleanup; } - if (cfg->privileged) { + if (virQEMUDriverIsPrivileged(qemu_driver)) { if (STRNEQ(conn->uri->path, "/system") && STRNEQ(conn->uri->path, "/session")) { virReportError(VIR_ERR_INTERNAL_ERROR, @@ -8927,7 +8929,6 @@ static char *qemuDomainGetSchedulerType(virDomainPtr dom, virDomainObjPtr vm = NULL; qemuDomainObjPrivatePtr priv; virQEMUDriverPtr driver = dom->conn->privateData; - virQEMUDriverConfigPtr cfg = NULL; if (!(vm = qemuDomObjFromDomain(dom))) goto cleanup; @@ -8937,8 +8938,7 @@ static char *qemuDomainGetSchedulerType(virDomainPtr dom, if (virDomainGetSchedulerTypeEnsureACL(dom->conn, vm->def) < 0) goto cleanup; - cfg = virQEMUDriverGetConfig(driver); - if (!cfg->privileged) { + if (!virQEMUDriverIsPrivileged(driver)) { virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", _("CPU tuning is not available in session mode")); goto cleanup; @@ -8969,7 +8969,6 @@ static char *qemuDomainGetSchedulerType(virDomainPtr dom, cleanup: virDomainObjEndAPI(&vm); - virObjectUnref(cfg); return ret; } @@ -9195,7 +9194,7 @@ qemuDomainSetBlkioParameters(virDomainPtr dom, if (virDomainSetBlkioParametersEnsureACL(dom->conn, vm->def, flags) < 0) goto cleanup; - if (!cfg->privileged) { + if (!virQEMUDriverIsPrivileged(driver)) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("Block I/O tuning is not available in session mode")); goto cleanup; @@ -9367,7 +9366,6 @@ qemuDomainGetBlkioParameters(virDomainPtr dom, int ret = -1; virCapsPtr caps = NULL; qemuDomainObjPrivatePtr priv; - virQEMUDriverConfigPtr cfg = NULL; virCheckFlags(VIR_DOMAIN_AFFECT_LIVE | VIR_DOMAIN_AFFECT_CONFIG | @@ -9386,8 +9384,7 @@ qemuDomainGetBlkioParameters(virDomainPtr dom, if (virDomainGetBlkioParametersEnsureACL(dom->conn, vm->def) < 0) goto cleanup; - cfg = virQEMUDriverGetConfig(driver); - if (!cfg->privileged) { + if (!virQEMUDriverIsPrivileged(driver)) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("Block I/O tuning is not available in session mode")); goto cleanup; @@ -9762,7 +9759,6 @@ qemuDomainGetBlkioParameters(virDomainPtr dom, cleanup: virDomainObjEndAPI(&vm); virObjectUnref(caps); - virObjectUnref(cfg); return ret; } @@ -9810,7 +9806,7 @@ qemuDomainSetMemoryParameters(virDomainPtr dom, if (virDomainSetMemoryParametersEnsureACL(dom->conn, vm->def, flags) < 0) goto cleanup; - if (!cfg->privileged) { + if (!virQEMUDriverIsPrivileged(driver)) { virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", _("Memory tuning is not available in session mode")); goto cleanup; @@ -9937,7 +9933,6 @@ qemuDomainGetMemoryParameters(virDomainPtr dom, virDomainDefPtr persistentDef = NULL; int ret = -1; qemuDomainObjPrivatePtr priv; - virQEMUDriverConfigPtr cfg = NULL; unsigned long long swap_hard_limit, mem_hard_limit, mem_soft_limit; virCheckFlags(VIR_DOMAIN_AFFECT_LIVE | @@ -9952,8 +9947,7 @@ qemuDomainGetMemoryParameters(virDomainPtr dom, if (virDomainGetMemoryParametersEnsureACL(dom->conn, vm->def) < 0) goto cleanup; - cfg = virQEMUDriverGetConfig(driver); - if (!cfg->privileged) { + if (!virQEMUDriverIsPrivileged(driver)) { virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", _("Memory tuning is not available in session mode")); goto cleanup; @@ -10004,7 +9998,6 @@ qemuDomainGetMemoryParameters(virDomainPtr dom, cleanup: virDomainObjEndAPI(&vm); - virObjectUnref(cfg); return ret; } #undef QEMU_ASSIGN_MEM_PARAM @@ -10134,7 +10127,7 @@ qemuDomainSetNumaParameters(virDomainPtr dom, goto endjob; if (def) { - if (!cfg->privileged) { + if (!virQEMUDriverIsPrivileged(driver)) { virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", _("NUMA tuning is not available in session mode")); goto endjob; @@ -10382,7 +10375,7 @@ qemuDomainSetSchedulerParametersFlags(virDomainPtr dom, if (virDomainSetSchedulerParametersFlagsEnsureACL(dom->conn, vm->def, flags) < 0) goto cleanup; - if (!cfg->privileged) { + if (!virQEMUDriverIsPrivileged(driver)) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("CPU tuning is not available in session mode")); goto cleanup; @@ -10676,7 +10669,6 @@ qemuDomainGetSchedulerParametersFlags(virDomainPtr dom, virDomainDefPtr persistentDef; virCapsPtr caps = NULL; qemuDomainObjPrivatePtr priv; - virQEMUDriverConfigPtr cfg = NULL; virCheckFlags(VIR_DOMAIN_AFFECT_LIVE | VIR_DOMAIN_AFFECT_CONFIG | @@ -10693,8 +10685,7 @@ qemuDomainGetSchedulerParametersFlags(virDomainPtr dom, if (virDomainGetSchedulerParametersFlagsEnsureACL(dom->conn, vm->def) < 0) goto cleanup; - cfg = virQEMUDriverGetConfig(driver); - if (!cfg->privileged) { + if (!virQEMUDriverIsPrivileged(driver)) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("CPU tuning is not available in session mode")); goto cleanup; @@ -10793,7 +10784,6 @@ qemuDomainGetSchedulerParametersFlags(virDomainPtr dom, cleanup: virDomainObjEndAPI(&vm); virObjectUnref(caps); - virObjectUnref(cfg); return ret; } diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index a90f9a6..becd5e7 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -486,8 +486,8 @@ mymain(void) driver.config = virQEMUDriverConfigNew(false); if (driver.config == NULL) return EXIT_FAILURE; - else - driver.config->privileged = true; + + driver.privileged = true; VIR_FREE(driver.config->spiceListen); VIR_FREE(driver.config->vncListen); -- 2.4.1
--- src/conf/domain_conf.c | 132 ++++++++++++++++++++++++++++--------------------- 1 file changed, 76 insertions(+), 56 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 35e1cb4..454d6cb 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -14082,6 +14082,81 @@ virDomainThreadSchedParse(xmlNodePtr node, return -1; } + +static int +virDomainVcpuParse(virDomainDefPtr def, + xmlXPathContextPtr ctxt) +{ + int n; + char *tmp = NULL; + int ret = -1; + + if ((n = virXPathUInt("string(./vcpu[1])", ctxt, &def->maxvcpus)) < 0) { + if (n == -2) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("maximum vcpus count must be an integer")); + goto cleanup; + } + + def->maxvcpus = 1; + } + + if ((n = virXPathUInt("string(./vcpu[1]/@current)", ctxt, &def->vcpus)) < 0) { + if (n == -2) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("current vcpus count must be an integer")); + goto cleanup; + } + + def->vcpus = def->maxvcpus; + } + + if (def->maxvcpus < def->vcpus) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("maxvcpus must not be less than current vcpus " + "(%u < %u)"), def->maxvcpus, def->vcpus); + goto cleanup; + } + + tmp = virXPathString("string(./vcpu[1]/@placement)", ctxt); + if (tmp) { + if ((def->placement_mode = + virDomainCpuPlacementModeTypeFromString(tmp)) < 0) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("Unsupported CPU placement mode '%s'"), + tmp); + goto cleanup; + } + VIR_FREE(tmp); + } else { + def->placement_mode = VIR_DOMAIN_CPU_PLACEMENT_MODE_STATIC; + } + + if (def->placement_mode != VIR_DOMAIN_CPU_PLACEMENT_MODE_AUTO) { + tmp = virXPathString("string(./vcpu[1]/@cpuset)", ctxt); + if (tmp) { + if (virBitmapParse(tmp, 0, &def->cpumask, + VIR_DOMAIN_CPUMASK_LEN) < 0) + goto cleanup; + + if (virBitmapIsAllClear(def->cpumask)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("Invalid value of 'cpuset': %s"), tmp); + goto cleanup; + } + + VIR_FREE(tmp); + } + } + + ret = 0; + + cleanup: + VIR_FREE(tmp); + + return ret; +} + static virDomainDefPtr virDomainDefParseXML(xmlDocPtr xml, xmlNodePtr root, @@ -14378,63 +14453,8 @@ virDomainDefParseXML(xmlDocPtr xml, &def->mem.swap_hard_limit) < 0) goto error; - if ((n = virXPathUInt("string(./vcpu[1])", ctxt, &def->maxvcpus)) < 0) { - if (n == -2) { - virReportError(VIR_ERR_XML_ERROR, "%s", - _("maximum vcpus count must be an integer")); - goto error; - } - - def->maxvcpus = 1; - } - - if ((n = virXPathUInt("string(./vcpu[1]/@current)", ctxt, &def->vcpus)) < 0) { - if (n == -2) { - virReportError(VIR_ERR_XML_ERROR, "%s", - _("current vcpus count must be an integer")); - goto error; - } - - def->vcpus = def->maxvcpus; - } - - if (def->maxvcpus < def->vcpus) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("maxvcpus must not be less than current vcpus " - "(%u < %u)"), def->maxvcpus, def->vcpus); + if (virDomainVcpuParse(def, ctxt) < 0) goto error; - } - - tmp = virXPathString("string(./vcpu[1]/@placement)", ctxt); - if (tmp) { - if ((def->placement_mode = - virDomainCpuPlacementModeTypeFromString(tmp)) < 0) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("Unsupported CPU placement mode '%s'"), - tmp); - goto error; - } - VIR_FREE(tmp); - } else { - def->placement_mode = VIR_DOMAIN_CPU_PLACEMENT_MODE_STATIC; - } - - if (def->placement_mode != VIR_DOMAIN_CPU_PLACEMENT_MODE_AUTO) { - tmp = virXPathString("string(./vcpu[1]/@cpuset)", ctxt); - if (tmp) { - if (virBitmapParse(tmp, 0, &def->cpumask, - VIR_DOMAIN_CPUMASK_LEN) < 0) - goto error; - - if (virBitmapIsAllClear(def->cpumask)) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("Invalid value of 'cpuset': %s"), tmp); - goto error; - } - - VIR_FREE(tmp); - } - } /* Optional - iothreads */ tmp = virXPathString("string(./iothreads[1])", ctxt); -- 2.4.1
On 06/15/2015 03:47 PM, Peter Krempa wrote:
--- src/conf/domain_conf.c | 132 ++++++++++++++++++++++++++++--------------------- 1 file changed, 76 insertions(+), 56 deletions(-)
ACK 12-13 John
participants (2)
-
John Ferlan -
Peter Krempa