Re: [libvirt] [PATCH] openvz: fixed an config file parsing error

Resend the email below. On Mon, Sep 1, 2014 at 8:28 PM, Hongbin Lu <hongbin034@gmail.com> 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 (e.g. 1024). 2. Assume the number is an integer. Actually, an OpenVZ config parameter may consists 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 | 99 ++++++++++++++++++++++++++++++++++++++++++++- src/openvz/openvz_conf.h | 6 +++ 2 files changed, 102 insertions(+), 3 deletions(-)
diff --git a/src/openvz/openvz_conf.c b/src/openvz/openvz_conf.c index 856c9f5..7dba925 100644 --- a/src/openvz/openvz_conf.c +++ b/src/openvz/openvz_conf.c @@ -127,6 +127,42 @@ int openvzExtractVersion(struct openvz_driver *driver) }
+static int +openvzParseParamUnit(char* str, openvzParamUnit *unit) +{ + int len = strlen(str); + if (len == 0) + return -1; + + switch (str[len - 1]) { + case 'G': + case 'g': + str[len - 1] = '\0'; + if (unit != NULL) + *unit = OPENVZ_PARAM_UNIT_GIGABYTE; + break; + case 'M': + case 'm': + str[len - 1] = '\0'; + if (unit != NULL) + *unit = OPENVZ_PARAM_UNIT_MEGABYTE; + break; + case 'K': + case 'k': + str[len - 1] = '\0'; + if (unit != NULL) + *unit = OPENVZ_PARAM_UNIT_KILOBYTE; + break; + case 'P': + case 'p': + str[len - 1] = '\0'; + break; + } + + return 0; +} + + /* Parse config values of the form barrier:limit into barrier and limit */ static int openvzParseBarrierLimit(const char* value, @@ -146,7 +182,10 @@ 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; } } @@ -155,7 +194,10 @@ openvzParseBarrierLimit(const char* value, goto error; } else { if (limit != NULL) { - if (virStrToLong_ull(token, NULL, 10, limit)) + if (openvzParseParamUnit(token, NULL) < 0) + goto error; + + if (virStrToLong_ull(token, NULL, 10, limit) < 0) goto error; } } @@ -166,6 +208,57 @@ openvzParseBarrierLimit(const char* value, }
+static int +openvzParseBarrierLimit2(const char* value, + unsigned long long *barrier, + unsigned long long *limit, + openvzParamUnit targetunit) +{ + char *token; + char *saveptr = NULL; + char *str; + double dbarrier, dlimit; + openvzParamUnit unit = OPENVZ_PARAM_UNIT_KILOBYTE; + int ret = -1; + + if (VIR_STRDUP(str, value) < 0) + goto error; + + token = strtok_r(str, ":", &saveptr); + if (token == NULL) + goto error; + + if (barrier != NULL) { + if (openvzParseParamUnit(token, &unit) < 0) + goto error; + + if (virStrToDouble(token, NULL, &dbarrier) < 0) + goto error; + + *barrier = (unsigned long long)(dbarrier * unit / targetunit); + } + + token = strtok_r(NULL, ":", &saveptr); + if (token == NULL) + goto error; + + if (limit != NULL) { + if (openvzParseParamUnit(token, &unit) < 0) + goto error; + + if (virStrToDouble(token, NULL, &dlimit) < 0) + goto error; + + *limit = (unsigned long long)(dlimit * unit / targetunit); + } + + ret = 0; + error: + VIR_FREE(str); + return ret; +} + + virCapsPtr openvzCapsInit(void) { virCapsPtr caps; @@ -396,7 +489,7 @@ openvzReadFSConf(virDomainDefPtr def, param = "DISKSPACE"; ret = openvzReadVPSConfigParam(veid, param, &temp); if (ret > 0) { - if (openvzParseBarrierLimit(temp, &barrier, &limit)) { + if (openvzParseBarrierLimit2(temp, &barrier, &limit, OPENVZ_PARAM_UNIT_KILOBYTE)) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Could not read '%s' from config for container %d"), param, veid); diff --git a/src/openvz/openvz_conf.h b/src/openvz/openvz_conf.h index a7de7d2..788dae9 100644 --- a/src/openvz/openvz_conf.h +++ b/src/openvz/openvz_conf.h @@ -41,6 +41,12 @@
# define VZCTL_BRIDGE_MIN_VERSION ((3 * 1000 * 1000) + (0 * 1000) + 22 + 1)
+typedef enum { + OPENVZ_PARAM_UNIT_KILOBYTE = 1, + OPENVZ_PARAM_UNIT_MEGABYTE = 1024, + OPENVZ_PARAM_UNIT_GIGABYTE = 1024 * 1024, +} openvzParamUnit; + struct openvz_driver { virMutex lock;
-- 1.7.1

On 09/03/2014 09:55 AM, Hongbin Lu wrote:
Resend the email below.
On Mon, Sep 1, 2014 at 8:28 PM, Hongbin Lu <hongbin034@gmail.com> 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 (e.g. 1024). 2. Assume the number is an integer. Actually, an OpenVZ config parameter may consists 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.
Can you please resend the patch directly? It's much easier to get 'git am' to apply a patch that isn't munged by a layer of > quoting. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org

On 09/03/2014 09:55 AM, 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 (e.g. 1024). 2. Assume the number is an integer. Actually, an OpenVZ config parameter may consists 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. ---
+static int +openvzParseParamUnit(char* str, openvzParamUnit *unit) +{ + int len = strlen(str); + if (len == 0) + return -1; + + switch (str[len - 1]) { + case 'G': + case 'g': + str[len - 1] = '\0'; + if (unit != NULL) + *unit = OPENVZ_PARAM_UNIT_GIGABYTE; + break;
This looks similar to the existing virScaleInteger (src/util/virutil.c) and its wrapper vshCommandOptScaledInt (tools/virsh.c); any chance you can reuse that code (maybe by making it more generic) instead of reimplementing it from scratch? -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org

Thanks Eric. I modified the fix accordingly. https://www.redhat.com/archives/libvir-list/2014-September/msg00363.html Best regards, Hongbin On Wed, Sep 3, 2014 at 12:19 PM, Eric Blake <eblake@redhat.com> wrote:
On 09/03/2014 09:55 AM, 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 (e.g. 1024). 2. Assume the number is an integer. Actually, an OpenVZ config parameter may consists 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. ---
+static int +openvzParseParamUnit(char* str, openvzParamUnit *unit) +{ + int len = strlen(str); + if (len == 0) + return -1; + + switch (str[len - 1]) { + case 'G': + case 'g': + str[len - 1] = '\0'; + if (unit != NULL) + *unit = OPENVZ_PARAM_UNIT_GIGABYTE; + break;
This looks similar to the existing virScaleInteger (src/util/virutil.c) and its wrapper vshCommandOptScaledInt (tools/virsh.c); any chance you can reuse that code (maybe by making it more generic) instead of reimplementing it from scratch?
-- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
participants (2)
-
Eric Blake
-
Hongbin Lu