9:47 a.m.
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__ */