> 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__ */
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list