[libvirt] [PATCH v2 0/3] unify memtune value representation in libvirt

The first patch rewrites virsh memtune command to accept 0 as valid value instead of ignoring it. In the second patch I'll introduce a simple helper to crop the *_limit values to VIR_DOMAIN_MEMORY_PARAM_UNLIMITED as its used several times while reading values from cgroups and also a helper to check whether the memory limits are set or not. The last patch actually changes the internal representation of unlimited memtune settings. More detailed description is in commit message. My motivation to change it completely is to prevent future bugs with those limits. Pavel Hrdina (3): virsh: fix memtune to also accept 0 as valid value virutil: introduce helper functions for memory limits memtune: change the way how we store unlimited value docs/formatdomain.html.in | 4 +- src/conf/domain_conf.c | 75 +++++++++++++++---- src/libvirt-domain.c | 3 + src/libvirt_private.syms | 3 +- src/lxc/lxc_cgroup.c | 18 ++--- src/lxc/lxc_driver.c | 7 +- src/lxc/lxc_fuse.c | 12 ++-- src/lxc/lxc_native.c | 6 +- src/openvz/openvz_conf.c | 4 +- src/openvz/openvz_driver.c | 4 +- src/qemu/qemu_cgroup.c | 24 +++---- src/qemu/qemu_command.c | 8 ++- src/qemu/qemu_driver.c | 10 +-- src/qemu/qemu_hotplug.c | 2 +- src/qemu/qemu_migration.c | 5 +- src/util/virutil.c | 47 ++++++------ src/util/virutil.h | 5 +- tests/qemuxml2argvdata/qemuxml2argv-memtune.xml | 2 +- .../qemuxml2xmloutdata/qemuxml2xmlout-memtune.xml | 2 +- tools/virsh-domain.c | 83 ++++++++++------------ 20 files changed, 185 insertions(+), 139 deletions(-) -- 2.0.5

Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- tools/virsh-domain.c | 83 ++++++++++++++++++++++++---------------------------- 1 file changed, 38 insertions(+), 45 deletions(-) diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 55c269c..3836a1b 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -8255,6 +8255,22 @@ static const vshCmdOptDef opts_memtune[] = { {.name = NULL} }; +/** + * vshMemtuneGetSize + * + * @cmd: pointer to vshCmd + * @name: name of a parameter for which we would like to get a value + * @value: pointer to variable where the value will be stored + * + * This function will parse virsh command line in order to load a value of + * specified parameter. If the value is -1 we will handle it as unlimited and + * use VIR_DOMAIN_MEMORY_PARAM_UNLIMITED instead. + * + * Returns: + * >0 if option found and valid (@value updated) + * 0 if option not found and not required (@value untouched) + * <0 in all other cases (@value untouched) + */ static int vshMemtuneGetSize(const vshCmd *cmd, const char *name, long long *value) { @@ -8276,7 +8292,7 @@ vshMemtuneGetSize(const vshCmd *cmd, const char *name, long long *value) if (virScaleInteger(&tmp, end, 1024, LLONG_MAX) < 0) return -1; *value = VIR_DIV_UP(tmp, 1024); - return 0; + return 1; } static bool @@ -8294,6 +8310,7 @@ cmdMemtune(vshControl *ctl, const vshCmd *cmd) bool current = vshCommandOptBool(cmd, "current"); bool config = vshCommandOptBool(cmd, "config"); bool live = vshCommandOptBool(cmd, "live"); + int rc; VSH_EXCLUSIVE_OPTIONS_VAR(current, live); VSH_EXCLUSIVE_OPTIONS_VAR(current, config); @@ -8306,50 +8323,26 @@ cmdMemtune(vshControl *ctl, const vshCmd *cmd) if (!(dom = vshCommandOptDomain(ctl, cmd, NULL))) return false; - if (vshMemtuneGetSize(cmd, "hard-limit", &hard_limit) < 0 || - vshMemtuneGetSize(cmd, "soft-limit", &soft_limit) < 0 || - vshMemtuneGetSize(cmd, "swap-hard-limit", &swap_hard_limit) < 0 || - vshMemtuneGetSize(cmd, "min-guarantee", &min_guarantee) < 0) { - vshError(ctl, "%s", - _("Unable to parse integer parameter")); - goto cleanup; - } - - if (hard_limit) { - if (hard_limit == -1) - hard_limit = VIR_DOMAIN_MEMORY_PARAM_UNLIMITED; - if (virTypedParamsAddULLong(¶ms, &nparams, &maxparams, - VIR_DOMAIN_MEMORY_HARD_LIMIT, - hard_limit) < 0) - goto save_error; - } - - if (soft_limit) { - if (soft_limit == -1) - soft_limit = VIR_DOMAIN_MEMORY_PARAM_UNLIMITED; - if (virTypedParamsAddULLong(¶ms, &nparams, &maxparams, - VIR_DOMAIN_MEMORY_SOFT_LIMIT, - soft_limit) < 0) - goto save_error; - } - - if (swap_hard_limit) { - if (swap_hard_limit == -1) - swap_hard_limit = VIR_DOMAIN_MEMORY_PARAM_UNLIMITED; - if (virTypedParamsAddULLong(¶ms, &nparams, &maxparams, - VIR_DOMAIN_MEMORY_SWAP_HARD_LIMIT, - swap_hard_limit) < 0) - goto save_error; - } - - if (min_guarantee) { - if (min_guarantee == -1) - min_guarantee = VIR_DOMAIN_MEMORY_PARAM_UNLIMITED; - if (virTypedParamsAddULLong(¶ms, &nparams, &maxparams, - VIR_DOMAIN_MEMORY_MIN_GUARANTEE, - min_guarantee) < 0) - goto save_error; - } +#define PARSE_MEMTUNE_PARAM(NAME, VALUE, FIELD) \ + if ((rc = vshMemtuneGetSize(cmd, NAME, &VALUE)) < 0) { \ + vshError(ctl, "%s", _("Unable to parse integer parameter 'NAME'")); \ + goto cleanup; \ + } \ + if (rc == 1) { \ + if (virTypedParamsAddULLong(¶ms, &nparams, &maxparams, \ + FIELD, VALUE) < 0) \ + goto save_error; \ + } \ + + + PARSE_MEMTUNE_PARAM("hard-limit", hard_limit, VIR_DOMAIN_MEMORY_HARD_LIMIT); + PARSE_MEMTUNE_PARAM("soft-limit", soft_limit, VIR_DOMAIN_MEMORY_SOFT_LIMIT); + PARSE_MEMTUNE_PARAM("swap-hard-limit", swap_hard_limit, + VIR_DOMAIN_MEMORY_SWAP_HARD_LIMIT); + PARSE_MEMTUNE_PARAM("min-guarantee", min_guarantee, + VIR_DOMAIN_MEMORY_MIN_GUARANTEE); + +#undef PARSE_MEMTUNE_PARAM if (nparams == 0) { /* get the number of memory parameters */ -- 2.0.5

