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@xxxxxxxxxx>
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@xxxxxxxxxxxxxxxxxx>
> ---
> 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@xxxxxxxxxx +1-919-301-3266
Libvirt virtualization library http://libvirt.org
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list