9:12 a.m.
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
9:12 a.m.
New subject: [libvirt] [PATCH 1/2] qemu: clean up qemuSetSchedulerParametersFlags()
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
9:48 a.m.
New subject: [libvirt] [PATCH 1/2] qemu: clean up qemuSetSchedulerParametersFlags()
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(a)redhat.com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
9:12 a.m.
New subject: [libvirt] [PATCH 2/2] qemu: Add range checking for scheduler tunables
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
10:03 a.m.
New subject: [libvirt] [PATCH 2/2] qemu: Add range checking for scheduler tunables
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(a)redhat.com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
4462
days inactive
4462
days old
4 comments
2 participants
participants (2)
-
Eric Blake -
Peter Krempa