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

[Martin Kletzander <mkletzan@xxxxxxxxxx>]

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@xxxxxxxxxx>
> > ---
> >  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

Attachment: signature.asc
Description: Digital signature

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list