Make the parser both more strict, by not ignoring errors reported
by virStrToLong_ui(), and more permissive, by not failing due to
unrelated fields which just happen to have a know prefix and
accepting any amount of whitespace before the numeric value.
Signed-off-by: Andrea Bolognani <abologna@redhat.com>
---
src/util/virhostcpu.c | 62 +++++++++++++++++++++----
tests/virhostcpudata/linux-x86_64-test1.cpuinfo | 4 ++
2 files changed, 57 insertions(+), 9 deletions(-)
diff --git a/src/util/virhostcpu.c b/src/util/virhostcpu.c
index 3c20755eb..9c9f362de 100644
--- a/src/util/virhostcpu.c
+++ b/src/util/virhostcpu.c
@@ -508,6 +508,27 @@ virHostCPUHasValidSubcoreConfiguration(int threads_per_subcore)
return ret;
}
+/**
+ * virHostCPUParseFrequencyString:
+ * @str: string to be parsed
+ * @prefix: expected prefix
+ * @mhz: output location
+ *
+ * Parse a /proc/cpuinfo line and extract the CPU frequency, if present.
+ *
+ * The expected format of @str looks like
+ *
+ * cpu MHz : 2100.000
+ *
+ * where @prefix ("cpu MHz" in the example), is architecture-dependent.
+ *
+ * The decimal part of the CPU frequency, as well as all whitespace, is
+ * ignored.
+ *
+ * Returns: 0 when the string has been parsed successfully and the CPU
+ * frequency has been stored in @mhz, >0 when the string has not
+ * been parsed but no error has occurred, <0 on failure.
+ */
static int
virHostCPUParseFrequencyString(const char *str,
const char *prefix,
@@ -516,26 +537,49 @@ virHostCPUParseFrequencyString(const char *str,
char *p;
unsigned int ui;
+ /* If the string doesn't start with the expected prefix, then
+ * we're not looking at the right string and we should move on */
if (!STRPREFIX(str, prefix))
return 1;
+ /* Skip the prefix */
str += strlen(prefix);
- while (*str && c_isspace(*str))
+ /* Skip all whitespace */
+ while (c_isspace(*str))
str++;
+ if (*str == '\0')
+ goto error;
- if (*str != ':' || !str[1]) {
- virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
- _("parsing cpu MHz from cpuinfo"));
- return -1;
+ /* Skip the colon. If anything but a colon is found, then we're
+ * not looking at the right string and we should move on */
+ if (*str != ':')
+ return 1;
+ str++;
+
+ /* Skip all whitespace */
+ while (c_isspace(*str))
+ str++;
+ if (*str == '\0')
+ goto error;
+
+ /* Parse the frequency. We expect an unsigned value, optionally
+ * followed by a fractional part (which gets discarded) or some
+ * leading whitespace */
+ if (virStrToLong_ui(str, &p, 10, &ui) < 0 ||
+ (*p != '.' && *p != '\0' && !c_isspace(*p))) {
+ goto error;
}
- if (virStrToLong_ui(str + 1, &p, 10, &ui) == 0 &&
- /* Accept trailing fractional part. */
- (*p == '\0' || *p == '.' || c_isspace(*p)))
- *mhz = ui;
+ *mhz = ui;
return 0;
+
+ error:
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Missing or invalid CPU frequency in %s"),
+ CPUINFO_PATH);
+ return -1;
}
static int
diff --git a/tests/virhostcpudata/linux-x86_64-test1.cpuinfo b/tests/virhostcpudata/linux-x86_64-test1.cpuinfo
index e88a48ff3..706b69a54 100644
--- a/tests/virhostcpudata/linux-x86_64-test1.cpuinfo
+++ b/tests/virhostcpudata/linux-x86_64-test1.cpuinfo
@@ -28,6 +28,10 @@ model : 4
model name : Intel(R) Xeon(TM) CPU 2.80GHz
stepping : 8
cpu MHz : 2800.000
+# The following field is a made-up one, intended to make sure our cpuinfo
+# parser deals correctly with the introduction of new fields that just so
+# happen to share a prefix with the one used for CPU frequency
+cpu MHz rounded up to GHz : 3000.000
cache size : 2048 KB
physical id : 0
siblings : 2
--
2.14.3
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Andrea Bolognani <abologna@redhat.com> [2017-12-14, 01:34PM +0100]:
> Make the parser both more strict, by not ignoring errors reported
> by virStrToLong_ui(), and more permissive, by not failing due to
> unrelated fields which just happen to have a know prefix and
> accepting any amount of whitespace before the numeric value.
>
> Signed-off-by: Andrea Bolognani <abologna@redhat.com>
> ---
> src/util/virhostcpu.c | 62 +++++++++++++++++++++----
> tests/virhostcpudata/linux-x86_64-test1.cpuinfo | 4 ++
> 2 files changed, 57 insertions(+), 9 deletions(-)
>
> diff --git a/src/util/virhostcpu.c b/src/util/virhostcpu.c
> index 3c20755eb..9c9f362de 100644
> --- a/src/util/virhostcpu.c
> +++ b/src/util/virhostcpu.c
> @@ -508,6 +508,27 @@ virHostCPUHasValidSubcoreConfiguration(int threads_per_subcore)
> return ret;
> }
>
> +/**
> + * virHostCPUParseFrequencyString:
> + * @str: string to be parsed
> + * @prefix: expected prefix
> + * @mhz: output location
> + *
> + * Parse a /proc/cpuinfo line and extract the CPU frequency, if present.
> + *
> + * The expected format of @str looks like
> + *
> + * cpu MHz : 2100.000
> + *
> + * where @prefix ("cpu MHz" in the example), is architecture-dependent.
> + *
> + * The decimal part of the CPU frequency, as well as all whitespace, is
> + * ignored.
> + *
> + * Returns: 0 when the string has been parsed successfully and the CPU
> + * frequency has been stored in @mhz, >0 when the string has not
Maybe, >0 when the line prefix does not match exactly?
> + * been parsed but no error has occurred, <0 on failure.
> + */
> static int
> virHostCPUParseFrequencyString(const char *str,
> const char *prefix,
> @@ -516,26 +537,49 @@ virHostCPUParseFrequencyString(const char *str,
> char *p;
> unsigned int ui;
>
> + /* If the string doesn't start with the expected prefix, then
> + * we're not looking at the right string and we should move on */
> if (!STRPREFIX(str, prefix))
> return 1;
>
> + /* Skip the prefix */
> str += strlen(prefix);
>
> - while (*str && c_isspace(*str))
> + /* Skip all whitespace */
> + while (c_isspace(*str))
> str++;
> + if (*str == '\0')
> + goto error;
>
> - if (*str != ':' || !str[1]) {
> - virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> - _("parsing cpu MHz from cpuinfo"));
> - return -1;
> + /* Skip the colon. If anything but a colon is found, then we're
> + * not looking at the right string and we should move on */
> + if (*str != ':')
> + return 1;
> + str++;
You could do *str++ != ':' and save one line.
> +
> + /* Skip all whitespace */
> + while (c_isspace(*str))
> + str++;
> + if (*str == '\0')
> + goto error;
> +
> + /* Parse the frequency. We expect an unsigned value, optionally
> + * followed by a fractional part (which gets discarded) or some
> + * leading whitespace */
> + if (virStrToLong_ui(str, &p, 10, &ui) < 0 ||
> + (*p != '.' && *p != '\0' && !c_isspace(*p))) {
> + goto error;
> }
>
> - if (virStrToLong_ui(str + 1, &p, 10, &ui) == 0 &&
> - /* Accept trailing fractional part. */
> - (*p == '\0' || *p == '.' || c_isspace(*p)))
> - *mhz = ui;
> + *mhz = ui;
>
> return 0;
> +
> + error:
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("Missing or invalid CPU frequency in %s"),
> + CPUINFO_PATH);
> + return -1;
> }
>
> static int
> diff --git a/tests/virhostcpudata/linux-x86_64-test1.cpuinfo b/tests/virhostcpudata/linux-x86_64-test1.cpuinfo
> index e88a48ff3..706b69a54 100644
> --- a/tests/virhostcpudata/linux-x86_64-test1.cpuinfo
> +++ b/tests/virhostcpudata/linux-x86_64-test1.cpuinfo
> @@ -28,6 +28,10 @@ model : 4
> model name : Intel(R) Xeon(TM) CPU 2.80GHz
> stepping : 8
> cpu MHz : 2800.000
> +# The following field is a made-up one, intended to make sure our cpuinfo
> +# parser deals correctly with the introduction of new fields that just so
> +# happen to share a prefix with the one used for CPU frequency
> +cpu MHz rounded up to GHz : 3000.000
> cache size : 2048 KB
> physical id : 0
> siblings : 2
> --
> 2.14.3
>
> --
> libvir-list mailing list
> libvir-list@redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
>
--
IBM Systems
Linux on z Systems & Virtualization Development
------------------------------------------------------------------------
IBM Deutschland
Schönaicher Str. 220
71032 Böblingen
Phone: +49 7031 16 1819
E-Mail: bwalk@de.ibm.com
------------------------------------------------------------------------
IBM Deutschland Research & Development GmbH
Vorsitzende des Aufsichtsrats: Martina Koederitz
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On Thu, 2017-12-14 at 14:55 +0100, Bjoern Walk wrote: > > + * Returns: 0 when the string has been parsed successfully and the CPU > > + * frequency has been stored in @mhz, >0 when the string has not > > Maybe, >0 when the line prefix does not match exactly? Documentation goes out of sync with reality quickly enough when the language used is purposefully vague ;) > > + /* Skip the colon. If anything but a colon is found, then we're > > + * not looking at the right string and we should move on */ > > + if (*str != ':') > > + return 1; > > + str++; > > You could do *str++ != ':' and save one line. I'd rather not. Lines are cheap :) -- Andrea Bolognani / Red Hat / Virtualization -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Andrea Bolognani <abologna@redhat.com> [2017-12-14, 03:01PM +0100]: > On Thu, 2017-12-14 at 14:55 +0100, Bjoern Walk wrote: > > > + * Returns: 0 when the string has been parsed successfully and the CPU > > > + * frequency has been stored in @mhz, >0 when the string has not > > > > Maybe, >0 when the line prefix does not match exactly? > > Documentation goes out of sync with reality quickly enough when > the language used is purposefully vague ;) > > > > + /* Skip the colon. If anything but a colon is found, then we're > > > + * not looking at the right string and we should move on */ > > > + if (*str != ':') > > > + return 1; > > > + str++; > > > > You could do *str++ != ':' and save one line. > > I'd rather not. Lines are cheap :) > Yeah, this was just nit-picking. Looks like I forgot my rb, so Reviewed-by: Bjoern Walk <bwalk@linux.vnet.ibm.com> > -- > Andrea Bolognani / Red Hat / Virtualization > > -- > libvir-list mailing list > libvir-list@redhat.com > https://www.redhat.com/mailman/listinfo/libvir-list > -- IBM Systems Linux on z Systems & Virtualization Development ------------------------------------------------------------------------ IBM Deutschland Schönaicher Str. 220 71032 Böblingen Phone: +49 7031 16 1819 E-Mail: bwalk@de.ibm.com ------------------------------------------------------------------------ IBM Deutschland Research & Development GmbH Vorsitzende des Aufsichtsrats: Martina Koederitz Geschäftsführung: Dirk Wittkopp Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
© 2016 - 2025 Red Hat, Inc.