7:27 a.m.
On Thu, Apr 06, 2017 at 01:32:13PM +0200, Erik Skultety wrote:
On Wed, Apr 05, 2017 at 04:36:26PM +0200, Martin Kletzander wrote:
> And use it in virFileRead*
>
> Signed-off-by: Martin Kletzander <mkletzan(a)redhat.com>
> ---
> src/util/virfile.c | 18 +++++++-----------
> src/util/virhostcpu.c | 4 ++--
> src/util/virstring.h | 8 ++++++++
> src/util/virsysfs.c | 2 ++
> 4 files changed, 19 insertions(+), 13 deletions(-)
>
> diff --git a/src/util/virfile.c b/src/util/virfile.c
> index c0f448d3437d..cbfa3849d793 100644
> --- a/src/util/virfile.c
> +++ b/src/util/virfile.c
> @@ -3811,7 +3811,6 @@ int
> virFileReadValueInt(const char *path, int *value)
> {
> char *str = NULL;
> - char *endp = NULL;
>
> if (!virFileExists(path))
> return -2;
> @@ -3819,8 +3818,9 @@ virFileReadValueInt(const char *path, int *value)
> if (virFileReadAll(path, INT_STRLEN_BOUND(*value), &str) < 0)
> return -1;
>
> - if (virStrToLong_i(str, &endp, 10, value) < 0 ||
> - (endp && !c_isspace(*endp))) {
> + virStringTrimOptionalNewline(str);
> +
> + if (virStrToLong_i(str, NULL, 10, value) < 0) {
> virReportError(VIR_ERR_INTERNAL_ERROR,
> _("Invalid integer value '%s' in file
'%s'"),
> str, path);
> @@ -3847,7 +3847,6 @@ int
> virFileReadValueUint(const char *path, unsigned int *value)
> {
> char *str = NULL;
> - char *endp = NULL;
>
> if (!virFileExists(path))
> return -2;
> @@ -3855,8 +3854,9 @@ virFileReadValueUint(const char *path, unsigned int *value)
> if (virFileReadAll(path, INT_STRLEN_BOUND(*value), &str) < 0)
> return -1;
>
> - if (virStrToLong_uip(str, &endp, 10, value) < 0 ||
> - (endp && !c_isspace(*endp))) {
> + virStringTrimOptionalNewline(str);
> +
> + if (virStrToLong_uip(str, NULL, 10, value)) {
> virReportError(VIR_ERR_INTERNAL_ERROR,
> _("Invalid unsigned integer value '%s' in file
'%s'"),
> str, path);
> @@ -3886,7 +3886,6 @@ virFileReadValueBitmap(const char *path,
> {
> char *buf = NULL;
> int ret = -1;
> - char *tmp = NULL;
>
> if (!virFileExists(path))
> return -2;
> @@ -3894,10 +3893,7 @@ virFileReadValueBitmap(const char *path,
> if (virFileReadAll(path, maxlen, &buf) < 0)
> goto cleanup;
>
> - /* trim optinoal newline at the end */
> - tmp = buf + strlen(buf) - 1;
> - if (*tmp == '\n')
> - *tmp = '\0';
> + virStringTrimOptionalNewline(buf);
>
> *value = virBitmapParseUnlimited(buf);
> if (!*value)
> diff --git a/src/util/virhostcpu.c b/src/util/virhostcpu.c
> index 02b9fc8eb94f..a660e3f4dbe5 100644
> --- a/src/util/virhostcpu.c
> +++ b/src/util/virhostcpu.c
> @@ -847,13 +847,13 @@ virHostCPUParseCountLinux(void)
> tmp = str;
> do {
> if (virStrToLong_i(tmp, &tmp, 10, &ret) < 0 ||
> - !strchr(",-\n", *tmp)) {
> + !strchr(",-", *tmp)) {
> virReportError(VIR_ERR_NO_SUPPORT,
> _("failed to parse %s"), str);
> ret = -1;
> goto cleanup;
> }
> - } while (*tmp++ != '\n');
> + } while (*tmp++ && *tmp);
> ret++;
>
> cleanup:
> diff --git a/src/util/virstring.h b/src/util/virstring.h
> index a5550e30d2e2..603650aa1588 100644
> --- a/src/util/virstring.h
> +++ b/src/util/virstring.h
> @@ -288,4 +288,12 @@ bool virStringBufferIsPrintable(const uint8_t *buf, size_t
buflen);
>
> char *virStringEncodeBase64(const uint8_t *buf, size_t buflen);
>
> +static inline void
> +virStringTrimOptionalNewline(char *str)
> +{
> + char *tmp = str + strlen(str) - 1;
> + if (*tmp == '\n')
> + *tmp = '\0';
> +}
Is there any other reason for using this instead of virTrimSpaces than just
being a bit faster? Because I think the performance gain in this case compared
to 1 iteration of the while loop is very small, thus if possible I would avoid
creating a function for it when there is virTrimSpaces (and I think
virSkipSpacesBackwards would be usable too).
So, several factors:
1) I wasn't looking for functions that would do what I do here, I just
didn't want to be open-coding these three lines all the time.
2) I didn't want it to be a separate function (hence static inline in
the header file).
3) I didn't know we have functions like this.
4) Looking at these function I really don't like them. It's the
precise example on how trying to do everything makes it more
useless. It's doing super easy tiny thing that you want, but
because it "configurable", the code is..., well, let's say "not
very nice".
Having said that, I'm perfectly fine with changing it to using
virTrimSpaces() and hating those functions in my own free time. I'll
just add them to my list.
Other than that, it makes perfect sense to me, ACK.
Erik