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

[Laine Stump]

On 12/23/2013 05:47 PM, Manuel VIVES wrote:
>> 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 ;)

ATTRIBUTE_NONNULL() is intended as a potential hint to the compiler's
optimizer, or to any static analysis tool such as coverity, that the
function will never be called with that particular argument being NULL.
Whether or not that information is put to any use is up to the
compiler/analyzer. In general, if an argument is given a very controlled
set of values (e.g. if it is always specified as a constant value, or if
all of the callers have already checked that arg for non-NULL) then it
can be useful to declare it as ATTRIBUTE_NONNULL.


> But maybe I should also include the !oldneedle, the !newneedle and
> !haystack tests inside the method, what do you think?

The choice is yours, but if you don't check, then you need to document
that in the description of the function so that future callers will know
to check for non-NULL prior to calling (unless they are already certain).