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