[libvirt] [PATCH] openvz: fixed an config file parsing error
Michal Privoznik
mprivozn at redhat.com
Mon Sep 15 12:19:01 UTC 2014
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
More information about the libvir-list
mailing list