[libvirt] [PATCH] qemu: Refactor qemuDomainSetMemoryParameters
John Ferlan
jferlan at redhat.com
Fri Feb 22 12:56:26 UTC 2013
On 02/18/2013 10:18 AM, Peter Krempa wrote:
> The new TypedParam helper APIs allow to simplify this function
> significantly.
>
> This patch integrates the fix in 75e5bec97b3045e4f926248d5c742f8a50d0f9
> by correctly ordering the setting functions instead of reordering the
> parameters.
> ---
> src/qemu/qemu_driver.c | 149 ++++++++++++++++++-------------------------------
> 1 file changed, 54 insertions(+), 95 deletions(-)
>
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 23499ef..f2f5872 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -7140,15 +7140,15 @@ qemuDomainSetMemoryParameters(virDomainPtr dom,
> unsigned int flags)
> {
> virQEMUDriverPtr driver = dom->conn->privateData;
> - int i;
> virDomainDefPtr persistentDef = NULL;
> virCgroupPtr group = NULL;
> virDomainObjPtr vm = NULL;
> - virTypedParameterPtr hard_limit = NULL;
> - virTypedParameterPtr swap_hard_limit = NULL;
> - int hard_limit_index = 0;
> - int swap_hard_limit_index = 0;
> - unsigned long long val = 0;
> + unsigned long long swap_hard_limit;
> + unsigned long long memory_hard_limit;
> + unsigned long long memory_soft_limit;
> + bool set_swap_hard_limit = false;
> + bool set_memory_hard_limit = false;
> + bool set_memory_soft_limit = false;
> virQEMUDriverConfigPtr cfg = NULL;
> int ret = -1;
> int rc;
> @@ -7167,13 +7167,9 @@ qemuDomainSetMemoryParameters(virDomainPtr dom,
> NULL) < 0)
> return -1;
>
> - vm = virDomainObjListFindByUUID(driver->domains, dom->uuid);
>
> - if (vm == NULL) {
> - virReportError(VIR_ERR_INTERNAL_ERROR,
> - _("No such domain %s"), dom->uuid);
> - goto cleanup;
> - }
> + if (!(vm = qemuDomObjFromDomain(dom)))
> + return -1;
>
> cfg = virQEMUDriverGetConfig(driver);
>
> @@ -7198,110 +7194,73 @@ qemuDomainSetMemoryParameters(virDomainPtr dom,
> }
> }
>
> - for (i = 0; i < nparams; i++) {
> - if (STREQ(params[i].field, VIR_DOMAIN_MEMORY_HARD_LIMIT)) {
> - hard_limit = ¶ms[i];
> - hard_limit_index = i;
> - } else if (STREQ(params[i].field, VIR_DOMAIN_MEMORY_SWAP_HARD_LIMIT)) {
> - swap_hard_limit = ¶ms[i];
> - swap_hard_limit_index = i;
> - }
> - }
> +#define VIR_GET_LIMIT_PARAMETER(PARAM, VALUE) \
> + if ((rc = virTypedParamsGetULLong(params, nparams, PARAM, &VALUE) < 0)) \
> + goto cleanup; \
> + \
> + if (rc == 1) \
> + set_ ## VALUE = true;
> +
> + VIR_GET_LIMIT_PARAMETER(VIR_DOMAIN_MEMORY_SWAP_HARD_LIMIT, swap_hard_limit)
> + VIR_GET_LIMIT_PARAMETER(VIR_DOMAIN_MEMORY_HARD_LIMIT, memory_hard_limit)
> + VIR_GET_LIMIT_PARAMETER(VIR_DOMAIN_MEMORY_SOFT_LIMIT, memory_soft_limit)
> +
> +#undef VIR_GET_LIMIT_PARAMETER
> +
>
> /* It will fail if hard limit greater than swap hard limit anyway */
> - if (swap_hard_limit &&
> - hard_limit &&
> - (hard_limit->value.ul > swap_hard_limit->value.ul)) {
> + if (set_swap_hard_limit && set_memory_hard_limit &&
> + (memory_hard_limit > swap_hard_limit)) {
> virReportError(VIR_ERR_INVALID_ARG, "%s",
> _("hard limit must be lower than swap hard limit"));
Consider the messages below, should this be "memory hard_limit tunable
value must be lower than swap_hard_limit"
> goto cleanup;
> }
>
> - if (flags & VIR_DOMAIN_AFFECT_LIVE) {
> - /* Get current swap hard limit */
> - rc = virCgroupGetMemSwapHardLimit(group, &val);
> - if (rc != 0) {
> + if (set_swap_hard_limit) {
> + if (flags & VIR_DOMAIN_AFFECT_LIVE &&
> + (rc = virCgroupSetMemSwapHardLimit(group, swap_hard_limit)) < 0) {
> virReportSystemError(-rc, "%s",
> - _("unable to get swap hard limit"));
> + _("unable to set memory swap_hard_limit tunable"));
> goto cleanup;
> }
>
> - /* Swap hard_limit and swap_hard_limit to ensure the setting
> - * could succeed if both of them are provided.
> - */
> - if (swap_hard_limit && hard_limit) {
> - virTypedParameter param;
> -
> - if (swap_hard_limit->value.ul > val) {
> - if (hard_limit_index < swap_hard_limit_index) {
> - param = params[hard_limit_index];
> - params[hard_limit_index] = params[swap_hard_limit_index];
> - params[swap_hard_limit_index] = param;
> - }
> - } else {
> - if (hard_limit_index > swap_hard_limit_index) {
> - param = params[hard_limit_index];
> - params[hard_limit_index] = params[swap_hard_limit_index];
> - params[swap_hard_limit_index] = param;
> - }
> - }
> - }
> + if (flags & VIR_DOMAIN_AFFECT_CONFIG)
> + persistentDef->mem.swap_hard_limit = swap_hard_limit;
> }
>
> - ret = 0;
> - for (i = 0; i < nparams; i++) {
> - virTypedParameterPtr param = ¶ms[i];
> -
> - if (STREQ(param->field, VIR_DOMAIN_MEMORY_HARD_LIMIT)) {
> - if (flags & VIR_DOMAIN_AFFECT_LIVE) {
> - rc = virCgroupSetMemoryHardLimit(group, param->value.ul);
> - if (rc != 0) {
> - virReportSystemError(-rc, "%s",
> - _("unable to set memory hard_limit tunable"));
> - ret = -1;
> - }
> - }
> + if (set_memory_hard_limit) {
> + if (flags & VIR_DOMAIN_AFFECT_LIVE &&
> + (rc = virCgroupSetMemoryHardLimit(group, memory_hard_limit)) < 0) {
> + virReportSystemError(-rc, "%s",
> + _("unable to set memory hard_limit tunable"));
> + goto cleanup;
> + }
>
> - if (flags & VIR_DOMAIN_AFFECT_CONFIG) {
> - persistentDef->mem.hard_limit = param->value.ul;
> - }
> - } else if (STREQ(param->field, VIR_DOMAIN_MEMORY_SOFT_LIMIT)) {
> - if (flags & VIR_DOMAIN_AFFECT_LIVE) {
> - rc = virCgroupSetMemorySoftLimit(group, param->value.ul);
> - if (rc != 0) {
> - virReportSystemError(-rc, "%s",
> - _("unable to set memory soft_limit tunable"));
> - ret = -1;
> - }
> - }
> + if (flags & VIR_DOMAIN_AFFECT_CONFIG)
> + persistentDef->mem.hard_limit = memory_hard_limit;
> + }
>
> - if (flags & VIR_DOMAIN_AFFECT_CONFIG) {
> - persistentDef->mem.soft_limit = param->value.ul;
> - }
> - } else if (STREQ(param->field, VIR_DOMAIN_MEMORY_SWAP_HARD_LIMIT)) {
> - if (flags & VIR_DOMAIN_AFFECT_LIVE) {
> - rc = virCgroupSetMemSwapHardLimit(group, param->value.ul);
> - if (rc != 0) {
> - virReportSystemError(-rc, "%s",
> - _("unable to set swap_hard_limit tunable"));
> - ret = -1;
> - }
> - }
> - if (flags & VIR_DOMAIN_AFFECT_CONFIG) {
> - persistentDef->mem.swap_hard_limit = param->value.ul;
> - }
> + if (set_memory_soft_limit) {
> + if (flags & VIR_DOMAIN_AFFECT_LIVE &&
> + (rc = virCgroupSetMemorySoftLimit(group, memory_soft_limit)) < 0) {
> + virReportSystemError(-rc, "%s",
> + _("unable to set memory soft_limit tunable"));
> + goto cleanup;
> }
> - }
>
> - if (flags & VIR_DOMAIN_AFFECT_CONFIG) {
> - if (virDomainSaveConfig(cfg->configDir, persistentDef) < 0)
> - ret = -1;
> + if (flags & VIR_DOMAIN_AFFECT_CONFIG)
> + persistentDef->mem.soft_limit = memory_soft_limit;
> }
>
> + if (flags & VIR_DOMAIN_AFFECT_CONFIG &&
> + virDomainSaveConfig(cfg->configDir, persistentDef) < 0)
> + goto cleanup;
> +
> + ret = 0;
> +
> cleanup:
> virCgroupFree(&group);
> - if (vm)
> - virObjectUnlock(vm);
> + virObjectUnlock(vm);
> virObjectUnref(caps);
> virObjectUnref(cfg);
> return ret;
>
ACK - seems reasonable to me - your choice on the error message
John
More information about the libvir-list
mailing list