[libvirt] [PATCHv4 1/5] viruuid.h/c: Util method for finding uuid patterns in some strings

[Laine Stump]

On 12/23/2013 05:47 PM, Manuel VIVES wrote:
>> On 11/25/2013 07:23 PM, Manuel VIVES wrote:
>>> + *
>>> + * @sourceString: String to parse
>>> + * @occurrence: We will return the nth occurrence of uuid in substring,
>>> if equals to 0 (or negative), will return the first occurence + * @ret:
>>> nth occurrence substring matching an uuid pattern
>>> + * @code
>>> +    char *source = "6853a496-1c10-472e-867a-8244937bd6f0
>>> 773ab075-4cd7-4fc2-8b6e-21c84e9cb391
>>> bbb3c75c-d60f-43b0-b802-fd56b84a4222
>>> 60c04aa1-0375-4654-8d9f-e149d9885273
>>> 4548d465-9891-4c34-a184-3b1c34a26aa8"; +    char *ret1=NULL;
>>> +    char *ret2=NULL;
>>> +    char *ret3=NULL;
>>> +    char *ret4=NULL;
>>> +    virSearchUuid(source, 4,&ret1);  //ret1 =
>>> "60c04aa1-0375-4654-8d9f-e149d9885273" +    virSearchUuid(source,
>>> 0,&ret2);  //ret2 = "6853a496-1c10-472e-867a-8244937bd6f0" +   
>>> virSearchUuid(source, 1,&ret3);  //ret3 =
>>> "6853a496-1c10-472e-867a-8244937bd6f0" +    virSearchUuid(source,
>>> -4,&ret4); //ret4 = "6853a496-1c10-472e-867a-8244937bd6f0"
>> What is the use of having occurrence == -n, 0, or 1 yield the same
>> result? It makes more sense to define the function as "return occurrence
>> 'n' (starting from 0) of a sub-string that matches the pattern for a
>> uuid". Documenting is then much simpler, and it's easier to know the
>> "best" way to get the first occurrence (since there is only one way to
>> do it).
>>
>> This seems like a very specific use case for a more general function.
>> How about making a virSearchRegex() function (or maybe some other name)
>> which took the regex as another arg? That would make it more likely that
>> the code would be reused.
>>
> I have modified this method, so now it takes a regex as argument and
> I have renamed it to 'virSearchRegex'. Should I move it in another file
> (it is still in viruuid.c/h)?

virstring.[ch] is probably the best place for it.

>
>
>>> + * @endcode
>>> + */
>>> +
>>> +int
>>> +virSearchUuid(const char *sourceString, int occurrence, char **ret)
>>> +{
>>> +    unsigned int position = ((occurrence -1) > 0) ? (occurrence -1) : 0;
>>> +    const char *uuidRegex =
>>> "([a-f0-9]{8}-[a-f0-9]{4}-[a-f0-9]{4}-[a-f0-9]{4}-[a-f0-9]{12})"; +   
>>> regex_t pregUuidBracket;
>>> +    size_t i = 0;
>>> +    size_t nmatch = 0;
>>> +    regmatch_t *pmatch = NULL;
>>> +    int retCode = 0;
>>> +    if (regcomp(&pregUuidBracket, uuidRegex, REG_EXTENDED) != 0) {
>> You're missing a blank line after the end of the data declarations.
>> Also, it's more common in libvirt to call the variable containing the
>> eventual function return value "ret" rather than "retCode", and
>> initialize it to -1. Then you just unconditionally set it to 0 just
>> before a successful return (or in this case, set it to 0 for the "not
>> found" case or 1 for the "found" case), but don't have to do anything to
>> it for all the failure cases.
>>
> I use retCode because I already have a parameter named ret which will
> contains the string matching, perhaps I should rename my parameter and
> use ret instead of retCode.

Yes, that would be more consistent, and consistency helps to avoid
future mistakes.