On Thu, Mar 05, 2015 at 16:10:28 +0100, Pavel Hrdina wrote:
Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
Commit message is still too sparse ...
--- tools/virsh-domain.c | 83 ++++++++++++++++++++++++---------------------------- 1 file changed, 38 insertions(+), 45 deletions(-)
diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 55c269c..3836a1b 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -8255,6 +8255,22 @@ static const vshCmdOptDef opts_memtune[] = { {.name = NULL} };
+/** + * vshMemtuneGetSize + * + * @cmd: pointer to vshCmd + * @name: name of a parameter for which we would like to get a value + * @value: pointer to variable where the value will be stored + * + * This function will parse virsh command line in order to load a value of + * specified parameter. If the value is -1 we will handle it as unlimited and + * use VIR_DOMAIN_MEMORY_PARAM_UNLIMITED instead. + * + * Returns: + * >0 if option found and valid (@value updated) + * 0 if option not found and not required (@value untouched) + * <0 in all other cases (@value untouched)
The last one is not true. If the scaling function fails @value is touched but the function returns -1. I'd drop the statements altogether.
+ */ static int vshMemtuneGetSize(const vshCmd *cmd, const char *name, long long *value) { @@ -8276,7 +8292,7 @@ vshMemtuneGetSize(const vshCmd *cmd, const char *name, long long *value) if (virScaleInteger(&tmp, end, 1024, LLONG_MAX) < 0) return -1; *value = VIR_DIV_UP(tmp, 1024); - return 0; + return 1; }
static bool @@ -8294,6 +8310,7 @@ cmdMemtune(vshControl *ctl, const vshCmd *cmd) bool current = vshCommandOptBool(cmd, "current"); bool config = vshCommandOptBool(cmd, "config"); bool live = vshCommandOptBool(cmd, "live"); + int rc;
VSH_EXCLUSIVE_OPTIONS_VAR(current, live); VSH_EXCLUSIVE_OPTIONS_VAR(current, config); @@ -8306,50 +8323,26 @@ cmdMemtune(vshControl *ctl, const vshCmd *cmd) if (!(dom = vshCommandOptDomain(ctl, cmd, NULL))) return false;
- if (vshMemtuneGetSize(cmd, "hard-limit", &hard_limit) < 0 || - vshMemtuneGetSize(cmd, "soft-limit", &soft_limit) < 0 || - vshMemtuneGetSize(cmd, "swap-hard-limit", &swap_hard_limit) < 0 || - vshMemtuneGetSize(cmd, "min-guarantee", &min_guarantee) < 0) { - vshError(ctl, "%s", - _("Unable to parse integer parameter")); - goto cleanup; - } - - if (hard_limit) { - if (hard_limit == -1) - hard_limit = VIR_DOMAIN_MEMORY_PARAM_UNLIMITED; - if (virTypedParamsAddULLong(¶ms, &nparams, &maxparams, - VIR_DOMAIN_MEMORY_HARD_LIMIT, - hard_limit) < 0) - goto save_error; - } - - if (soft_limit) { - if (soft_limit == -1) - soft_limit = VIR_DOMAIN_MEMORY_PARAM_UNLIMITED; - if (virTypedParamsAddULLong(¶ms, &nparams, &maxparams, - VIR_DOMAIN_MEMORY_SOFT_LIMIT, - soft_limit) < 0) - goto save_error; - } - - if (swap_hard_limit) { - if (swap_hard_limit == -1) - swap_hard_limit = VIR_DOMAIN_MEMORY_PARAM_UNLIMITED; - if (virTypedParamsAddULLong(¶ms, &nparams, &maxparams, - VIR_DOMAIN_MEMORY_SWAP_HARD_LIMIT, - swap_hard_limit) < 0) - goto save_error; - } - - if (min_guarantee) { - if (min_guarantee == -1) - min_guarantee = VIR_DOMAIN_MEMORY_PARAM_UNLIMITED; - if (virTypedParamsAddULLong(¶ms, &nparams, &maxparams, - VIR_DOMAIN_MEMORY_MIN_GUARANTEE, - min_guarantee) < 0) - goto save_error; - } +#define PARSE_MEMTUNE_PARAM(NAME, VALUE, FIELD) \ + if ((rc = vshMemtuneGetSize(cmd, NAME, &VALUE)) < 0) { \ + vshError(ctl, "%s", _("Unable to parse integer parameter 'NAME'")); \ + goto cleanup; \ + } \ + if (rc == 1) { \ + if (virTypedParamsAddULLong(¶ms, &nparams, &maxparams, \ + FIELD, VALUE) < 0) \ + goto save_error; \ + } \ + + + PARSE_MEMTUNE_PARAM("hard-limit", hard_limit, VIR_DOMAIN_MEMORY_HARD_LIMIT); + PARSE_MEMTUNE_PARAM("soft-limit", soft_limit, VIR_DOMAIN_MEMORY_SOFT_LIMIT);
I just realized that the 'hard_limit', 'soft_limit' ... variables are used just as temp variables in the macro. You could get rid of them and use a single temp variable instead.
+ PARSE_MEMTUNE_PARAM("swap-hard-limit", swap_hard_limit, + VIR_DOMAIN_MEMORY_SWAP_HARD_LIMIT);
Which would make these fit on one line.
+ PARSE_MEMTUNE_PARAM("min-guarantee", min_guarantee, + VIR_DOMAIN_MEMORY_MIN_GUARANTEE); + +#undef PARSE_MEMTUNE_PARAM
if (nparams == 0) { /* get the number of memory parameters */
ACK if you drop the parenthesed section in the comment as pointed above. The rest is not mandatory, although I'd prefer if the patch had a proper commit message. Peter

On Fri, Mar 06, 2015 at 10:35:17AM +0100, Peter Krempa wrote:
On Thu, Mar 05, 2015 at 16:10:28 +0100, Pavel Hrdina wrote:
Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
Commit message is still too sparse ...
Ok, I've added few more lines to the commit message.
--- tools/virsh-domain.c | 83 ++++++++++++++++++++++++---------------------------- 1 file changed, 38 insertions(+), 45 deletions(-)
diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 55c269c..3836a1b 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -8255,6 +8255,22 @@ static const vshCmdOptDef opts_memtune[] = { {.name = NULL} };
+/** + * vshMemtuneGetSize + * + * @cmd: pointer to vshCmd + * @name: name of a parameter for which we would like to get a value + * @value: pointer to variable where the value will be stored + * + * This function will parse virsh command line in order to load a value of + * specified parameter. If the value is -1 we will handle it as unlimited and + * use VIR_DOMAIN_MEMORY_PARAM_UNLIMITED instead. + * + * Returns: + * >0 if option found and valid (@value updated) + * 0 if option not found and not required (@value untouched) + * <0 in all other cases (@value untouched)
The last one is not true. If the scaling function fails @value is touched but the function returns -1. I'd drop the statements altogether.
Good point, it was a copy&paste from another function comment.
+ */ static int vshMemtuneGetSize(const vshCmd *cmd, const char *name, long long *value) { @@ -8276,7 +8292,7 @@ vshMemtuneGetSize(const vshCmd *cmd, const char *name, long long *value) if (virScaleInteger(&tmp, end, 1024, LLONG_MAX) < 0) return -1; *value = VIR_DIV_UP(tmp, 1024); - return 0; + return 1; }
static bool @@ -8294,6 +8310,7 @@ cmdMemtune(vshControl *ctl, const vshCmd *cmd) bool current = vshCommandOptBool(cmd, "current"); bool config = vshCommandOptBool(cmd, "config"); bool live = vshCommandOptBool(cmd, "live"); + int rc;
VSH_EXCLUSIVE_OPTIONS_VAR(current, live); VSH_EXCLUSIVE_OPTIONS_VAR(current, config); @@ -8306,50 +8323,26 @@ cmdMemtune(vshControl *ctl, const vshCmd *cmd) if (!(dom = vshCommandOptDomain(ctl, cmd, NULL))) return false;
- if (vshMemtuneGetSize(cmd, "hard-limit", &hard_limit) < 0 || - vshMemtuneGetSize(cmd, "soft-limit", &soft_limit) < 0 || - vshMemtuneGetSize(cmd, "swap-hard-limit", &swap_hard_limit) < 0 || - vshMemtuneGetSize(cmd, "min-guarantee", &min_guarantee) < 0) { - vshError(ctl, "%s", - _("Unable to parse integer parameter")); - goto cleanup; - } - - if (hard_limit) { - if (hard_limit == -1) - hard_limit = VIR_DOMAIN_MEMORY_PARAM_UNLIMITED; - if (virTypedParamsAddULLong(¶ms, &nparams, &maxparams, - VIR_DOMAIN_MEMORY_HARD_LIMIT, - hard_limit) < 0) - goto save_error; - } - - if (soft_limit) { - if (soft_limit == -1) - soft_limit = VIR_DOMAIN_MEMORY_PARAM_UNLIMITED; - if (virTypedParamsAddULLong(¶ms, &nparams, &maxparams, - VIR_DOMAIN_MEMORY_SOFT_LIMIT, - soft_limit) < 0) - goto save_error; - } - - if (swap_hard_limit) { - if (swap_hard_limit == -1) - swap_hard_limit = VIR_DOMAIN_MEMORY_PARAM_UNLIMITED; - if (virTypedParamsAddULLong(¶ms, &nparams, &maxparams, - VIR_DOMAIN_MEMORY_SWAP_HARD_LIMIT, - swap_hard_limit) < 0) - goto save_error; - } - - if (min_guarantee) { - if (min_guarantee == -1) - min_guarantee = VIR_DOMAIN_MEMORY_PARAM_UNLIMITED; - if (virTypedParamsAddULLong(¶ms, &nparams, &maxparams, - VIR_DOMAIN_MEMORY_MIN_GUARANTEE, - min_guarantee) < 0) - goto save_error; - } +#define PARSE_MEMTUNE_PARAM(NAME, VALUE, FIELD) \ + if ((rc = vshMemtuneGetSize(cmd, NAME, &VALUE)) < 0) { \ + vshError(ctl, "%s", _("Unable to parse integer parameter 'NAME'")); \ + goto cleanup; \ + } \ + if (rc == 1) { \ + if (virTypedParamsAddULLong(¶ms, &nparams, &maxparams, \ + FIELD, VALUE) < 0) \ + goto save_error; \ + } \ + + + PARSE_MEMTUNE_PARAM("hard-limit", hard_limit, VIR_DOMAIN_MEMORY_HARD_LIMIT); + PARSE_MEMTUNE_PARAM("soft-limit", soft_limit, VIR_DOMAIN_MEMORY_SOFT_LIMIT);
I just realized that the 'hard_limit', 'soft_limit' ... variables are used just as temp variables in the macro. You could get rid of them and use a single temp variable instead.
+ PARSE_MEMTUNE_PARAM("swap-hard-limit", swap_hard_limit, + VIR_DOMAIN_MEMORY_SWAP_HARD_LIMIT);
Which would make these fit on one line.
+ PARSE_MEMTUNE_PARAM("min-guarantee", min_guarantee, + VIR_DOMAIN_MEMORY_MIN_GUARANTEE); + +#undef PARSE_MEMTUNE_PARAM
if (nparams == 0) { /* get the number of memory parameters */
ACK if you drop the parenthesed section in the comment as pointed above. The rest is not mandatory, although I'd prefer if the patch had a proper commit message.
Peter
Thanks, I've also updated the macro to use tmpVal and pushed it. Pavel
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list

The first one is to truncate the memory limit to VIR_DOMAIN_MEMORY_PARAM_UNLIMITED if the value is greater and the second one is to decide whether the memory limit is set or not, unlimited means that it's not set. Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/libvirt_private.syms | 2 ++ src/util/virutil.c | 24 ++++++++++++++++++++++++ src/util/virutil.h | 3 +++ 3 files changed, 29 insertions(+) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index c810cf7..66d2333 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2269,6 +2269,8 @@ virIsCapableVport; virIsDevMapperDevice; virIsSUID; virManageVport; +virMemoryLimitIsSet; +virMemoryLimitTruncate; virParseNumber; virParseOwnershipIds; virParseVersionString; diff --git a/src/util/virutil.c b/src/util/virutil.c index 4a95292..599d59c 100644 --- a/src/util/virutil.c +++ b/src/util/virutil.c @@ -2598,3 +2598,27 @@ long virGetSystemPageSizeKB(void) return val; return val / 1024; } + +/** + * virMemoryLimitTruncate + * + * Return truncated memory limit to VIR_DOMAIN_MEMORY_PARAM_UNLIMITED as maximum + * which means that the limit is not set => unlimited. + */ +unsigned long long +virMemoryLimitTruncate(unsigned long long value) +{ + return value < VIR_DOMAIN_MEMORY_PARAM_UNLIMITED ? value : + VIR_DOMAIN_MEMORY_PARAM_UNLIMITED; +} + +/** + * virMemoryLimitIsSet + * + * Returns true if the limit is set and false for unlimited value. + */ +bool +virMemoryLimitIsSet(unsigned long long value) +{ + return value < VIR_DOMAIN_MEMORY_PARAM_UNLIMITED; +} diff --git a/src/util/virutil.h b/src/util/virutil.h index d1173c1..b8f5036 100644 --- a/src/util/virutil.h +++ b/src/util/virutil.h @@ -247,4 +247,7 @@ unsigned int virGetListenFDs(void); long virGetSystemPageSize(void); long virGetSystemPageSizeKB(void); +unsigned long long virMemoryLimitTruncate(unsigned long long value); +bool virMemoryLimitIsSet(unsigned long long value); + #endif /* __VIR_UTIL_H__ */ -- 2.0.5

On Thu, Mar 05, 2015 at 16:10:29 +0100, Pavel Hrdina wrote:
The first one is to truncate the memory limit to VIR_DOMAIN_MEMORY_PARAM_UNLIMITED if the value is greater and the second one is to decide whether the memory limit is set or not, unlimited means that it's not set.
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/libvirt_private.syms | 2 ++ src/util/virutil.c | 24 ++++++++++++++++++++++++ src/util/virutil.h | 3 +++ 3 files changed, 29 insertions(+)
ACK, Peter

There was a mess in the way how we store unlimited value for memory limits and how we handled values provided by user. Internally there were two possible ways how to store unlimited value: as 0 value or as VIR_DOMAIN_MEMORY_PARAM_UNLIMITED. Because we chose to store memory limits as unsigned long long, we cannot use -1 to represent unlimited. It's much easier for us to say that everything greater than VIR_DOMAIN_MEMORY_PARAM_UNLIMITED means unlimited and leave 0 as valid value despite that it makes no sense to set limit to 0. Remove unnecessary function virCompareLimitUlong. The update of test is to prevent the 0 to be miss-used as unlimited in future. Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1146539 Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- docs/formatdomain.html.in | 4 +- src/conf/domain_conf.c | 75 +++++++++++++++++----- src/libvirt-domain.c | 3 + src/libvirt_private.syms | 1 - src/lxc/lxc_cgroup.c | 18 +++--- src/lxc/lxc_driver.c | 7 +- src/lxc/lxc_fuse.c | 12 ++-- src/lxc/lxc_native.c | 6 +- src/openvz/openvz_conf.c | 4 +- src/openvz/openvz_driver.c | 4 +- src/qemu/qemu_cgroup.c | 24 +++---- src/qemu/qemu_command.c | 8 ++- src/qemu/qemu_driver.c | 10 +-- src/qemu/qemu_hotplug.c | 2 +- src/qemu/qemu_migration.c | 5 +- src/util/virutil.c | 23 ------- src/util/virutil.h | 2 - tests/qemuxml2argvdata/qemuxml2argv-memtune.xml | 2 +- .../qemuxml2xmloutdata/qemuxml2xmlout-memtune.xml | 2 +- 19 files changed, 118 insertions(+), 94 deletions(-) diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index 6276a61..335763f 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -798,7 +798,9 @@ for <code><memory></code>. For backwards compatibility, output is always in KiB. <span class='since'><code>unit</code> - since 0.9.11</span></dd> + since 0.9.11</span> + Possible values for all *_limit parameters are in range from 0 to + VIR_DOMAIN_MEMORY_PARAM_UNLIMITED.</dd> <dt><code>hard_limit</code></dt> <dd> The optional <code>hard_limit</code> element is the maximum memory the guest can use. The units for this value are kibibytes (i.e. blocks diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 6da02b0..efa142c 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -2320,6 +2320,10 @@ virDomainDefNew(void) if (!(ret->numa = virDomainNumaNew())) goto error; + ret->mem.hard_limit = VIR_DOMAIN_MEMORY_PARAM_UNLIMITED; + ret->mem.soft_limit = VIR_DOMAIN_MEMORY_PARAM_UNLIMITED; + ret->mem.swap_hard_limit = VIR_DOMAIN_MEMORY_PARAM_UNLIMITED; + return ret; error: @@ -6884,6 +6888,50 @@ virDomainParseMemory(const char *xpath, } +/** + * virDomainParseMemoryLimit: + * + * @xpath: XPath to memory amount + * @units_xpath: XPath to units attribute + * @ctxt: XPath context + * @mem: scaled memory amount is stored here + * + * Parse a memory element or attribute located at @xpath within @ctxt, and + * store the result into @mem, in blocks of 1024. The value is scaled by + * units located at @units_xpath (or the 'unit' attribute under @xpath if + * @units_xpath is NULL). If units are not present, he default scale of 1024 + * is used. The value must not exceed VIR_DOMAIN_MEMORY_PARAM_UNLIMITED + * once scaled. + * + * This helper should be used only on *_limit memory elements. + * + * Return 0 on success, -1 on failure after issuing error. + */ +static int +virDomainParseMemoryLimit(const char *xpath, + const char *units_xpath, + xmlXPathContextPtr ctxt, + unsigned long long *mem) +{ + int ret; + unsigned long long bytes; + + ret = virDomainParseScaledValue(xpath, units_xpath, ctxt, &bytes, 1024, + VIR_DOMAIN_MEMORY_PARAM_UNLIMITED << 10, + false); + + if (ret < 0) + return -1; + + if (ret == 0) + *mem = VIR_DOMAIN_MEMORY_PARAM_UNLIMITED; + else + *mem = virMemoryLimitTruncate(VIR_DIV_UP(bytes, 1024)); + + return 0; +} + + static int virDomainControllerModelTypeFromString(const virDomainControllerDef *def, const char *model) @@ -13185,20 +13233,20 @@ virDomainDefParseXML(xmlDocPtr xml, VIR_FREE(nodes); /* Extract other memory tunables */ - if (virDomainParseMemory("./memtune/hard_limit[1]", NULL, ctxt, - &def->mem.hard_limit, false, false) < 0) + if (virDomainParseMemoryLimit("./memtune/hard_limit[1]", NULL, ctxt, + &def->mem.hard_limit) < 0) goto error; - if (virDomainParseMemory("./memtune/soft_limit[1]", NULL, ctxt, - &def->mem.soft_limit, false, false) < 0) + if (virDomainParseMemoryLimit("./memtune/soft_limit[1]", NULL, ctxt, + &def->mem.soft_limit) < 0) goto error; if (virDomainParseMemory("./memtune/min_guarantee[1]", NULL, ctxt, &def->mem.min_guarantee, false, false) < 0) goto error; - if (virDomainParseMemory("./memtune/swap_hard_limit[1]", NULL, ctxt, - &def->mem.swap_hard_limit, false, false) < 0) + if (virDomainParseMemoryLimit("./memtune/swap_hard_limit[1]", NULL, ctxt, + &def->mem.swap_hard_limit) < 0) goto error; n = virXPathULong("string(./vcpu[1])", ctxt, &count); @@ -19767,20 +19815,17 @@ virDomainDefFormatInternal(virDomainDefPtr def, } /* add memtune only if there are any */ - if ((def->mem.hard_limit && - def->mem.hard_limit != VIR_DOMAIN_MEMORY_PARAM_UNLIMITED) || - (def->mem.soft_limit && - def->mem.soft_limit != VIR_DOMAIN_MEMORY_PARAM_UNLIMITED) || - (def->mem.swap_hard_limit && - def->mem.swap_hard_limit != VIR_DOMAIN_MEMORY_PARAM_UNLIMITED) || + if (virMemoryLimitIsSet(def->mem.hard_limit) || + virMemoryLimitIsSet(def->mem.soft_limit) || + virMemoryLimitIsSet(def->mem.swap_hard_limit) || def->mem.min_guarantee) { virBufferAddLit(buf, "<memtune>\n"); virBufferAdjustIndent(buf, 2); - if (def->mem.hard_limit) { + if (virMemoryLimitIsSet(def->mem.hard_limit)) { virBufferAsprintf(buf, "<hard_limit unit='KiB'>" "%llu</hard_limit>\n", def->mem.hard_limit); } - if (def->mem.soft_limit) { + if (virMemoryLimitIsSet(def->mem.soft_limit)) { virBufferAsprintf(buf, "<soft_limit unit='KiB'>" "%llu</soft_limit>\n", def->mem.soft_limit); } @@ -19788,7 +19833,7 @@ virDomainDefFormatInternal(virDomainDefPtr def, virBufferAsprintf(buf, "<min_guarantee unit='KiB'>" "%llu</min_guarantee>\n", def->mem.min_guarantee); } - if (def->mem.swap_hard_limit) { + if (virMemoryLimitIsSet(def->mem.swap_hard_limit)) { virBufferAsprintf(buf, "<swap_hard_limit unit='KiB'>" "%llu</swap_hard_limit>\n", def->mem.swap_hard_limit); } diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c index 89d1eab..8345397 100644 --- a/src/libvirt-domain.c +++ b/src/libvirt-domain.c @@ -2075,6 +2075,9 @@ virDomainSetMemoryStatsPeriod(virDomainPtr domain, int period, * Change all or a subset of the memory tunables. * This function may require privileged access to the hypervisor. * + * Possible values for all *_limit memory tunables are in range from 0 to + * VIR_DOMAIN_MEMORY_PARAM_UNLIMITED. + * * Returns -1 in case of error, 0 in case of success. */ int diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 66d2333..8b55601 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2232,7 +2232,6 @@ virUSBDeviceSetUsedBy; # util/virutil.h -virCompareLimitUlong; virDoubleToStr; virEnumFromString; virEnumToString; diff --git a/src/lxc/lxc_cgroup.c b/src/lxc/lxc_cgroup.c index 8e46a01..5a49e2d 100644 --- a/src/lxc/lxc_cgroup.c +++ b/src/lxc/lxc_cgroup.c @@ -149,17 +149,17 @@ static int virLXCCgroupSetupMemTune(virDomainDefPtr def, if (virCgroupSetMemory(cgroup, def->mem.max_balloon) < 0) goto cleanup; - if (def->mem.hard_limit && - virCgroupSetMemoryHardLimit(cgroup, def->mem.hard_limit) < 0) - goto cleanup; + if (virMemoryLimitIsSet(def->mem.hard_limit)) + if (virCgroupSetMemoryHardLimit(cgroup, def->mem.hard_limit) < 0) + goto cleanup; - if (def->mem.soft_limit && - virCgroupSetMemorySoftLimit(cgroup, def->mem.soft_limit) < 0) - goto cleanup; + if (virMemoryLimitIsSet(def->mem.soft_limit)) + if (virCgroupSetMemorySoftLimit(cgroup, def->mem.soft_limit) < 0) + goto cleanup; - if (def->mem.swap_hard_limit && - virCgroupSetMemSwapHardLimit(cgroup, def->mem.swap_hard_limit) < 0) - goto cleanup; + if (virMemoryLimitIsSet(def->mem.swap_hard_limit)) + if (virCgroupSetMemSwapHardLimit(cgroup, def->mem.swap_hard_limit) < 0) + goto cleanup; ret = 0; cleanup: diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c index 3a28dd5..6b0dea1 100644 --- a/src/lxc/lxc_driver.c +++ b/src/lxc/lxc_driver.c @@ -874,7 +874,7 @@ lxcDomainSetMemoryParameters(virDomainPtr dom, if (set_hard_limit) mem_limit = hard_limit; - if (virCompareLimitUlong(mem_limit, swap_limit) > 0) { + if (mem_limit > swap_limit) { virReportError(VIR_ERR_INVALID_ARG, "%s", _("memory hard_limit tunable value must be lower " "than or equal to swap_hard_limit")); @@ -902,7 +902,7 @@ lxcDomainSetMemoryParameters(virDomainPtr dom, LXC_SET_MEM_PARAMETER(virCgroupSetMemorySoftLimit, soft_limit); /* set hard limit before swap hard limit if decreasing it */ - if (virCompareLimitUlong(vm->def->mem.hard_limit, hard_limit) > 0) { + if (vm->def->mem.hard_limit > hard_limit) { LXC_SET_MEM_PARAMETER(virCgroupSetMemoryHardLimit, hard_limit); /* inhibit changing the limit a second time */ set_hard_limit = false; @@ -983,7 +983,6 @@ lxcDomainGetMemoryParameters(virDomainPtr dom, case 0: /* fill memory hard limit here */ if (flags & VIR_DOMAIN_AFFECT_CONFIG) { val = vmdef->mem.hard_limit; - val = val ? val : VIR_DOMAIN_MEMORY_PARAM_UNLIMITED; } else if (virCgroupGetMemoryHardLimit(priv->cgroup, &val) < 0) { goto cleanup; } @@ -994,7 +993,6 @@ lxcDomainGetMemoryParameters(virDomainPtr dom, case 1: /* fill memory soft limit here */ if (flags & VIR_DOMAIN_AFFECT_CONFIG) { val = vmdef->mem.soft_limit; - val = val ? val : VIR_DOMAIN_MEMORY_PARAM_UNLIMITED; } else if (virCgroupGetMemorySoftLimit(priv->cgroup, &val) < 0) { goto cleanup; } @@ -1005,7 +1003,6 @@ lxcDomainGetMemoryParameters(virDomainPtr dom, case 2: /* fill swap hard limit here */ if (flags & VIR_DOMAIN_AFFECT_CONFIG) { val = vmdef->mem.swap_hard_limit; - val = val ? val : VIR_DOMAIN_MEMORY_PARAM_UNLIMITED; } else if (virCgroupGetMemSwapHardLimit(priv->cgroup, &val) < 0) { goto cleanup; } diff --git a/src/lxc/lxc_fuse.c b/src/lxc/lxc_fuse.c index 5c18cff..bc2b92c 100644 --- a/src/lxc/lxc_fuse.c +++ b/src/lxc/lxc_fuse.c @@ -165,11 +165,13 @@ static int lxcProcReadMeminfo(char *hostpath, virDomainDefPtr def, *ptr = '\0'; if (STREQ(line, "MemTotal") && - (def->mem.hard_limit || def->mem.max_balloon)) { + (virMemoryLimitIsSet(def->mem.hard_limit) || + def->mem.max_balloon)) { virBufferAsprintf(new_meminfo, "MemTotal: %8llu kB\n", meminfo.memtotal); } else if (STREQ(line, "MemFree") && - (def->mem.hard_limit || def->mem.max_balloon)) { + (virMemoryLimitIsSet(def->mem.hard_limit) || + def->mem.max_balloon)) { virBufferAsprintf(new_meminfo, "MemFree: %8llu kB\n", (meminfo.memtotal - meminfo.memusage)); } else if (STREQ(line, "Buffers")) { @@ -198,10 +200,12 @@ static int lxcProcReadMeminfo(char *hostpath, virDomainDefPtr def, } else if (STREQ(line, "Unevictable")) { virBufferAsprintf(new_meminfo, "Unevictable: %8llu kB\n", meminfo.unevictable); - } else if (STREQ(line, "SwapTotal") && def->mem.swap_hard_limit) { + } else if (STREQ(line, "SwapTotal") && + virMemoryLimitIsSet(def->mem.swap_hard_limit)) { virBufferAsprintf(new_meminfo, "SwapTotal: %8llu kB\n", (meminfo.swaptotal - meminfo.memtotal)); - } else if (STREQ(line, "SwapFree") && def->mem.swap_hard_limit) { + } else if (STREQ(line, "SwapFree") && + virMemoryLimitIsSet(def->mem.swap_hard_limit)) { virBufferAsprintf(new_meminfo, "SwapFree: %8llu kB\n", (meminfo.swaptotal - meminfo.memtotal - meminfo.swapusage + meminfo.memusage)); diff --git a/src/lxc/lxc_native.c b/src/lxc/lxc_native.c index abf07ce..2ebe610 100644 --- a/src/lxc/lxc_native.c +++ b/src/lxc/lxc_native.c @@ -773,7 +773,7 @@ lxcSetMemTune(virDomainDefPtr def, virConfPtr properties) return -1; size = size / 1024; def->mem.max_balloon = size; - def->mem.hard_limit = size; + def->mem.hard_limit = virMemoryLimitTruncate(size); } if ((value = virConfGetValue(properties, @@ -782,7 +782,7 @@ lxcSetMemTune(virDomainDefPtr def, virConfPtr properties) if (lxcConvertSize(value->str, &size) < 0) return -1; - def->mem.soft_limit = size / 1024; + def->mem.soft_limit = virMemoryLimitTruncate(size / 1024); } if ((value = virConfGetValue(properties, @@ -791,7 +791,7 @@ lxcSetMemTune(virDomainDefPtr def, virConfPtr properties) if (lxcConvertSize(value->str, &size) < 0) return -1; - def->mem.swap_hard_limit = size / 1024; + def->mem.swap_hard_limit = virMemoryLimitTruncate(size / 1024); } return 0; } diff --git a/src/openvz/openvz_conf.c b/src/openvz/openvz_conf.c index 848e230..bfa2141 100644 --- a/src/openvz/openvz_conf.c +++ b/src/openvz/openvz_conf.c @@ -479,12 +479,12 @@ openvzReadMemConf(virDomainDefPtr def, int veid) goto error; } if (barrier == LONG_MAX) - def->mem.soft_limit = 0ull; + def->mem.soft_limit = VIR_DOMAIN_MEMORY_PARAM_UNLIMITED; else def->mem.soft_limit = barrier * kb_per_pages; if (limit == LONG_MAX) - def->mem.hard_limit = 0ull; + def->mem.hard_limit = VIR_DOMAIN_MEMORY_PARAM_UNLIMITED; else def->mem.hard_limit = limit * kb_per_pages; } diff --git a/src/openvz/openvz_driver.c b/src/openvz/openvz_driver.c index 5a5cd8d..a55e6a6 100644 --- a/src/openvz/openvz_driver.c +++ b/src/openvz/openvz_driver.c @@ -1850,7 +1850,7 @@ openvzDomainGetMemoryParameters(virDomainPtr domain, if (openvzDomainGetBarrierLimit(domain, name, &barrier, &limit) < 0) goto cleanup; - val = (limit == LONG_MAX) ? 0ull : limit * kb_per_pages; + val = (limit == LONG_MAX) ? VIR_DOMAIN_MEMORY_PARAM_UNLIMITED : limit * kb_per_pages; if (virTypedParameterAssign(param, VIR_DOMAIN_MEMORY_HARD_LIMIT, VIR_TYPED_PARAM_ULLONG, val) < 0) goto cleanup; @@ -1861,7 +1861,7 @@ openvzDomainGetMemoryParameters(virDomainPtr domain, if (openvzDomainGetBarrierLimit(domain, name, &barrier, &limit) < 0) goto cleanup; - val = (barrier == LONG_MAX) ? 0ull : barrier * kb_per_pages; + val = (barrier == LONG_MAX) ? VIR_DOMAIN_MEMORY_PARAM_UNLIMITED : barrier * kb_per_pages; if (virTypedParameterAssign(param, VIR_DOMAIN_MEMORY_SOFT_LIMIT, VIR_TYPED_PARAM_ULLONG, val) < 0) goto cleanup; diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c index f8f0e3c..2b5a182 100644 --- a/src/qemu/qemu_cgroup.c +++ b/src/qemu/qemu_cgroup.c @@ -471,9 +471,9 @@ qemuSetupMemoryCgroup(virDomainObjPtr vm) qemuDomainObjPrivatePtr priv = vm->privateData; if (!virCgroupHasController(priv->cgroup, VIR_CGROUP_CONTROLLER_MEMORY)) { - if (vm->def->mem.hard_limit != 0 || - vm->def->mem.soft_limit != 0 || - vm->def->mem.swap_hard_limit != 0) { + if (virMemoryLimitIsSet(vm->def->mem.hard_limit) || + virMemoryLimitIsSet(vm->def->mem.soft_limit) || + virMemoryLimitIsSet(vm->def->mem.swap_hard_limit)) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("Memory cgroup is not available on this host")); return -1; @@ -482,17 +482,17 @@ qemuSetupMemoryCgroup(virDomainObjPtr vm) } } - if (vm->def->mem.hard_limit != 0 && - virCgroupSetMemoryHardLimit(priv->cgroup, vm->def->mem.hard_limit) < 0) - return -1; + if (virMemoryLimitIsSet(vm->def->mem.hard_limit)) + if (virCgroupSetMemoryHardLimit(priv->cgroup, vm->def->mem.hard_limit) < 0) + return -1; - if (vm->def->mem.soft_limit != 0 && - virCgroupSetMemorySoftLimit(priv->cgroup, vm->def->mem.soft_limit) < 0) - return -1; + if (virMemoryLimitIsSet(vm->def->mem.soft_limit)) + if (virCgroupSetMemorySoftLimit(priv->cgroup, vm->def->mem.soft_limit) < 0) + return -1; - if (vm->def->mem.swap_hard_limit != 0 && - virCgroupSetMemSwapHardLimit(priv->cgroup, vm->def->mem.swap_hard_limit) < 0) - return -1; + if (virMemoryLimitIsSet(vm->def->mem.swap_hard_limit)) + if (virCgroupSetMemSwapHardLimit(priv->cgroup, vm->def->mem.swap_hard_limit) < 0) + return -1; return 0; } diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index a5a1f11..841b4d6 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -8272,8 +8272,10 @@ qemuBuildCommandLine(virConnectPtr conn, /* If we have no cgroups then we can have no tunings that * require them */ - if (def->mem.hard_limit || def->mem.soft_limit || - def->mem.min_guarantee || def->mem.swap_hard_limit) { + if (virMemoryLimitIsSet(def->mem.hard_limit) || + virMemoryLimitIsSet(def->mem.soft_limit) || + def->mem.min_guarantee || + virMemoryLimitIsSet(def->mem.swap_hard_limit)) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("Memory tuning is not available in session mode")); goto error; @@ -10357,7 +10359,7 @@ qemuBuildCommandLine(virConnectPtr conn, * space just to be safe (some finer tuning might be * nice, though). */ - memKB = def->mem.hard_limit ? + memKB = virMemoryLimitIsSet(def->mem.hard_limit) ? def->mem.hard_limit : def->mem.max_balloon + 1024 * 1024; virCommandSetMaxMemLock(cmd, memKB * 1024); } diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index ffa4e19..a79bc5e 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -9143,8 +9143,7 @@ qemuDomainSetMemoryParameters(virDomainPtr dom, #undef VIR_GET_LIMIT_PARAMETER - /* Swap hard limit must be greater than hard limit. - * Note that limit of 0 denotes unlimited */ + /* Swap hard limit must be greater than hard limit. */ if (set_swap_hard_limit || set_hard_limit) { unsigned long long mem_limit = vm->def->mem.hard_limit; unsigned long long swap_limit = vm->def->mem.swap_hard_limit; @@ -9155,7 +9154,7 @@ qemuDomainSetMemoryParameters(virDomainPtr dom, if (set_hard_limit) mem_limit = hard_limit; - if (virCompareLimitUlong(mem_limit, swap_limit) > 0) { + if (mem_limit > swap_limit) { virReportError(VIR_ERR_INVALID_ARG, "%s", _("memory hard_limit tunable value must be lower " "than or equal to swap_hard_limit")); @@ -9183,7 +9182,7 @@ qemuDomainSetMemoryParameters(virDomainPtr dom, QEMU_SET_MEM_PARAMETER(virCgroupSetMemorySoftLimit, soft_limit); /* set hard limit before swap hard limit if decreasing it */ - if (virCompareLimitUlong(vm->def->mem.hard_limit, hard_limit) > 0) { + if (vm->def->mem.hard_limit > hard_limit) { QEMU_SET_MEM_PARAMETER(virCgroupSetMemoryHardLimit, hard_limit); /* inhibit changing the limit a second time */ set_hard_limit = false; @@ -9279,7 +9278,6 @@ qemuDomainGetMemoryParameters(virDomainPtr dom, switch (i) { case 0: /* fill memory hard limit here */ value = persistentDef->mem.hard_limit; - value = value ? value : VIR_DOMAIN_MEMORY_PARAM_UNLIMITED; if (virTypedParameterAssign(param, VIR_DOMAIN_MEMORY_HARD_LIMIT, VIR_TYPED_PARAM_ULLONG, value) < 0) goto cleanup; @@ -9287,7 +9285,6 @@ qemuDomainGetMemoryParameters(virDomainPtr dom, case 1: /* fill memory soft limit here */ value = persistentDef->mem.soft_limit; - value = value ? value : VIR_DOMAIN_MEMORY_PARAM_UNLIMITED; if (virTypedParameterAssign(param, VIR_DOMAIN_MEMORY_SOFT_LIMIT, VIR_TYPED_PARAM_ULLONG, value) < 0) goto cleanup; @@ -9295,7 +9292,6 @@ qemuDomainGetMemoryParameters(virDomainPtr dom, case 2: /* fill swap hard limit here */ value = persistentDef->mem.swap_hard_limit; - value = value ? value : VIR_DOMAIN_MEMORY_PARAM_UNLIMITED; if (virTypedParameterAssign(param, VIR_DOMAIN_MEMORY_SWAP_HARD_LIMIT, VIR_TYPED_PARAM_ULLONG, value) < 0) goto cleanup; diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 08047ce..6ad48f7 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -1250,7 +1250,7 @@ qemuDomainAttachHostPCIDevice(virQEMUDriverPtr driver, * Kibibytes, but virProcessSetMaxMemLock expects the value in * bytes. */ - memKB = vm->def->mem.hard_limit + memKB = virMemoryLimitIsSet(vm->def->mem.hard_limit) ? vm->def->mem.hard_limit : vm->def->mem.max_balloon + (1024 * 1024); virProcessSetMaxMemLock(vm->pid, memKB * 1024); diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index 20e40aa..782c13c 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -2986,7 +2986,8 @@ qemuMigrationPrepareAny(virQEMUDriverPtr driver, QEMU_MIGRATION_COOKIE_NBD))) goto cleanup; - if (STREQ_NULLABLE(protocol, "rdma") && !vm->def->mem.hard_limit) { + if (STREQ_NULLABLE(protocol, "rdma") && + virMemoryLimitIsSet(vm->def->mem.hard_limit)) { virReportError(VIR_ERR_OPERATION_INVALID, "%s", _("cannot start RDMA migration with no memory hard " "limit set")); @@ -4102,7 +4103,7 @@ static int doNativeMigrate(virQEMUDriverPtr driver, "with this QEMU binary")); goto cleanup; } - if (!vm->def->mem.hard_limit) { + if (virMemoryLimitIsSet(vm->def->mem.hard_limit)) { virReportError(VIR_ERR_OPERATION_INVALID, "%s", _("cannot start RDMA migration with no memory hard " "limit set")); diff --git a/src/util/virutil.c b/src/util/virutil.c index 599d59c..658723b 100644 --- a/src/util/virutil.c +++ b/src/util/virutil.c @@ -2365,29 +2365,6 @@ virFindFCHostCapableVport(const char *sysfs_prefix ATTRIBUTE_UNUSED) #endif /* __linux__ */ /** - * virCompareLimitUlong: - * - * Compare two unsigned long long numbers. Value '0' of the arguments has a - * special meaning of 'unlimited' and thus greater than any other value. - * - * Returns 0 if the numbers are equal, -1 if b is greater, 1 if a is greater. - */ -int -virCompareLimitUlong(unsigned long long a, unsigned long long b) -{ - if (a == b) - return 0; - - if (!b) - return -1; - - if (a == 0 || a > b) - return 1; - - return -1; -} - -/** * virParseOwnershipIds: * * Parse the usual "uid:gid" ownership specification into uid_t and diff --git a/src/util/virutil.h b/src/util/virutil.h index b8f5036..25524e2 100644 --- a/src/util/virutil.h +++ b/src/util/virutil.h @@ -210,8 +210,6 @@ char *virGetFCHostNameByWWN(const char *sysfs_prefix, char *virFindFCHostCapableVport(const char *sysfs_prefix); -int virCompareLimitUlong(unsigned long long a, unsigned long long b); - int virParseOwnershipIds(const char *label, uid_t *uidPtr, gid_t *gidPtr); const char *virGetEnvBlockSUID(const char *name); diff --git a/tests/qemuxml2argvdata/qemuxml2argv-memtune.xml b/tests/qemuxml2argvdata/qemuxml2argv-memtune.xml index 1a244f0..f838d95 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-memtune.xml +++ b/tests/qemuxml2argvdata/qemuxml2argv-memtune.xml @@ -5,7 +5,7 @@ <currentMemory unit='KiB'>219136</currentMemory> <memtune> <hard_limit unit='KiB'>512000</hard_limit> - <soft_limit unit='bytes'>131071999</soft_limit> + <soft_limit unit='bytes'>0</soft_limit> <swap_hard_limit unit='KB'>1048576</swap_hard_limit> </memtune> <vcpu placement='static'>1</vcpu> diff --git a/tests/qemuxml2xmloutdata/qemuxml2xmlout-memtune.xml b/tests/qemuxml2xmloutdata/qemuxml2xmlout-memtune.xml index 92dcacf..07989d1 100644 --- a/tests/qemuxml2xmloutdata/qemuxml2xmlout-memtune.xml +++ b/tests/qemuxml2xmloutdata/qemuxml2xmlout-memtune.xml @@ -5,7 +5,7 @@ <currentMemory unit='KiB'>219136</currentMemory> <memtune> <hard_limit unit='KiB'>512000</hard_limit> - <soft_limit unit='KiB'>128000</soft_limit> + <soft_limit unit='KiB'>0</soft_limit> <swap_hard_limit unit='KiB'>1024000</swap_hard_limit> </memtune> <vcpu placement='static'>1</vcpu> -- 2.0.5

On Thu, Mar 05, 2015 at 16:10:30 +0100, Pavel Hrdina wrote:
There was a mess in the way how we store unlimited value for memory limits and how we handled values provided by user. Internally there were two possible ways how to store unlimited value: as 0 value or as VIR_DOMAIN_MEMORY_PARAM_UNLIMITED. Because we chose to store memory limits as unsigned long long, we cannot use -1 to represent unlimited. It's much easier for us to say that everything greater than VIR_DOMAIN_MEMORY_PARAM_UNLIMITED means unlimited and leave 0 as valid value despite that it makes no sense to set limit to 0.
Remove unnecessary function virCompareLimitUlong. The update of test is to prevent the 0 to be miss-used as unlimited in future.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1146539
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- docs/formatdomain.html.in | 4 +- src/conf/domain_conf.c | 75 +++++++++++++++++----- src/libvirt-domain.c | 3 + src/libvirt_private.syms | 1 - src/lxc/lxc_cgroup.c | 18 +++--- src/lxc/lxc_driver.c | 7 +- src/lxc/lxc_fuse.c | 12 ++-- src/lxc/lxc_native.c | 6 +- src/openvz/openvz_conf.c | 4 +- src/openvz/openvz_driver.c | 4 +- src/qemu/qemu_cgroup.c | 24 +++---- src/qemu/qemu_command.c | 8 ++- src/qemu/qemu_driver.c | 10 +-- src/qemu/qemu_hotplug.c | 2 +- src/qemu/qemu_migration.c | 5 +- src/util/virutil.c | 23 ------- src/util/virutil.h | 2 - tests/qemuxml2argvdata/qemuxml2argv-memtune.xml | 2 +- .../qemuxml2xmloutdata/qemuxml2xmlout-memtune.xml | 2 +- 19 files changed, 118 insertions(+), 94 deletions(-)
...
diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index 20e40aa..782c13c 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -2986,7 +2986,8 @@ qemuMigrationPrepareAny(virQEMUDriverPtr driver, QEMU_MIGRATION_COOKIE_NBD))) goto cleanup;
- if (STREQ_NULLABLE(protocol, "rdma") && !vm->def->mem.hard_limit) { + if (STREQ_NULLABLE(protocol, "rdma") && + virMemoryLimitIsSet(vm->def->mem.hard_limit)) {
You negated the condition above when adding the helper ...
virReportError(VIR_ERR_OPERATION_INVALID, "%s", _("cannot start RDMA migration with no memory hard " "limit set")); @@ -4102,7 +4103,7 @@ static int doNativeMigrate(virQEMUDriverPtr driver, "with this QEMU binary")); goto cleanup; } - if (!vm->def->mem.hard_limit) { + if (virMemoryLimitIsSet(vm->def->mem.hard_limit)) {
... here too.
virReportError(VIR_ERR_OPERATION_INVALID, "%s", _("cannot start RDMA migration with no memory hard " "limit set"));
ACK if you fix the two conditions above. Peter

On Fri, Mar 06, 2015 at 11:28:15AM +0100, Peter Krempa wrote:
On Thu, Mar 05, 2015 at 16:10:30 +0100, Pavel Hrdina wrote:
There was a mess in the way how we store unlimited value for memory limits and how we handled values provided by user. Internally there were two possible ways how to store unlimited value: as 0 value or as VIR_DOMAIN_MEMORY_PARAM_UNLIMITED. Because we chose to store memory limits as unsigned long long, we cannot use -1 to represent unlimited. It's much easier for us to say that everything greater than VIR_DOMAIN_MEMORY_PARAM_UNLIMITED means unlimited and leave 0 as valid value despite that it makes no sense to set limit to 0.
Remove unnecessary function virCompareLimitUlong. The update of test is to prevent the 0 to be miss-used as unlimited in future.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1146539
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- docs/formatdomain.html.in | 4 +- src/conf/domain_conf.c | 75 +++++++++++++++++----- src/libvirt-domain.c | 3 + src/libvirt_private.syms | 1 - src/lxc/lxc_cgroup.c | 18 +++--- src/lxc/lxc_driver.c | 7 +- src/lxc/lxc_fuse.c | 12 ++-- src/lxc/lxc_native.c | 6 +- src/openvz/openvz_conf.c | 4 +- src/openvz/openvz_driver.c | 4 +- src/qemu/qemu_cgroup.c | 24 +++---- src/qemu/qemu_command.c | 8 ++- src/qemu/qemu_driver.c | 10 +-- src/qemu/qemu_hotplug.c | 2 +- src/qemu/qemu_migration.c | 5 +- src/util/virutil.c | 23 ------- src/util/virutil.h | 2 - tests/qemuxml2argvdata/qemuxml2argv-memtune.xml | 2 +- .../qemuxml2xmloutdata/qemuxml2xmlout-memtune.xml | 2 +- 19 files changed, 118 insertions(+), 94 deletions(-)
...
diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index 20e40aa..782c13c 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -2986,7 +2986,8 @@ qemuMigrationPrepareAny(virQEMUDriverPtr driver, QEMU_MIGRATION_COOKIE_NBD))) goto cleanup;
- if (STREQ_NULLABLE(protocol, "rdma") && !vm->def->mem.hard_limit) { + if (STREQ_NULLABLE(protocol, "rdma") && + virMemoryLimitIsSet(vm->def->mem.hard_limit)) {
You negated the condition above when adding the helper ...
virReportError(VIR_ERR_OPERATION_INVALID, "%s", _("cannot start RDMA migration with no memory hard " "limit set")); @@ -4102,7 +4103,7 @@ static int doNativeMigrate(virQEMUDriverPtr driver, "with this QEMU binary")); goto cleanup; } - if (!vm->def->mem.hard_limit) { + if (virMemoryLimitIsSet(vm->def->mem.hard_limit)) {
... here too.
Nice catch.
virReportError(VIR_ERR_OPERATION_INVALID, "%s", _("cannot start RDMA migration with no memory hard " "limit set"));
ACK if you fix the two conditions above.
Peter
Pushed with the fixed conditions. Pavel
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
participants (2)
-
Pavel Hrdina
-
Peter Krempa