On 07.09.2014 05:30, Hongbin Lu wrote:

The OpenVZ driver reported an error on parsing some OpenVZ config parameters (e.g. diskspace). This issue is due to the driver made two incorrect assumptions about the value of the parameters: 1. Assume paramaeter is just a number (without unit suffix). 2. Assume the number is an integer. Actually, an OpenVZ config parameter may consist of a scalar and a unit, and the scalar is not necessary an integer (e.g. 2.2G). This patch is for fixing this issue. --- src/openvz/openvz_conf.c | 109 +++++++++++++++++++++++++++++++++++++++++----- src/util/virutil.c | 84 ++++++++++++++++++++++++----------- src/util/virutil.h | 3 + 3 files changed, 157 insertions(+), 39 deletions(-)

diff --git a/src/openvz/openvz_conf.c b/src/openvz/openvz_conf.c index 856c9f5..0b163b4 100644 --- a/src/openvz/openvz_conf.c +++ b/src/openvz/openvz_conf.c @@ -51,6 +51,7 @@ #include "virfile.h" #include "vircommand.h" #include "virstring.h" +#include "c-ctype.h"

#define VIR_FROM_THIS VIR_FROM_OPENVZ

@@ -127,6 +128,32 @@ int openvzExtractVersion(struct openvz_driver *driver) }

+/* Split a string, which is a config parameter, into a scalar and a unit. + * Recognized unit: + * - Gigabyte: 'G' or 'g' + * - Megabyte: 'M' or 'm' + * - Kilobyte: 'K' or 'k' + * - Page: 'P' or 'g' */

so 'g' refers both to Gigabyte and Page? Or is it a typo?

+static int +openvzParseParamUnit(char* str, char *unit) +{ + char last; + int len = strlen(str); + + if (len == 0) + return -1; + + last = c_tolower(str[len - 1]); + if (last == 'g' || last == 'm' || last == 'k' || last == 'p') {

What if last == 'x' (or any arbitrary character)? This should throw an error IMO.

+ str[len - 1] = '\0'; + if (unit) + *unit = last; + } + + return 0; +} +

This is basically reimplementation of virStrToDouble() && virScaleDouble(). I'm not against wrapping these two into a driver specific wrapper, but I don't like the reimplementation.

