[libvirt] [PATCHv2 0/3] Add range checking for scheduler tunables
This series adds range checking to cpu scheduler tunables when changed using the API. V2 contains a new patch that avoids changing all tunables from virsh when setting just a single one. Peter Krempa (3): qemu: clean up qemuSetSchedulerParametersFlags() qemu: Add range checking for scheduler tunables when changed by API virsh: Update only changed scheduler tunables src/qemu/qemu_driver.c | 96 ++++++++++++++++++++++++++++++-------------------- tools/virsh-domain.c | 14 ++++---- 2 files changed, 66 insertions(+), 44 deletions(-) -- 1.7.12
This patch tries to clean the code up a little bit and shorten very long lines. The apparent semantic change from moving the condition before calling the setter function is a non-issue here as the setter function is a no-op when called with both arguments zero. --- Diff to v1: - enhanced commit message to explain apparent semantic change --- 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
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. This check is done only when using the API. It is still possible to specify invalid values in the XML. --- Diff to v1: - changed typecast to native long long specification --- 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..8e8e00c 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 1000LL +#define QEMU_SCHED_MAX_PERIOD 1000000LL +#define QEMU_SCHED_MIN_QUOTA 1000LL +#define QEMU_SCHED_MAX_QUOTA 18446744073709551LL + #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
When setting the cpu tunables in virsh you are able to update only one of them. Virsh while doing the update updated all of the tunables with old values except for the one changed. This is unfortunate as it: a) might overwrite some other change by a race condition (unprobable) b) fails with range checking as some of the old values saved might be out of range This patch changes the update procedure so that only the changed value is updated on the host. --- New in series. --- tools/virsh-domain.c | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index dcf2b8c..2abb457 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -3390,7 +3390,7 @@ cmdSchedinfo(vshControl *ctl, const vshCmd *cmd) virDomainPtr dom; virTypedParameterPtr params = NULL; int nparams = 0; - int update = 0; + int update = -1; int i, ret; bool ret_val = false; unsigned int flags = 0; @@ -3450,16 +3450,18 @@ cmdSchedinfo(vshControl *ctl, const vshCmd *cmd) goto cleanup; if (ret == 1) - update = 1; + update = i; } /* Update parameters & refresh data */ - if (update) { + if (update >= 0) { if (flags || current) - ret = virDomainSetSchedulerParametersFlags(dom, params, - nparams, flags); + ret = virDomainSetSchedulerParametersFlags(dom, + &(params[update]), + 1, flags); else - ret = virDomainSetSchedulerParameters(dom, params, nparams); + ret = virDomainSetSchedulerParameters(dom, + &(params[update]), 1); if (ret == -1) goto cleanup; -- 1.7.12
participants (1)
-
Peter Krempa