[libvirt] [PATCH 0/2] Add range checking for scheduler tunables
Peter Krempa (2): qemu: clean up qemuSetSchedulerParametersFlags() qemu: Add range checking for scheduler tunables src/qemu/qemu_driver.c | 96 ++++++++++++++++++++++++++++++-------------------- 1 file changed, 58 insertions(+), 38 deletions(-) -- 1.7.12
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; if (STREQ(param->field, VIR_DOMAIN_SCHEDULER_CPU_SHARES)) { if (flags & VIR_DOMAIN_AFFECT_LIVE) { - rc = virCgroupSetCpuShares(group, params[i].value.ul); - if (rc != 0) { + if ((rc = virCgroupSetCpuShares(group, value_ul))) { virReportSystemError(-rc, "%s", _("unable to set cpu shares tunable")); goto cleanup; } - - vm->def->cputune.shares = params[i].value.ul; + vm->def->cputune.shares = value_ul; } - if (flags & VIR_DOMAIN_AFFECT_CONFIG) { - vmdef->cputune.shares = params[i].value.ul; - } + if (flags & VIR_DOMAIN_AFFECT_CONFIG) + vmdef->cputune.shares = value_ul; + } 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; } - if (flags & VIR_DOMAIN_AFFECT_CONFIG) { + if (flags & VIR_DOMAIN_AFFECT_CONFIG) vmdef->cputune.period = params[i].value.ul; - } + } 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) { + if ((rc = qemuSetVcpusBWLive(vm, group, 0, value_l))) goto cleanup; - if (params[i].value.l) - vm->def->cputune.quota = params[i].value.l; + vm->def->cputune.quota = value_l; } - if (flags & VIR_DOMAIN_AFFECT_CONFIG) { - vmdef->cputune.quota = params[i].value.l; - } + if (flags & VIR_DOMAIN_AFFECT_CONFIG) + vmdef->cputune.quota = value_l; + } 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) { + if ((rc = qemuSetEmulatorBandwidthLive(vm, group, value_ul, 0))) goto cleanup; - if (params[i].value.ul) - vm->def->cputune.emulator_period = params[i].value.ul; + vm->def->cputune.emulator_period = value_ul; } - if (flags & VIR_DOMAIN_AFFECT_CONFIG) { - vmdef->cputune.emulator_period = params[i].value.ul; - } + if (flags & VIR_DOMAIN_AFFECT_CONFIG) + vmdef->cputune.emulator_period = value_ul; + } 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) { + if ((rc = qemuSetEmulatorBandwidthLive(vm, group, 0, value_l))) goto cleanup; - if (params[i].value.l) - vm->def->cputune.emulator_quota = params[i].value.l; + vm->def->cputune.emulator_quota = value_l; } - if (flags & VIR_DOMAIN_AFFECT_CONFIG) { - vmdef->cputune.emulator_quota = params[i].value.l; - } + if (flags & VIR_DOMAIN_AFFECT_CONFIG) + vmdef->cputune.emulator_quota = value_l; } } -- 1.7.12
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
The quota and period tunables for cpu scheduler accept only a certain range of values. When changing the live configuration invalid values get rejected. This check is not performed when changing persistent config. This patch adds a separate range check, that improves error messages when changing live config and adds the check for persistent config. --- src/qemu/qemu_driver.c | 27 +++++++++++++++++++++++++++ 1 file changed, 27 insertions(+) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 4b8b751..3d59594 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -106,6 +106,11 @@ #define QEMU_NB_TOTAL_CPU_STAT_PARAM 3 #define QEMU_NB_PER_CPU_STAT_PARAM 2 +#define QEMU_SCHED_MIN_PERIOD (long long) 1000 +#define QEMU_SCHED_MAX_PERIOD (long long) 1000000 +#define QEMU_SCHED_MIN_QUOTA (long long) 1000 +#define QEMU_SCHED_MAX_QUOTA (long long) 18446744073709551 + #if HAVE_LINUX_KVM_H # include <linux/kvm.h> #endif @@ -7790,6 +7795,15 @@ cleanup: return -1; } +#define SCHED_RANGE_CHECK(VAR, NAME, MIN, MAX) \ + if (((VAR) > 0 && (VAR) < (MIN)) || (VAR) > (MAX)) { \ + virReportError(VIR_ERR_INVALID_ARG, \ + _("value of '%s' is out of range [%lld,%lld]"), \ + NAME, MIN, MAX); \ + rc = -1; \ + goto cleanup; \ + } + static int qemuSetSchedulerParametersFlags(virDomainPtr dom, virTypedParameterPtr params, @@ -7876,6 +7890,9 @@ qemuSetSchedulerParametersFlags(virDomainPtr dom, vmdef->cputune.shares = value_ul; } else if (STREQ(param->field, VIR_DOMAIN_SCHEDULER_VCPU_PERIOD)) { + SCHED_RANGE_CHECK(value_ul, VIR_DOMAIN_SCHEDULER_VCPU_PERIOD, + QEMU_SCHED_MIN_PERIOD, QEMU_SCHED_MAX_PERIOD); + if (flags & VIR_DOMAIN_AFFECT_LIVE && value_ul) { if ((rc = qemuSetVcpusBWLive(vm, group, value_ul, 0))) goto cleanup; @@ -7887,6 +7904,9 @@ qemuSetSchedulerParametersFlags(virDomainPtr dom, vmdef->cputune.period = params[i].value.ul; } else if (STREQ(param->field, VIR_DOMAIN_SCHEDULER_VCPU_QUOTA)) { + SCHED_RANGE_CHECK(value_l, VIR_DOMAIN_SCHEDULER_VCPU_QUOTA, + QEMU_SCHED_MIN_QUOTA, QEMU_SCHED_MAX_QUOTA); + if (flags & VIR_DOMAIN_AFFECT_LIVE && value_l) { if ((rc = qemuSetVcpusBWLive(vm, group, 0, value_l))) goto cleanup; @@ -7898,6 +7918,9 @@ qemuSetSchedulerParametersFlags(virDomainPtr dom, vmdef->cputune.quota = value_l; } else if (STREQ(param->field, VIR_DOMAIN_SCHEDULER_EMULATOR_PERIOD)) { + SCHED_RANGE_CHECK(value_ul, VIR_DOMAIN_SCHEDULER_EMULATOR_PERIOD, + QEMU_SCHED_MIN_PERIOD, QEMU_SCHED_MAX_PERIOD); + if (flags & VIR_DOMAIN_AFFECT_LIVE && value_ul) { if ((rc = qemuSetEmulatorBandwidthLive(vm, group, value_ul, 0))) goto cleanup; @@ -7909,6 +7932,9 @@ qemuSetSchedulerParametersFlags(virDomainPtr dom, vmdef->cputune.emulator_period = value_ul; } else if (STREQ(param->field, VIR_DOMAIN_SCHEDULER_EMULATOR_QUOTA)) { + SCHED_RANGE_CHECK(value_l, VIR_DOMAIN_SCHEDULER_EMULATOR_QUOTA, + QEMU_SCHED_MIN_QUOTA, QEMU_SCHED_MAX_QUOTA); + if (flags & VIR_DOMAIN_AFFECT_LIVE && value_l) { if ((rc = qemuSetEmulatorBandwidthLive(vm, group, 0, value_l))) goto cleanup; @@ -7944,6 +7970,7 @@ cleanup: qemuDriverUnlock(driver); return ret; } +#undef SCHED_RANGE_CHECK static int qemuSetSchedulerParameters(virDomainPtr dom, -- 1.7.12
On 09/04/2012 08:12 AM, Peter Krempa wrote:
The quota and period tunables for cpu scheduler accept only a certain range of values. When changing the live configuration invalid values get rejected. This check is not performed when changing persistent config.
This patch adds a separate range check, that improves error messages when changing live config and adds the check for persistent config. --- src/qemu/qemu_driver.c | 27 +++++++++++++++++++++++++++ 1 file changed, 27 insertions(+)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 4b8b751..3d59594 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -106,6 +106,11 @@ #define QEMU_NB_TOTAL_CPU_STAT_PARAM 3 #define QEMU_NB_PER_CPU_STAT_PARAM 2
+#define QEMU_SCHED_MIN_PERIOD (long long) 1000
Under-parenthesized; if you insist on the cast, it should be: #define QEMU_SCHED_MIN_PERIOD ((long long) 1000) that said, it is possible to be more concise (with no parentheses necessary): #define QEMU_SCHED_MIN_PERIOD 1000LL
+#define QEMU_SCHED_MAX_PERIOD (long long) 1000000 +#define QEMU_SCHED_MIN_QUOTA (long long) 1000 +#define QEMU_SCHED_MAX_QUOTA (long long) 18446744073709551
Furthermore, in the case of MAX_QUOTA, your code is wrong. The C compiler treats this as ((long long) ((int) 18446744073709551)), which is 1271310319; here, you absolutely need the LL suffix, at which point you no longer need the cast. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
participants (2)
-
Eric Blake -
Peter Krempa