On 09/04/2012 08:12 AM, Peter Krempa wrote:
This patch tries to clean the code up a little bit and shorten very long lines. --- src/qemu/qemu_driver.c | 69 +++++++++++++++++++++++--------------------------- 1 file changed, 31 insertions(+), 38 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index b12d9bc..4b8b751 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -7801,6 +7801,8 @@ qemuSetSchedulerParametersFlags(virDomainPtr dom, virCgroupPtr group = NULL; virDomainObjPtr vm = NULL; virDomainDefPtr vmdef = NULL; + unsigned long long value_ul; + long long value_l; int ret = -1; int rc;
@@ -7857,74 +7859,65 @@ qemuSetSchedulerParametersFlags(virDomainPtr dom,
for (i = 0; i < nparams; i++) { virTypedParameterPtr param = ¶ms[i]; + value_ul = param->value.ul; + value_l = param->value.l;
Technically, reading two distinct values from a single union is undefined behavior; it is only well-defined to read a single value according to the discriminating type found outside the union. In practice, though, I see no practical issues with this, if Coverity and the like don't complain.
} else if (STREQ(param->field, VIR_DOMAIN_SCHEDULER_VCPU_PERIOD)) { - if (flags & VIR_DOMAIN_AFFECT_LIVE) { - rc = qemuSetVcpusBWLive(vm, group, params[i].value.ul, 0); - if (rc != 0) + if (flags & VIR_DOMAIN_AFFECT_LIVE && value_ul) { + if ((rc = qemuSetVcpusBWLive(vm, group, value_ul, 0))) goto cleanup;
- if (params[i].value.ul) - vm->def->cputune.period = params[i].value.ul; + vm->def->cputune.period = value_ul; }
This is a slight change in semantics; beforehand, it called qemuSetVcpusBWLive even when the value was 0; now it only calls the helper function for a non-zero value. Your commit message made it sound like this patch has no semantic impact; either it is wrong (this is intentional, so you should document it), or it was right (and this hunk is wrong). Which is it?
} else if (STREQ(param->field, VIR_DOMAIN_SCHEDULER_VCPU_QUOTA)) { - if (flags & VIR_DOMAIN_AFFECT_LIVE) { - rc = qemuSetVcpusBWLive(vm, group, 0, params[i].value.l); - if (rc != 0) + if (flags & VIR_DOMAIN_AFFECT_LIVE && value_l) {
And another one of those semantic changes.
} else if (STREQ(param->field, VIR_DOMAIN_SCHEDULER_EMULATOR_PERIOD)) { - if (flags & VIR_DOMAIN_AFFECT_LIVE) { - rc = qemuSetEmulatorBandwidthLive(vm, group, params[i].value.ul, 0); - if (rc != 0) + if (flags & VIR_DOMAIN_AFFECT_LIVE && value_ul) {
And another.
} else if (STREQ(param->field, VIR_DOMAIN_SCHEDULER_EMULATOR_QUOTA)) { - if (flags & VIR_DOMAIN_AFFECT_LIVE) { - rc = qemuSetEmulatorBandwidthLive(vm, group, 0, params[i].value.l); - if (rc != 0) + if (flags & VIR_DOMAIN_AFFECT_LIVE && value_l) {
And another. Depending on how you answer my complaint, I might be able to ack this with just an improved commit message, instead of needing a v2. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org