[libvirt] [PATCHv3 0/4] Add range checking for scheduler tunables
This version improves virsh so that it doesn't update all tunables but only those that changed (and now correctly). This series contains a new patch that adds a helper to deal with typed parameters. Regarding typed parameters: Shouldn't we make the helpers to deal with them public? (Patches 1 and 2 didn't change to v2) Peter Krempa (4): qemu: clean up qemuSetSchedulerParametersFlags() qemu: Add range checking for scheduler tunables when changed by API util: Add helper to assign typed params from string virsh: Update only changed scheduler tunables src/libvirt_private.syms | 1 + src/qemu/qemu_driver.c | 96 ++++++++++++++++----------- src/util/virtypedparam.c | 97 ++++++++++++++++++++++++++++ src/util/virtypedparam.h | 6 ++ tools/virsh-domain.c | 164 +++++++++++++++++++++-------------------------- 5 files changed, 236 insertions(+), 128 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. --- 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. --- 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
This patch adds a helper to deal with assigning values to virTypedParameter structures from strings. The helper parses the value from the string and assigns it to the corresponding union value. --- New in series. --- src/libvirt_private.syms | 1 + src/util/virtypedparam.c | 97 ++++++++++++++++++++++++++++++++++++++++++++++++ src/util/virtypedparam.h | 6 +++ 3 files changed, 104 insertions(+) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 65067d6..c8dcece 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1734,6 +1734,7 @@ virTimeStringThenRaw; virTypedParameterArrayClear; virTypedParameterArrayValidate; virTypedParameterAssign; +virTypedParameterAssignFromStr; # viruri.h diff --git a/src/util/virtypedparam.c b/src/util/virtypedparam.c index d68d79f..89a0caa 100644 --- a/src/util/virtypedparam.c +++ b/src/util/virtypedparam.c @@ -181,3 +181,100 @@ cleanup: va_end(ap); return ret; } + +/* Assign name, type, and convert the argument from a const string. + * In case of a string, the string is copied. + * Return 0 on success, -1 after an error message on failure. */ +int +virTypedParameterAssignFromStr(virTypedParameterPtr param, const char *name, + int type, const char *val) +{ + int ret = -1; + + if (!val) { + virReportError(VIR_ERR_INVALID_ARG, _("NULL value for field '%s'"), + name); + goto cleanup; + } + + if (virStrcpyStatic(param->field, name) == NULL) { + virReportError(VIR_ERR_INTERNAL_ERROR, _("Field name '%s' too long"), + name); + goto cleanup; + } + + param->type = type; + switch (type) { + case VIR_TYPED_PARAM_INT: + if (virStrToLong_i(val, NULL, 10, ¶m->value.i) < 0) { + virReportError(VIR_ERR_INVALID_ARG, + _("Invalid value for field '%s': expected int"), + name); + goto cleanup; + } + break; + case VIR_TYPED_PARAM_UINT: + if (virStrToLong_ui(val, NULL, 10, ¶m->value.ui) < 0) { + virReportError(VIR_ERR_INVALID_ARG, + _("Invalid value for field '%s': " + "expected unsigned int"), + name); + goto cleanup; + } + break; + case VIR_TYPED_PARAM_LLONG: + if (virStrToLong_ll(val, NULL, 10, ¶m->value.l) < 0) { + virReportError(VIR_ERR_INVALID_ARG, + _("Invalid value for field '%s': " + "expected long long"), + name); + goto cleanup; + } + break; + case VIR_TYPED_PARAM_ULLONG: + if (virStrToLong_ull(val, NULL, 10, ¶m->value.ul) < 0) { + virReportError(VIR_ERR_INVALID_ARG, + _("Invalid value for field '%s': " + "expected long long"), + name); + goto cleanup; + } + break; + case VIR_TYPED_PARAM_DOUBLE: + if (virStrToDouble(val, NULL, ¶m->value.d) < 0) { + virReportError(VIR_ERR_INVALID_ARG, + _("Invalid value for field '%s': " + "expected double"), + name); + goto cleanup; + } + break; + case VIR_TYPED_PARAM_BOOLEAN: + if (STRCASEEQ(val, "true") || + STREQ(val, "1")) { + param->value.b = true; + } else if (STRCASEEQ(val, "false") || + STREQ(val, "0")) { + param->value.b = false; + } else { + virReportError(VIR_ERR_INVALID_ARG, + _("Invalid boolean value for field '%s'"), name); + goto cleanup; + } + break; + case VIR_TYPED_PARAM_STRING: + if (!(param->value.s = strdup(val))) { + virReportOOMError(); + goto cleanup; + } + break; + default: + virReportError(VIR_ERR_INTERNAL_ERROR, + _("unexpected type %d for field %s"), type, name); + goto cleanup; + } + + ret = 0; +cleanup: + return ret; +} diff --git a/src/util/virtypedparam.h b/src/util/virtypedparam.h index f117b84..581201c 100644 --- a/src/util/virtypedparam.h +++ b/src/util/virtypedparam.h @@ -35,4 +35,10 @@ int virTypedParameterAssign(virTypedParameterPtr param, const char *name, int type, /* TYPE arg */ ...) ATTRIBUTE_RETURN_CHECK; +int virTypedParameterAssignFromStr(virTypedParameterPtr param, + const char *name, + int type, + const char *val) + ATTRIBUTE_RETURN_CHECK; + #endif /* __VIR_TYPED_PARAM_H */ -- 1.7.12
On 09/06/2012 08:01 AM, Peter Krempa wrote:
This patch adds a helper to deal with assigning values to virTypedParameter structures from strings. The helper parses the value from the string and assigns it to the corresponding union value. --- New in series.
+ case VIR_TYPED_PARAM_ULLONG: + if (virStrToLong_ull(val, NULL, 10, ¶m->value.ul) < 0) { + virReportError(VIR_ERR_INVALID_ARG, + _("Invalid value for field '%s': " + "expected long long"),
expected unsigned long long -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
When setting the cpu tunables in virsh you are able to update only a subset of them. Virsh while doing the update updated all of the tunables, changed ones with new values and unchanged with old ones. 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. This patch also fixes a very unprobable memory leak if the daemon would return a string tunable parameter, as the typed parameter array was not cleared. --- Diff to v2: - handle legacy xen parameters correctly - create copy of parameters that should be updated (allows multiple parameter update) - simplify the update code using the new helper --- tools/virsh-domain.c | 164 +++++++++++++++++++++++---------------------------- 1 file changed, 74 insertions(+), 90 deletions(-) diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 4684466..33229d6 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -3292,95 +3292,79 @@ static const vshCmdOptDef opts_schedinfo[] = { static int cmdSchedInfoUpdate(vshControl *ctl, const vshCmd *cmd, - virTypedParameterPtr param) + virTypedParameterPtr src_params, int nsrc_params, + virTypedParameterPtr *update_params) { - const char *data = NULL; - - /* Legacy 'weight' parameter */ - if (STREQ(param->field, "weight") && - param->type == VIR_TYPED_PARAM_UINT && - vshCommandOptBool(cmd, "weight")) { - int val; - if (vshCommandOptInt(cmd, "weight", &val) <= 0) { - vshError(ctl, "%s", _("Invalid value of weight")); - return -1; - } else { - param->value.ui = val; + const char *set_arg; + char *set_field = NULL; + char *set_val = NULL; + + virTypedParameterPtr param; + virTypedParameterPtr params = NULL; + int nparams = 0; + size_t params_size = 0; + int ret = -1; + int rv; + int val; + int i; + + if (vshCommandOptString(cmd, "set", &set_arg) > 0) { + set_field = vshStrdup(ctl, set_arg); + if (!(set_val = strchr(set_field, '='))) { + vshError(ctl, "%s", _("Invalid syntax for --set, expecting name=value")); + goto cleanup; } - return 1; + + *set_val = '\0'; + set_val++; } - /* Legacy 'cap' parameter */ - if (STREQ(param->field, "cap") && - param->type == VIR_TYPED_PARAM_UINT && - vshCommandOptBool(cmd, "cap")) { - int val; - if (vshCommandOptInt(cmd, "cap", &val) <= 0) { - vshError(ctl, "%s", _("Invalid value of cap")); - return -1; - } else { - param->value.ui = val; + for (i = 0; i < nsrc_params; i++) { + param = &(src_params[i]); + if (VIR_RESIZE_N(params, params_size, nparams, 1) < 0) { + virReportOOMError(); + goto cleanup; } - return 1; - } - if (vshCommandOptString(cmd, "set", &data) > 0) { - char *val = strchr(data, '='); - int match = 0; - if (!val) { - vshError(ctl, "%s", _("Invalid syntax for --set, expecting name=value")); - return -1; + /* Legacy 'weight' and 'cap' parameter */ + if (param->type == VIR_TYPED_PARAM_UINT && + (STREQ(param->field, "weight") || STREQ(param->field, "cap")) && + (rv = vshCommandOptInt(cmd, param->field, &val)) != 0) { + if (rv < 0) { + vshError(ctl, _("Invalid value of %s"), param->field); + goto cleanup; + } + + if (virTypedParameterAssign(&(params[nparams++]), + param->field, + param->type, + val) < 0) + goto cleanup; + + continue; } - *val = '\0'; - match = STREQ(data, param->field); - *val = '='; - val++; - if (!match) - return 0; - switch (param->type) { - case VIR_TYPED_PARAM_INT: - if (virStrToLong_i(val, NULL, 10, ¶m->value.i) < 0) { - vshError(ctl, "%s", - _("Invalid value for parameter, expecting an int")); - return -1; - } - break; - case VIR_TYPED_PARAM_UINT: - if (virStrToLong_ui(val, NULL, 10, ¶m->value.ui) < 0) { - vshError(ctl, "%s", - _("Invalid value for parameter, expecting an unsigned int")); - return -1; - } - break; - case VIR_TYPED_PARAM_LLONG: - if (virStrToLong_ll(val, NULL, 10, ¶m->value.l) < 0) { - vshError(ctl, "%s", - _("Invalid value for parameter, expecting a long long")); - return -1; - } - break; - case VIR_TYPED_PARAM_ULLONG: - if (virStrToLong_ull(val, NULL, 10, ¶m->value.ul) < 0) { - vshError(ctl, "%s", - _("Invalid value for parameter, expecting an unsigned long long")); - return -1; - } - break; - case VIR_TYPED_PARAM_DOUBLE: - if (virStrToDouble(val, NULL, ¶m->value.d) < 0) { - vshError(ctl, "%s", _("Invalid value for parameter, expecting a double")); - return -1; - } - break; - case VIR_TYPED_PARAM_BOOLEAN: - param->value.b = STREQ(val, "0") ? 0 : 1; + if (set_field && STREQ(set_field, param->field)) { + if (virTypedParameterAssignFromStr(&(params[nparams++]), + param->field, + param->type, + set_val) < 0) + goto cleanup; + + continue; } - return 1; } - return 0; + *update_params = params; + ret = nparams; + params = NULL; + +cleanup: + VIR_FREE(set_field); + virTypedParameterArrayClear(params, nparams); + VIR_FREE(params); + return ret; } static bool @@ -3389,8 +3373,9 @@ cmdSchedinfo(vshControl *ctl, const vshCmd *cmd) char *schedulertype; virDomainPtr dom; virTypedParameterPtr params = NULL; + virTypedParameterPtr updates = NULL; int nparams = 0; - int update = 0; + int nupdates = 0; int i, ret; bool ret_val = false; unsigned int flags = 0; @@ -3444,22 +3429,18 @@ cmdSchedinfo(vshControl *ctl, const vshCmd *cmd) goto cleanup; /* See if any params are being set */ - for (i = 0; i < nparams; i++) { - ret = cmdSchedInfoUpdate(ctl, cmd, &(params[i])); - if (ret == -1) - goto cleanup; - - if (ret == 1) - update = 1; - } + if ((nupdates = cmdSchedInfoUpdate(ctl, cmd, params, nparams, + &updates)) < 0) + goto cleanup; /* Update parameters & refresh data */ - if (update) { + if (nupdates > 0) { if (flags || current) - ret = virDomainSetSchedulerParametersFlags(dom, params, - nparams, flags); + ret = virDomainSetSchedulerParametersFlags(dom, updates, + nupdates, flags); else - ret = virDomainSetSchedulerParameters(dom, params, nparams); + ret = virDomainSetSchedulerParameters(dom, updates, nupdates); + if (ret == -1) goto cleanup; @@ -3499,7 +3480,10 @@ cmdSchedinfo(vshControl *ctl, const vshCmd *cmd) } cleanup: + virTypedParameterArrayClear(params, nparams); + virTypedParameterArrayClear(updates, nupdates); VIR_FREE(params); + VIR_FREE(updates); virDomainFree(dom); return ret_val; } -- 1.7.12
On 09/06/2012 08:01 AM, Peter Krempa wrote:
This version improves virsh so that it doesn't update all tunables but only those that changed (and now correctly).
This series contains a new patch that adds a helper to deal with typed parameters.
Regarding typed parameters: Shouldn't we make the helpers to deal with them public?
(Patches 1 and 2 didn't change to v2)
Technically, the commit message of 1 changed, but for the better; and you folded in my LL hint in commit 2.
Peter Krempa (4): qemu: clean up qemuSetSchedulerParametersFlags() qemu: Add range checking for scheduler tunables when changed by API util: Add helper to assign typed params from string virsh: Update only changed scheduler tunables
ACK series, with nit in 3 fixed. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 09/07/12 01:03, Eric Blake wrote:
On 09/06/2012 08:01 AM, Peter Krempa wrote:
This version improves virsh so that it doesn't update all tunables but only those that changed (and now correctly).
This series contains a new patch that adds a helper to deal with typed parameters.
Regarding typed parameters: Shouldn't we make the helpers to deal with them public?
(Patches 1 and 2 didn't change to v2)
Technically, the commit message of 1 changed, but for the better; and you folded in my LL hint in commit 2.
I noted that in v2, but that version had some trouble.
Peter Krempa (4): qemu: clean up qemuSetSchedulerParametersFlags() qemu: Add range checking for scheduler tunables when changed by API util: Add helper to assign typed params from string virsh: Update only changed scheduler tunables
ACK series, with nit in 3 fixed.
I fixed the nit and pushed the series. Thanks! Peter
participants (2)
-
Eric Blake -
Peter Krempa