Re: [libvirt] [PATCH] qemu: Refactor qemuDomainSetMemoryParameters

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 = &params[i]; - hard_limit_index = i; - } else if (STREQ(params[i].field, VIR_DOMAIN_MEMORY_SWAP_HARD_LIMIT)) { - swap_hard_limit = &params[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"));

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 = &params[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;