[libvirt] [PATCH v2] Provide a helper method virDomainLiveConfigHelperMethod
Changes since v1 - With Eric's comments squashed in. This chunk of code below repeated in several functions, factor it into a helper method virDomainLiveConfigHelperMethod to eliminate duplicated code based on Eric and Adam's suggestion. I have tested it for all the relevant APIs changed. Signed-off-by: Eric Blake <eblake@redhat.com> Signed-off-by: Lei Li <lilei@linux.vnet.ibm.com> --- src/conf/domain_conf.c | 51 ++++++++ src/conf/domain_conf.h | 7 + src/libvirt_private.syms | 1 + src/qemu/qemu_driver.c | 288 ++++------------------------------------------ 4 files changed, 81 insertions(+), 266 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index d68ab10..da98a22 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -1670,6 +1670,57 @@ virDomainObjGetPersistentDef(virCapsPtr caps, } /* + * Helper method for --current --live --config option, and check with + * whether domain is active or can get persistent domain configuration. + * + * Return 0 if success, also change the flags and get the persistent + * domain configuration if needed. Return -1 on error. + */ +int +virDomainLiveConfigHelperMethod(virCapsPtr caps, + virDomainObjPtr dom, + unsigned int *flags, + virDomainDefPtr *persistentDef) +{ + bool isActive; + int ret = -1; + + isActive = virDomainObjIsActive(dom); + + if ((*flags & (VIR_DOMAIN_AFFECT_LIVE | VIR_DOMAIN_AFFECT_CONFIG)) == + VIR_DOMAIN_AFFECT_CURRENT) { + if (isActive) + *flags |= VIR_DOMAIN_AFFECT_LIVE; + else + *flags |= VIR_DOMAIN_AFFECT_CONFIG; + } + + if (!isActive && (*flags & VIR_DOMAIN_AFFECT_LIVE)) { + virDomainReportError(VIR_ERR_OPERATION_INVALID, "%s", + _("domain is not running")); + goto cleanup; + } + + if (*flags & VIR_DOMAIN_AFFECT_CONFIG) { + if (!dom->persistent) { + virDomainReportError(VIR_ERR_OPERATION_INVALID, "%s", + _("cannot change persistent config of a transient domain")); + goto cleanup; + } + if (!(*persistentDef = virDomainObjGetPersistentDef(caps, dom))) { + virDomainReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Get persistent config failed")); + goto cleanup; + } + } + + ret = 0; + +cleanup: + return ret; +} + +/* * The caller must hold a lock on the driver owning 'doms', * and must also have locked 'dom', to ensure no one else * is either waiting for 'dom' or still using it diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index d6ed898..3229a6f 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -1737,6 +1737,13 @@ int virDomainObjSetDefTransient(virCapsPtr caps, virDomainDefPtr virDomainObjGetPersistentDef(virCapsPtr caps, virDomainObjPtr domain); + +int +virDomainLiveConfigHelperMethod(virCapsPtr caps, + virDomainObjPtr dom, + unsigned int *flags, + virDomainDefPtr *persistentDef); + virDomainDefPtr virDomainObjCopyPersistentDef(virCapsPtr caps, virDomainObjPtr dom); diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index a81c230..48ffdf2 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -358,6 +358,7 @@ virDomainLifecycleCrashTypeFromString; virDomainLifecycleCrashTypeToString; virDomainLifecycleTypeFromString; virDomainLifecycleTypeToString; +virDomainLiveConfigHelperMethod; virDomainLoadAllConfigs; virDomainMemballoonModelTypeFromString; virDomainMemballoonModelTypeToString; diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 10a289e..aa92573 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -1822,12 +1822,6 @@ static int qemudDomainSetMemoryFlags(virDomainPtr dom, unsigned long newmem, isActive = virDomainObjIsActive(vm); - if (flags == VIR_DOMAIN_AFFECT_CURRENT) { - if (isActive) - flags = VIR_DOMAIN_AFFECT_LIVE; - else - flags = VIR_DOMAIN_AFFECT_CONFIG; - } if (flags == VIR_DOMAIN_MEM_MAXIMUM) { if (isActive) flags = VIR_DOMAIN_AFFECT_LIVE | VIR_DOMAIN_MEM_MAXIMUM; @@ -1835,21 +1829,8 @@ static int qemudDomainSetMemoryFlags(virDomainPtr dom, unsigned long newmem, flags = VIR_DOMAIN_AFFECT_CONFIG | VIR_DOMAIN_MEM_MAXIMUM; } - if (!isActive && (flags & VIR_DOMAIN_AFFECT_LIVE)) { - qemuReportError(VIR_ERR_OPERATION_INVALID, - "%s", _("domain is not running")); + if (virDomainLiveConfigHelperMethod(driver->caps, vm, &flags, &persistentDef) < 0) goto endjob; - } - - if (flags & VIR_DOMAIN_AFFECT_CONFIG) { - if (!vm->persistent) { - qemuReportError(VIR_ERR_OPERATION_INVALID, "%s", - _("cannot change persistent config of a transient domain")); - goto endjob; - } - if (!(persistentDef = virDomainObjGetPersistentDef(driver->caps, vm))) - goto endjob; - } if (flags & VIR_DOMAIN_MEM_MAXIMUM) { /* resize the maximum memory */ @@ -3271,7 +3252,6 @@ qemuDomainSetVcpusFlags(virDomainPtr dom, unsigned int nvcpus, const char * type; int max; int ret = -1; - bool isActive; bool maximum; virCheckFlags(VIR_DOMAIN_AFFECT_LIVE | @@ -3299,16 +3279,11 @@ qemuDomainSetVcpusFlags(virDomainPtr dom, unsigned int nvcpus, if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0) goto cleanup; - isActive = virDomainObjIsActive(vm); maximum = (flags & VIR_DOMAIN_VCPU_MAXIMUM) != 0; flags &= ~VIR_DOMAIN_VCPU_MAXIMUM; - if (flags == VIR_DOMAIN_AFFECT_CURRENT) { - if (isActive) - flags |= VIR_DOMAIN_AFFECT_LIVE; - else - flags |= VIR_DOMAIN_AFFECT_CONFIG; - } + if (virDomainLiveConfigHelperMethod(driver->caps, vm, &flags, &persistentDef) < 0) + goto endjob; /* MAXIMUM cannot be mixed with LIVE. */ if (maximum && (flags & VIR_DOMAIN_AFFECT_LIVE)) { @@ -3317,18 +3292,6 @@ qemuDomainSetVcpusFlags(virDomainPtr dom, unsigned int nvcpus, goto endjob; } - if (!isActive && (flags & VIR_DOMAIN_AFFECT_LIVE)) { - qemuReportError(VIR_ERR_OPERATION_INVALID, - "%s", _("domain is not running")); - goto endjob; - } - - if (!vm->persistent && (flags & VIR_DOMAIN_AFFECT_CONFIG)) { - qemuReportError(VIR_ERR_OPERATION_INVALID, "%s", - _("cannot change persistent config of a transient domain")); - goto endjob; - } - if (!(type = virDomainVirtTypeToString(vm->def->virtType))) { qemuReportError(VIR_ERR_INTERNAL_ERROR, _("unknown virt type in domain definition '%d'"), @@ -3353,9 +3316,6 @@ qemuDomainSetVcpusFlags(virDomainPtr dom, unsigned int nvcpus, goto endjob; } - if (!(persistentDef = virDomainObjGetPersistentDef(driver->caps, vm))) - goto endjob; - switch (flags) { case VIR_DOMAIN_AFFECT_CONFIG: if (maximum) { @@ -3414,7 +3374,6 @@ qemudDomainPinVcpuFlags(virDomainPtr dom, int maxcpu, hostcpus; virNodeInfo nodeinfo; int ret = -1; - bool isActive; qemuDomainObjPrivatePtr priv; bool canResetting = true; int pcpu; @@ -3434,20 +3393,8 @@ qemudDomainPinVcpuFlags(virDomainPtr dom, goto cleanup; } - isActive = virDomainObjIsActive(vm); - if (flags == VIR_DOMAIN_AFFECT_CURRENT) { - if (isActive) - flags = VIR_DOMAIN_AFFECT_LIVE; - else - flags = VIR_DOMAIN_AFFECT_CONFIG; - } - - if (!isActive && (flags & VIR_DOMAIN_AFFECT_LIVE)) { - qemuReportError(VIR_ERR_OPERATION_INVALID, "%s", - _("a domain is inactive; can change only " - "persistent config")); + if (virDomainLiveConfigHelperMethod(driver->caps, vm, &flags, &persistentDef) < 0) goto cleanup; - } priv = vm->privateData; @@ -3458,16 +3405,6 @@ qemudDomainPinVcpuFlags(virDomainPtr dom, goto cleanup; } - if (flags & VIR_DOMAIN_AFFECT_CONFIG) { - if (!vm->persistent) { - qemuReportError(VIR_ERR_OPERATION_INVALID, "%s", - _("cannot change persistent config of a transient domain")); - goto cleanup; - } - if (!(persistentDef = virDomainObjGetPersistentDef(driver->caps, vm))) - goto cleanup; - } - if (nodeGetInfo(dom->conn, &nodeinfo) < 0) goto cleanup; hostcpus = VIR_NODEINFO_MAXCPUS(nodeinfo); @@ -3567,7 +3504,6 @@ qemudDomainGetVcpuPinInfo(virDomainPtr dom, virNodeInfo nodeinfo; virDomainDefPtr targetDef = NULL; int ret = -1; - bool isActive; int maxcpu, hostcpus, vcpu, pcpu; int n; virDomainVcpuPinDefPtr *vcpupin_list; @@ -3589,33 +3525,13 @@ qemudDomainGetVcpuPinInfo(virDomainPtr dom, goto cleanup; } - isActive = virDomainObjIsActive(vm); - if (flags == VIR_DOMAIN_AFFECT_CURRENT) { - if (isActive) - flags = VIR_DOMAIN_AFFECT_LIVE; - else - flags = VIR_DOMAIN_AFFECT_CONFIG; - } + if (virDomainLiveConfigHelperMethod(driver->caps, vm, &flags, &targetDef) < 0) + goto cleanup; if (flags & VIR_DOMAIN_AFFECT_LIVE) { - if (!isActive) { - qemuReportError(VIR_ERR_OPERATION_INVALID, - "%s", _("domain is not running")); - goto cleanup; - } targetDef = vm->def; } - if (flags & VIR_DOMAIN_AFFECT_CONFIG) { - if (!vm->persistent) { - qemuReportError(VIR_ERR_OPERATION_INVALID, "%s", - _("cannot get persistent config of a transient domain")); - goto cleanup; - } - if (!(targetDef = virDomainObjGetPersistentDef(driver->caps, vm))) - goto cleanup; - } - /* Coverity didn't realize that targetDef must be set if we got here. */ sa_assert(targetDef); @@ -3760,7 +3676,6 @@ qemudDomainGetVcpusFlags(virDomainPtr dom, unsigned int flags) virDomainObjPtr vm; virDomainDefPtr def; int ret = -1; - bool active; virCheckFlags(VIR_DOMAIN_AFFECT_LIVE | VIR_DOMAIN_AFFECT_CONFIG | @@ -3778,34 +3693,11 @@ qemudDomainGetVcpusFlags(virDomainPtr dom, unsigned int flags) goto cleanup; } - active = virDomainObjIsActive(vm); - - if ((flags & (VIR_DOMAIN_VCPU_LIVE | VIR_DOMAIN_VCPU_CONFIG)) == 0) { - if (active) - flags |= VIR_DOMAIN_VCPU_LIVE; - else - flags |= VIR_DOMAIN_VCPU_CONFIG; - } - if ((flags & VIR_DOMAIN_AFFECT_LIVE) && (flags & VIR_DOMAIN_AFFECT_CONFIG)) { - qemuReportError(VIR_ERR_INVALID_ARG, - _("invalid flag combination: (0x%x)"), flags); - return -1; - } + if (virDomainLiveConfigHelperMethod(driver->caps, vm, &flags, &def) < 0) + goto cleanup; if (flags & VIR_DOMAIN_AFFECT_LIVE) { - if (!active) { - qemuReportError(VIR_ERR_OPERATION_INVALID, "%s", - _("domain not active")); - goto cleanup; - } def = vm->def; - } else { - if (!vm->persistent) { - qemuReportError(VIR_ERR_OPERATION_INVALID, "%s", - _("domain is transient")); - goto cleanup; - } - def = vm->newDef ? vm->newDef : vm->def; } ret = (flags & VIR_DOMAIN_VCPU_MAXIMUM) ? def->maxvcpus : def->vcpus; @@ -6014,7 +5906,6 @@ static int qemuDomainSetBlkioParameters(virDomainPtr dom, virDomainObjPtr vm = NULL; virDomainDefPtr persistentDef = NULL; int ret = -1; - bool isActive; virCheckFlags(VIR_DOMAIN_AFFECT_LIVE | VIR_DOMAIN_AFFECT_CONFIG, -1); @@ -6028,22 +5919,10 @@ static int qemuDomainSetBlkioParameters(virDomainPtr dom, goto cleanup; } - isActive = virDomainObjIsActive(vm); - - if (flags == VIR_DOMAIN_AFFECT_CURRENT) { - if (isActive) - flags = VIR_DOMAIN_AFFECT_LIVE; - else - flags = VIR_DOMAIN_AFFECT_CONFIG; - } + if (virDomainLiveConfigHelperMethod(driver->caps, vm, &flags, &persistentDef) < 0) + goto cleanup; if (flags & VIR_DOMAIN_AFFECT_LIVE) { - if (!isActive) { - qemuReportError(VIR_ERR_OPERATION_INVALID, - "%s", _("domain is not running")); - goto cleanup; - } - if (!qemuCgroupControllerActive(driver, VIR_CGROUP_CONTROLLER_BLKIO)) { qemuReportError(VIR_ERR_OPERATION_INVALID, _("blkio cgroup isn't mounted")); goto cleanup; @@ -6056,16 +5935,6 @@ static int qemuDomainSetBlkioParameters(virDomainPtr dom, } } - if (flags & VIR_DOMAIN_AFFECT_CONFIG) { - if (!vm->persistent) { - qemuReportError(VIR_ERR_OPERATION_INVALID, "%s", - _("cannot change persistent config of a transient domain")); - goto cleanup; - } - if (!(persistentDef = virDomainObjGetPersistentDef(driver->caps, vm))) - goto cleanup; - } - ret = 0; if (flags & VIR_DOMAIN_AFFECT_LIVE) { for (i = 0; i < nparams; i++) { @@ -6220,7 +6089,6 @@ static int qemuDomainGetBlkioParameters(virDomainPtr dom, unsigned int val; int ret = -1; int rc; - bool isActive; virCheckFlags(VIR_DOMAIN_AFFECT_LIVE | VIR_DOMAIN_AFFECT_CONFIG | @@ -6247,22 +6115,10 @@ static int qemuDomainGetBlkioParameters(virDomainPtr dom, goto cleanup; } - isActive = virDomainObjIsActive(vm); - - if (flags == VIR_DOMAIN_AFFECT_CURRENT) { - if (isActive) - flags = VIR_DOMAIN_AFFECT_LIVE; - else - flags = VIR_DOMAIN_AFFECT_CONFIG; - } + if (virDomainLiveConfigHelperMethod(driver->caps, vm, &flags, &persistentDef) < 0) + goto cleanup; if (flags & VIR_DOMAIN_AFFECT_LIVE) { - if (!isActive) { - qemuReportError(VIR_ERR_OPERATION_INVALID, - "%s", _("domain is not running")); - goto cleanup; - } - if (!qemuCgroupControllerActive(driver, VIR_CGROUP_CONTROLLER_BLKIO)) { qemuReportError(VIR_ERR_OPERATION_INVALID, _("blkio cgroup isn't mounted")); goto cleanup; @@ -6275,16 +6131,6 @@ static int qemuDomainGetBlkioParameters(virDomainPtr dom, } } - if (flags & VIR_DOMAIN_AFFECT_CONFIG) { - if (!vm->persistent) { - qemuReportError(VIR_ERR_OPERATION_INVALID, "%s", - _("cannot change persistent config of a transient domain")); - goto cleanup; - } - if (!(persistentDef = virDomainObjGetPersistentDef(driver->caps, vm))) - goto cleanup; - } - if (flags & VIR_DOMAIN_AFFECT_LIVE) { for (i = 0; i < *nparams && i < QEMU_NB_BLKIO_PARAM; i++) { virTypedParameterPtr param = ¶ms[i]; @@ -6440,7 +6286,6 @@ static int qemuDomainSetMemoryParameters(virDomainPtr dom, virCgroupPtr group = NULL; virDomainObjPtr vm = NULL; int ret = -1; - bool isActive; virCheckFlags(VIR_DOMAIN_AFFECT_LIVE | VIR_DOMAIN_AFFECT_CONFIG, -1); @@ -6455,22 +6300,10 @@ static int qemuDomainSetMemoryParameters(virDomainPtr dom, goto cleanup; } - isActive = virDomainObjIsActive(vm); - - if (flags == VIR_DOMAIN_AFFECT_CURRENT) { - if (isActive) - flags = VIR_DOMAIN_AFFECT_LIVE; - else - flags = VIR_DOMAIN_AFFECT_CONFIG; - } + if (virDomainLiveConfigHelperMethod(driver->caps, vm, &flags, &persistentDef) < 0) + goto cleanup; if (flags & VIR_DOMAIN_AFFECT_LIVE) { - if (!isActive) { - qemuReportError(VIR_ERR_OPERATION_INVALID, - "%s", _("domain is not running")); - goto cleanup; - } - if (!qemuCgroupControllerActive(driver, VIR_CGROUP_CONTROLLER_MEMORY)) { qemuReportError(VIR_ERR_OPERATION_INVALID, "%s", _("cgroup memory controller is not mounted")); @@ -6484,16 +6317,6 @@ static int qemuDomainSetMemoryParameters(virDomainPtr dom, } } - if (flags & VIR_DOMAIN_AFFECT_CONFIG) { - if (!vm->persistent) { - qemuReportError(VIR_ERR_OPERATION_INVALID, "%s", - _("cannot change persistent config of a transient domain")); - goto cleanup; - } - if (!(persistentDef = virDomainObjGetPersistentDef(driver->caps, vm))) - goto cleanup; - } - ret = 0; for (i = 0; i < nparams; i++) { virTypedParameterPtr param = ¶ms[i]; @@ -6598,7 +6421,6 @@ static int qemuDomainGetMemoryParameters(virDomainPtr dom, unsigned long long val; int ret = -1; int rc; - bool isActive; virCheckFlags(VIR_DOMAIN_AFFECT_LIVE | VIR_DOMAIN_AFFECT_CONFIG | @@ -6617,22 +6439,10 @@ static int qemuDomainGetMemoryParameters(virDomainPtr dom, goto cleanup; } - isActive = virDomainObjIsActive(vm); - - if (flags == VIR_DOMAIN_AFFECT_CURRENT) { - if (isActive) - flags = VIR_DOMAIN_AFFECT_LIVE; - else - flags = VIR_DOMAIN_AFFECT_CONFIG; - } + if (virDomainLiveConfigHelperMethod(driver->caps, vm, &flags, &persistentDef) < 0) + goto cleanup; if (flags & VIR_DOMAIN_AFFECT_LIVE) { - if (!isActive) { - qemuReportError(VIR_ERR_OPERATION_INVALID, - "%s", _("domain is not running")); - goto cleanup; - } - if (!qemuCgroupControllerActive(driver, VIR_CGROUP_CONTROLLER_MEMORY)) { qemuReportError(VIR_ERR_OPERATION_INVALID, "%s", _("cgroup memory controller is not mounted")); @@ -6646,16 +6456,6 @@ static int qemuDomainGetMemoryParameters(virDomainPtr dom, } } - if (flags & VIR_DOMAIN_AFFECT_CONFIG) { - if (!vm->persistent) { - qemuReportError(VIR_ERR_OPERATION_INVALID, "%s", - _("cannot change persistent config of a transient domain")); - goto cleanup; - } - if (!(persistentDef = virDomainObjGetPersistentDef(driver->caps, vm))) - goto cleanup; - } - if ((*nparams) == 0) { /* Current number of memory parameters supported by cgroups */ *nparams = QEMU_NB_MEM_PARAM; @@ -11126,7 +10926,6 @@ qemuDomainSetBlockIoTune(virDomainPtr dom, const char *device = NULL; int ret = -1; int i; - bool isActive; int idx = -1; virCheckFlags(VIR_DOMAIN_AFFECT_LIVE | @@ -11151,33 +10950,8 @@ qemuDomainSetBlockIoTune(virDomainPtr dom, if (qemuDomainObjBeginJobWithDriver(driver, vm, QEMU_JOB_MODIFY) < 0) goto cleanup; - isActive = virDomainObjIsActive(vm); - - if (flags == VIR_DOMAIN_AFFECT_CURRENT) { - if (isActive) - flags = VIR_DOMAIN_AFFECT_LIVE; - else - flags = VIR_DOMAIN_AFFECT_CONFIG; - } - - if (!isActive && (flags & VIR_DOMAIN_AFFECT_LIVE)) { - qemuReportError(VIR_ERR_OPERATION_INVALID, "%s", - _("domain is not running")); + if (virDomainLiveConfigHelperMethod(driver->caps, vm, &flags, &persistentDef) < 0) goto endjob; - } - - if (flags & VIR_DOMAIN_AFFECT_CONFIG) { - if (!vm->persistent) { - qemuReportError(VIR_ERR_OPERATION_INVALID, "%s", - _("cannot change persistent config of a transient domain")); - goto endjob; - } - if (!(persistentDef = virDomainObjGetPersistentDef(driver->caps, vm))) - goto endjob; - idx = virDomainDiskIndexByName(persistentDef, disk, true); - if (idx < 0) - goto endjob; - } for (i = 0; i < nparams; i++) { virTypedParameterPtr param = ¶ms[i]; @@ -11238,7 +11012,10 @@ qemuDomainSetBlockIoTune(virDomainPtr dom, } if (flags & VIR_DOMAIN_AFFECT_CONFIG) { - sa_assert(persistentDef && idx >= 0); + sa_assert(persistentDef); + idx = virDomainDiskIndexByName(persistentDef, disk, true); + if (idx < 0) + goto endjob; persistentDef->disks[idx]->blkdeviotune = info; ret = virDomainSaveConfig(driver->configDir, persistentDef); if (ret < 0) { @@ -11276,7 +11053,6 @@ qemuDomainGetBlockIoTune(virDomainPtr dom, const char *device = NULL; int ret = -1; int i; - bool isActive; virCheckFlags(VIR_DOMAIN_AFFECT_LIVE | VIR_DOMAIN_AFFECT_CONFIG | @@ -11310,20 +11086,8 @@ qemuDomainGetBlockIoTune(virDomainPtr dom, if (qemuDomainObjBeginJobWithDriver(driver, vm, QEMU_JOB_MODIFY) < 0) goto cleanup; - isActive = virDomainObjIsActive(vm); - - if (flags == VIR_DOMAIN_AFFECT_CURRENT) { - if (isActive) - flags = VIR_DOMAIN_AFFECT_LIVE; - else - flags = VIR_DOMAIN_AFFECT_CONFIG; - } - - if (!isActive && (flags & VIR_DOMAIN_AFFECT_LIVE)) { - qemuReportError(VIR_ERR_OPERATION_INVALID, "%s", - _("domain is not running")); + if (virDomainLiveConfigHelperMethod(driver->caps, vm, &flags, &persistentDef) < 0) goto endjob; - } if (flags & VIR_DOMAIN_AFFECT_LIVE) { priv = vm->privateData; @@ -11335,14 +11099,6 @@ qemuDomainGetBlockIoTune(virDomainPtr dom, } if (flags & VIR_DOMAIN_AFFECT_CONFIG) { - if (!vm->persistent) { - qemuReportError(VIR_ERR_OPERATION_INVALID, "%s", - _("domain is transient")); - goto endjob; - } - if (!(persistentDef = virDomainObjGetPersistentDef(driver->caps, vm))) - goto endjob; - int idx = virDomainDiskIndexByName(vm->def, disk, true); if (idx < 0) goto endjob; -- 1.7.1
On 12/11/2011 10:16 PM, Lei Li wrote:
Changes since v1 - With Eric's comments squashed in.
I tend to put revision comments like this after the ---, so as to be helpful in the review but not part of the final commit (once it's part of the tree, we don't care how many prior reversions were omitted from the tree to get us to a working commit). Supposedly, 'git notes' has a way of making this easier to do, although I still haven't been able to tune that to work nicely with my personal git usage.
This chunk of code below repeated in several functions, factor it into a helper method virDomainLiveConfigHelperMethod to eliminate duplicated code based on Eric and Adam's suggestion. I have tested it for all the relevant APIs changed.
Signed-off-by: Eric Blake <eblake@redhat.com>
Technically, I hadn't signed off quite yet. On the kernel list, this should have been a CC: line. But you're lucky that 1) I'm reviewing it, and will sign off in the end, and 2) libvirt is not half as strict as the kernel folks about sign-off notations :)
Signed-off-by: Lei Li <lilei@linux.vnet.ibm.com> --- src/conf/domain_conf.c | 51 ++++++++ src/conf/domain_conf.h | 7 + src/libvirt_private.syms | 1 + src/qemu/qemu_driver.c | 288 ++++------------------------------------------ 4 files changed, 81 insertions(+), 266 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index d68ab10..da98a22 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -1670,6 +1670,57 @@ virDomainObjGetPersistentDef(virCapsPtr caps, }
/* + * Helper method for --current --live --config option, and check with
Grammar: s/option/options/ s/with//
+ * whether domain is active or can get persistent domain configuration. + * + * Return 0 if success, also change the flags and get the persistent + * domain configuration if needed. Return -1 on error. + */ +int +virDomainLiveConfigHelperMethod(virCapsPtr caps, + virDomainObjPtr dom, + unsigned int *flags, + virDomainDefPtr *persistentDef) +{ + if (!(*persistentDef = virDomainObjGetPersistentDef(caps, dom))) { + virDomainReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Get persistent config failed"));
Indentation.
+++ b/src/qemu/qemu_driver.c @@ -1822,12 +1822,6 @@ static int qemudDomainSetMemoryFlags(virDomainPtr dom, unsigned long newmem,
isActive = virDomainObjIsActive(vm);
- if (flags == VIR_DOMAIN_AFFECT_CURRENT) { - if (isActive) - flags = VIR_DOMAIN_AFFECT_LIVE; - else - flags = VIR_DOMAIN_AFFECT_CONFIG; - } if (flags == VIR_DOMAIN_MEM_MAXIMUM) { if (isActive) flags = VIR_DOMAIN_AFFECT_LIVE | VIR_DOMAIN_MEM_MAXIMUM;
We can simplify this even further, now that virDomainLiveConfigHelperMethod leaves the rest of the bits in flags alone.
@@ -1835,21 +1829,8 @@ static int qemudDomainSetMemoryFlags(virDomainPtr dom, unsigned long newmem, flags = VIR_DOMAIN_AFFECT_CONFIG | VIR_DOMAIN_MEM_MAXIMUM; }
- if (!isActive && (flags & VIR_DOMAIN_AFFECT_LIVE)) { - qemuReportError(VIR_ERR_OPERATION_INVALID, - "%s", _("domain is not running")); + if (virDomainLiveConfigHelperMethod(driver->caps, vm, &flags, &persistentDef) < 0)
I wrapped to fit in 80 columns. I also found some spots in test_driver.c, for a further size reduction. ACK and pushed with this squashed in: diff --git i/src/conf/domain_conf.c w/src/conf/domain_conf.c index da98a22..4be8fe0 100644 --- i/src/conf/domain_conf.c +++ w/src/conf/domain_conf.c @@ -1670,7 +1670,7 @@ virDomainObjGetPersistentDef(virCapsPtr caps, } /* - * Helper method for --current --live --config option, and check with + * Helper method for --current, --live, and --config options, and check * whether domain is active or can get persistent domain configuration. * * Return 0 if success, also change the flags and get the persistent @@ -1704,12 +1704,13 @@ virDomainLiveConfigHelperMethod(virCapsPtr caps, if (*flags & VIR_DOMAIN_AFFECT_CONFIG) { if (!dom->persistent) { virDomainReportError(VIR_ERR_OPERATION_INVALID, "%s", - _("cannot change persistent config of a transient domain")); + _("cannot change persistent config of a " + "transient domain")); goto cleanup; } if (!(*persistentDef = virDomainObjGetPersistentDef(caps, dom))) { virDomainReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("Get persistent config failed")); + _("Get persistent config failed")); goto cleanup; } } diff --git i/src/qemu/qemu_driver.c w/src/qemu/qemu_driver.c index d63eeda..725b593 100644 --- i/src/qemu/qemu_driver.c +++ w/src/qemu/qemu_driver.c @@ -1810,7 +1810,6 @@ static int qemudDomainSetMemoryFlags(virDomainPtr dom, unsigned long newmem, virDomainObjPtr vm; virDomainDefPtr persistentDef = NULL; int ret = -1, r; - bool isActive; virCheckFlags(VIR_DOMAIN_AFFECT_LIVE | VIR_DOMAIN_AFFECT_CONFIG | @@ -1830,16 +1829,8 @@ static int qemudDomainSetMemoryFlags(virDomainPtr dom, unsigned long newmem, if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0) goto cleanup; - isActive = virDomainObjIsActive(vm); - - if (flags == VIR_DOMAIN_MEM_MAXIMUM) { - if (isActive) - flags = VIR_DOMAIN_AFFECT_LIVE | VIR_DOMAIN_MEM_MAXIMUM; - else - flags = VIR_DOMAIN_AFFECT_CONFIG | VIR_DOMAIN_MEM_MAXIMUM; - } - - if (virDomainLiveConfigHelperMethod(driver->caps, vm, &flags, &persistentDef) < 0) + if (virDomainLiveConfigHelperMethod(driver->caps, vm, &flags, + &persistentDef) < 0) goto endjob; if (flags & VIR_DOMAIN_MEM_MAXIMUM) { @@ -3292,7 +3283,8 @@ qemuDomainSetVcpusFlags(virDomainPtr dom, unsigned int nvcpus, maximum = (flags & VIR_DOMAIN_VCPU_MAXIMUM) != 0; flags &= ~VIR_DOMAIN_VCPU_MAXIMUM; - if (virDomainLiveConfigHelperMethod(driver->caps, vm, &flags, &persistentDef) < 0) + if (virDomainLiveConfigHelperMethod(driver->caps, vm, &flags, + &persistentDef) < 0) goto endjob; /* MAXIMUM cannot be mixed with LIVE. */ @@ -3403,7 +3395,8 @@ qemudDomainPinVcpuFlags(virDomainPtr dom, goto cleanup; } - if (virDomainLiveConfigHelperMethod(driver->caps, vm, &flags, &persistentDef) < 0) + if (virDomainLiveConfigHelperMethod(driver->caps, vm, &flags, + &persistentDef) < 0) goto cleanup; priv = vm->privateData; @@ -3535,7 +3528,8 @@ qemudDomainGetVcpuPinInfo(virDomainPtr dom, goto cleanup; } - if (virDomainLiveConfigHelperMethod(driver->caps, vm, &flags, &targetDef) < 0) + if (virDomainLiveConfigHelperMethod(driver->caps, vm, &flags, + &targetDef) < 0) goto cleanup; if (flags & VIR_DOMAIN_AFFECT_LIVE) { @@ -5929,7 +5923,8 @@ static int qemuDomainSetBlkioParameters(virDomainPtr dom, goto cleanup; } - if (virDomainLiveConfigHelperMethod(driver->caps, vm, &flags, &persistentDef) < 0) + if (virDomainLiveConfigHelperMethod(driver->caps, vm, &flags, + &persistentDef) < 0) goto cleanup; if (flags & VIR_DOMAIN_AFFECT_LIVE) { @@ -6125,7 +6120,8 @@ static int qemuDomainGetBlkioParameters(virDomainPtr dom, goto cleanup; } - if (virDomainLiveConfigHelperMethod(driver->caps, vm, &flags, &persistentDef) < 0) + if (virDomainLiveConfigHelperMethod(driver->caps, vm, &flags, + &persistentDef) < 0) goto cleanup; if (flags & VIR_DOMAIN_AFFECT_LIVE) { @@ -6310,7 +6306,8 @@ static int qemuDomainSetMemoryParameters(virDomainPtr dom, goto cleanup; } - if (virDomainLiveConfigHelperMethod(driver->caps, vm, &flags, &persistentDef) < 0) + if (virDomainLiveConfigHelperMethod(driver->caps, vm, &flags, + &persistentDef) < 0) goto cleanup; if (flags & VIR_DOMAIN_AFFECT_LIVE) { @@ -6449,7 +6446,8 @@ static int qemuDomainGetMemoryParameters(virDomainPtr dom, goto cleanup; } - if (virDomainLiveConfigHelperMethod(driver->caps, vm, &flags, &persistentDef) < 0) + if (virDomainLiveConfigHelperMethod(driver->caps, vm, &flags, + &persistentDef) < 0) goto cleanup; if (flags & VIR_DOMAIN_AFFECT_LIVE) { @@ -10960,7 +10958,8 @@ qemuDomainSetBlockIoTune(virDomainPtr dom, if (qemuDomainObjBeginJobWithDriver(driver, vm, QEMU_JOB_MODIFY) < 0) goto cleanup; - if (virDomainLiveConfigHelperMethod(driver->caps, vm, &flags, &persistentDef) < 0) + if (virDomainLiveConfigHelperMethod(driver->caps, vm, &flags, + &persistentDef) < 0) goto endjob; for (i = 0; i < nparams; i++) { @@ -11096,7 +11095,8 @@ qemuDomainGetBlockIoTune(virDomainPtr dom, if (qemuDomainObjBeginJobWithDriver(driver, vm, QEMU_JOB_MODIFY) < 0) goto cleanup; - if (virDomainLiveConfigHelperMethod(driver->caps, vm, &flags, &persistentDef) < 0) + if (virDomainLiveConfigHelperMethod(driver->caps, vm, &flags, + &persistentDef) < 0) goto endjob; if (flags & VIR_DOMAIN_AFFECT_LIVE) { diff --git i/src/test/test_driver.c w/src/test/test_driver.c index 89f7df1..8f13ceb 100644 --- i/src/test/test_driver.c +++ w/src/test/test_driver.c @@ -2103,7 +2103,6 @@ testDomainGetVcpusFlags(virDomainPtr domain, unsigned int flags) virDomainObjPtr vm; virDomainDefPtr def; int ret = -1; - bool active; virCheckFlags(VIR_DOMAIN_AFFECT_LIVE | VIR_DOMAIN_AFFECT_CONFIG | @@ -2121,37 +2120,11 @@ testDomainGetVcpusFlags(virDomainPtr domain, unsigned int flags) goto cleanup; } - active = virDomainObjIsActive(vm); - - if ((flags & (VIR_DOMAIN_VCPU_LIVE | VIR_DOMAIN_VCPU_CONFIG)) == 0) { - if (active) - flags |= VIR_DOMAIN_VCPU_LIVE; - else - flags |= VIR_DOMAIN_VCPU_CONFIG; - } - if ((flags & VIR_DOMAIN_AFFECT_LIVE) && (flags & VIR_DOMAIN_AFFECT_CONFIG)) { - testError(VIR_ERR_INVALID_ARG, - _("invalid flag combination: (0x%x)"), flags); - return -1; - } - - + if (virDomainLiveConfigHelperMethod(privconn->caps, vm, &flags, &def) < 0) + goto cleanup; - if (flags & VIR_DOMAIN_AFFECT_LIVE) { - if (!active) { - testError(VIR_ERR_OPERATION_INVALID, "%s", - _("domain not active")); - goto cleanup; - } + if (flags & VIR_DOMAIN_AFFECT_LIVE) def = vm->def; - } else { - if (!vm->persistent) { - testError(VIR_ERR_OPERATION_INVALID, "%s", - _("domain is transient")); - goto cleanup; - } - def = vm->newDef ? vm->newDef : vm->def; - } ret = (flags & VIR_DOMAIN_VCPU_MAXIMUM) ? def->maxvcpus : def->vcpus; -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
participants (2)
-
Eric Blake -
Lei Li