+ /* Parse config values of the form barrier:limit into barrier and limit */ static int openvzParseBarrierLimit(const char* value, @@ -146,7 +173,59 @@ openvzParseBarrierLimit(const char* value, goto error; } else { if (barrier != NULL) { - if (virStrToLong_ull(token, NULL, 10, barrier)) + if (openvzParseParamUnit(token, NULL) < 0) + goto error; + + if (virStrToLong_ull(token, NULL, 10, barrier) < 0) + goto error; + } + } + token = strtok_r(NULL, ":", &saveptr); + if (token == NULL) { + goto error; + } else { + if (limit != NULL) { + if (openvzParseParamUnit(token, NULL) < 0) + goto error; + + if (virStrToLong_ull(token, NULL, 10, limit) < 0) + goto error; + } + } + ret = 0; + error: + VIR_FREE(str); + return ret; +} + + +/* Just like openvzParseBarrierLimit, above, but produce "double" + * barrier and limit. This version also outputs the units of barrier + * and limit, which are bunit and lunit, if they are present.*/ +static int +openvzParseBarrierLimitDouble(const char* value, + double *barrier, + double *limit, + char *bunit, + char *lunit) +{ + char *token; + char *saveptr = NULL; + char *str; + int ret = -1; + + if (VIR_STRDUP(str, value) < 0) + goto error; + + token = strtok_r(str, ":", &saveptr); + if (token == NULL) { + goto error; + } else { + if (barrier != NULL) { + if (openvzParseParamUnit(token, bunit) < 0) + goto error; + + if (virStrToDouble(token, NULL, barrier) < 0) goto error; } } @@ -155,7 +234,10 @@ openvzParseBarrierLimit(const char* value, goto error; } else { if (limit != NULL) { - if (virStrToLong_ull(token, NULL, 10, limit)) + if (openvzParseParamUnit(token, lunit) < 0) + goto error; + + if (virStrToDouble(token, NULL, limit) < 0) goto error; } } @@ -352,7 +434,8 @@ openvzReadFSConf(virDomainDefPtr def, char *veid_str = NULL; char *temp = NULL; const char *param; - unsigned long long barrier, limit; + double barrier, limit; + char bunit, lunit;

ret = openvzReadVPSConfigParam(veid, "OSTEMPLATE", &temp); if (ret < 0) { @@ -396,21 +479,23 @@ openvzReadFSConf(virDomainDefPtr def, param = "DISKSPACE"; ret = openvzReadVPSConfigParam(veid, param, &temp); if (ret > 0) { - if (openvzParseBarrierLimit(temp, &barrier, &limit)) { + bunit = 'k'; /* default unit is kilobyte */ + lunit = 'k'; /* default unit is kilobyte */ + + if (openvzParseBarrierLimitDouble(temp, &barrier, &limit, &bunit, &lunit)) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Could not read '%s' from config for container %d"), param, veid); goto error; } else { - /* Ensure that we can multiply by 1024 without overflowing. */ - if (barrier > ULLONG_MAX / 1024 || - limit > ULLONG_MAX / 1024) { - virReportSystemError(VIR_ERR_OVERFLOW, "%s", - _("Unable to parse quota")); + if (virScaleDouble(&barrier, &bunit) < 0) goto error; - } - fs->space_soft_limit = barrier * 1024; /* unit is bytes */ - fs->space_hard_limit = limit * 1024; /* unit is bytes */ + + if (virScaleDouble(&limit, &lunit) < 0) + goto error; + + fs->space_soft_limit = (unsigned long long)barrier; + fs->space_hard_limit = (unsigned long long)limit; } }

diff --git a/src/util/virutil.c b/src/util/virutil.c index 2edbec5..3b6e617 100644 --- a/src/util/virutil.c +++ b/src/util/virutil.c @@ -268,28 +268,12 @@ virHexToBin(unsigned char c) } }

-/* Scale an integer VALUE in-place by an optional case-insensitive - * SUFFIX, defaulting to SCALE if suffix is NULL or empty (scale is - * typically 1 or 1024). Recognized suffixes include 'b' or 'bytes', - * as well as power-of-two scaling via binary abbreviations ('KiB', - * 'MiB', ...) or their one-letter counterpart ('k', 'M', ...), and - * power-of-ten scaling via SI abbreviations ('KB', 'MB', ...). - * Ensure that the result does not exceed LIMIT. Return 0 on success, - * -1 with error message raised on failure. */ int -virScaleInteger(unsigned long long *value, const char *suffix, - unsigned long long scale, unsigned long long limit) +virSuffixToScale(const char *suffix, unsigned long long *scale) { - if (!suffix || !*suffix) { - if (!scale) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("invalid scale %llu"), scale); - return -1; - } - suffix = ""; - } else if (STRCASEEQ(suffix, "b") || STRCASEEQ(suffix, "byte") || + if (STRCASEEQ(suffix, "b") || STRCASEEQ(suffix, "byte") || STRCASEEQ(suffix, "bytes")) { - scale = 1; + *scale = 1; } else { int base;

@@ -299,28 +283,28 @@ virScaleInteger(unsigned long long *value, const char *suffix, base = 1000; } else { virReportError(VIR_ERR_INVALID_ARG, - _("unknown suffix '%s'"), suffix); + _("unknown suffix '%s'"), suffix); return -1; } - scale = 1; + *scale = 1; switch (c_tolower(*suffix)) { case 'e': - scale *= base; + *scale *= base; /* fallthrough */ case 'p': - scale *= base; + *scale *= base; /* fallthrough */ case 't': - scale *= base; + *scale *= base; /* fallthrough */ case 'g': - scale *= base; + *scale *= base; /* fallthrough */ case 'm': - scale *= base; + *scale *= base; /* fallthrough */ case 'k': - scale *= base; + *scale *= base; break; default: virReportError(VIR_ERR_INVALID_ARG, @@ -329,6 +313,34 @@ virScaleInteger(unsigned long long *value, const char *suffix, } }

+ return 0; +} + +/* Scale an integer VALUE in-place by an optional case-insensitive + * SUFFIX, defaulting to SCALE if suffix is NULL or empty (scale is + * typically 1 or 1024). Recognized suffixes include 'b' or 'bytes', + * as well as power-of-two scaling via binary abbreviations ('KiB', + * 'MiB', ...) or their one-letter counterpart ('k', 'M', ...), and + * power-of-ten scaling via SI abbreviations ('KB', 'MB', ...). + * Ensure that the result does not exceed LIMIT. Return 0 on success, + * -1 with error message raised on failure. */ +int +virScaleInteger(unsigned long long *value, const char *suffix, + unsigned long long scale, unsigned long long limit) +{ + if (!suffix || !*suffix) { + if (!scale) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("invalid scale %llu"), scale); + return -1; + } + suffix = ""; + } else if (virSuffixToScale(suffix, &scale) < 0) { + virReportError(VIR_ERR_INVALID_ARG, + _("unable to parse suffix '%s'"), suffix); + return -1;

This is reporting error twice. virSuffixToScale() already reported an error, so there's no need to report second error here.

+ } + if (*value && *value > (limit / scale)) { virReportError(VIR_ERR_OVERFLOW, _("value too large: %llu%s"), *value, suffix); @@ -338,6 +350,24 @@ virScaleInteger(unsigned long long *value, const char *suffix, return 0; }

+/* Just like virScaleInteger, above, but produce an "double" value. */ +int +virScaleDouble(double *value, const char *suffix) +{ + unsigned long long scale; + + if (!suffix || !*suffix) + return -1; + + if (virSuffixToScale(suffix, &scale) < 0) { + virReportError(VIR_ERR_INVALID_ARG, + _("unable to parse suffix '%s'"), suffix); + return -1; + } + *value *= scale; + return 0; +} +

/** * virParseNumber: diff --git a/src/util/virutil.h b/src/util/virutil.h index 89b7923..d74b581 100644 --- a/src/util/virutil.h +++ b/src/util/virutil.h @@ -57,6 +57,9 @@ int virScaleInteger(unsigned long long *value, const char *suffix, unsigned long long scale, unsigned long long limit) ATTRIBUTE_NONNULL(1) ATTRIBUTE_RETURN_CHECK;

+int virScaleDouble(double *value, const char *suffix) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_RETURN_CHECK; +

This need src/libvirt_private.syms adjustment too.

int virHexToBin(unsigned char c);

int virParseNumber(const char **str);

I like the idea overall. But those nits I raised need to be fixed in v2. Michal