[libvirt] [PATCHv4 2/5] virstring.h/c: Util method for making some find and replace in strings

[Manuel VIVES]

> On 11/25/2013 07:23 PM, Manuel VIVES wrote:
> > ---
> > 
> >  src/libvirt_private.syms |    1 +
> >  src/util/virstring.c     |   48
> >  ++++++++++++++++++++++++++++++++++++++++++++++ src/util/virstring.h    
> >  |    2 ++
> >  3 files changed, 51 insertions(+)
> > 
> > diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> > index 99cc32a..b761fb6 100644
> > --- a/src/libvirt_private.syms
> > +++ b/src/libvirt_private.syms
> > @@ -1750,6 +1750,7 @@ virStringListLength;
> > 
> >  virStringSplit;
> >  virStrncpy;
> >  virStrndup;
> > 
> > +virStrReplace;
> > 
> >  virStrToDouble;
> >  virStrToLong_i;
> >  virStrToLong_l;
> > 
> > diff --git a/src/util/virstring.c b/src/util/virstring.c
> > index d11db5c..a30a4ef 100644
> > --- a/src/util/virstring.c
> > +++ b/src/util/virstring.c
> > @@ -616,3 +616,51 @@ size_t virStringListLength(char **strings)
> > 
> >      return i;
> >  
> >  }
> > 
> > +
> > +/*
> > + virStrReplace(haystack, oldneedle, newneedle) --
> > +  Search haystack and replace all occurences of oldneedle with
> > newneedle. +  Return a string with all the replacements in case of
> > success, NULL in case +  of failure
> > +*/
> > +char *
> > +virStrReplace(char *haystack,
> > +              const char *oldneedle, const char *newneedle)
> > +{
> > +    char *ret;
> > +    size_t i, count = 0;
> > +    size_t newneedle_len = strlen(newneedle);
> > +    size_t oldneedle_len = strlen(oldneedle);
> > +    size_t totalLength = 0;
> > +    if (strlen(oldneedle) == 0) {
> 
> You can just check oldneedle_len here since it already contains
> strlen(oldneedle).
> 
> Also, should this really be a NOP? It makes no sense to ask to replace
> all occurrences of "" with anything (kind of like dividing by 0), so I
> think it should rather be an error (likewise, do we need to check for
> !oldneedle or !newneedle ? Or are the callers controlled enough that we
> can just put that requirement on them? If the latter, then the
> declaration in the .h file needs to have "ATTRIBUTE_NONNULL(2),
> ATTRIBUTE_NONNULL(3) (and we probably should have ATTRIBUTE_NONNULL(1)
> in any case, unless there is a danger a caller could send a null haystack)
> 

I'm going to modify my patch in order to return an error if oldneedle
is empty.
I also looked at ATTRIBUTE_NONNULL and I did't really understand the
purpose so if someone could explain it ;)
But maybe I should also include the !oldneedle, the !newneedle and
!haystack tests inside the method, what do you think?

> > +        ignore_value(VIR_STRDUP(ret, haystack));
> > +        goto cleanup;
> > +    }
> > +
> > +    for (i = 0; haystack[i] != '\0'; i++) {
> > +        if (strstr(&haystack[i], oldneedle) == &haystack[i]) {
> 
> This just makes no sense. strstr is used to search for a substring
> within a string. Here you are searching for a substring, but only
> counting it if the match is at the beginning of the string. If you're
> going to do that, why not just use memcmp() instead and save all the
> extra scanning?
> 
> Or better yet, just let strstr() do its work and  set "i = [return from
> strstr] + oldneedle_len" when you get a match.
> 
> > +            count++;
> > +            i += oldneedle_len - 1;
> > +        }
> > +    }
> 
> Okay, so at this point i == strlen(haystack)...
> 
> > +    if (VIR_ALLOC_N(ret, (i + count * (newneedle_len - oldneedle_len)))
> > < 0) {
> 
> You need to allocate one extra byte for the terminating \0.
> 
> > +        ret = NULL;
> 
> No need to set ret = NULL - that is done by VIR_ALLOC_N.
> 
> > +        goto cleanup;
> > +    }
> > +    totalLength = sizeof(char *)*(i + count * (newneedle_len -
> > oldneedle_len));
> 
> This is incorrect and potentially dangerous - "sizeof(char*) is the size
> of a char* (usually 8 bytes), NOT the size of a char (1 byte), so you're
> telling virStrcpy() that there are 8 times as many bytes available as
> there actually are.
> 
> > +    i = 0;
> > +    while (*haystack) {
> > +        if (strstr(haystack, oldneedle) == haystack) {
> 
> Again, if you're only going to count a match when it is exactly at the
> start of the string, you should just use memcmp instead of strstr. Or
> alternately, make the algorithm more intelligent so that it takes
> advantage of any platform-specific optimizations that may be hidden in
> strstr.
> 
> > +            if (virStrcpy(&ret[i], newneedle, totalLength) == NULL) {
> 
> Even if you had set the value of totalLength properly above (you
> didn't), this is also incorrect, because you haven't accounted for the
> fact that you're copying into the middle of the string, not the start.
> 
> > +                ret = NULL;
> 
> You just leaked the memory at ret. Once you've allocated memory to it,
> if you're going to return failure you need to do "VIR_FREE(ret) instead
> (which will free the memory and set it to NULL).
> 
> > +                goto cleanup;
> > +            }
> > +            i += newneedle_len;
> > +            haystack += oldneedle_len;
> > +        } else
> > +            ret[i++] = *haystack++;
> > +    }
> 
> I'm not going to try and verify correctness of the remainder of this
> function, because it is broken and needs reworking anyway.
> 
> > +    ret[i] = '\0';
> > +cleanup:
> > +    return ret;
> > +}
> > diff --git a/src/util/virstring.h b/src/util/virstring.h
> > index b390150..90522bd 100644
> > --- a/src/util/virstring.h
> > +++ b/src/util/virstring.h
> > @@ -223,4 +223,6 @@ size_t virStringListLength(char **strings);
> > 
> >      virAsprintfInternal(false, 0, NULL, NULL, 0, \
> >      
> >                          strp, __VA_ARGS__)
> > 
> > +char * virStrReplace(char *haystack, const char *oldneedle, const char
> > *newneedle); +
> > 
> >  #endif /* __VIR_STRING_H__ */