[libvirt] [PATCH v2 03/12] util: Add virStringTrimOptionalNewline

Erik Skultety eskultet at redhat.com
Thu Apr 6 11:32:13 UTC 2017


On Wed, Apr 05, 2017 at 04:36:26PM +0200, Martin Kletzander wrote:
> And use it in virFileRead*
>
> Signed-off-by: Martin Kletzander <mkletzan at 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).

Other than that, it makes perfect sense to me, ACK.

Erik




More information about the libvir-list mailing list