Re: [libvirt] [PATCH v2] Provide a helper method virDomainLiveConfigHelperMethod

On 12/11/2011 10:16 PM, Lei Li wrote: 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. 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 :) Grammar: s/option/options/ s/with// Indentation. We can simplify this even further, now that virDomainLiveConfigHelperMethod leaves the rest of the bits in flags alone. 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(a)redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org