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

[Ján Tomko]

On 09/02/2013 06:05 PM, Manuel VIVES wrote:
> ---
>  src/libvirt_private.syms |    1 +
>  src/util/viruuid.c       |   79 ++++++++++++++++++++++++++++++++++++++++++++++
>  src/util/viruuid.h       |    1 +
>  3 files changed, 81 insertions(+)
> 

> @@ -333,3 +337,78 @@ int virGetHostUUID(unsigned char *uuid)
>  
>      return ret;
>  }
> +
> +
> +/**
> + * virSearchUuid:
> + * Return the nth occurrence of a substring in sourceString which matches an uuid pattern
> + * If there is no substring, ret is not modified
> + *
> + * @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"
> + * @endcode
> + */
> +
> +char **
> +virSearchUuid(const char *sourceString, int occurrence,char **ret)

char ** func(..., char **ret) seems redundant.

Either drop the 'ret' parameter completely and return just char,
or change the return value to int (e.g. -1 = error, 0 = not found, 1 = found).

> +{
> +    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;
> +    if (regcomp(&pregUuidBracket, uuidRegex, REG_EXTENDED) != 0) {
> +        VIR_DEBUG("Error while compiling regular expression");

This should be virReportError.

> +        goto cleanup;

and a return here, or you'll call regfree on an uninitialized variable.

> +    }
> +    nmatch = pregUuidBracket.re_nsub;
> +    if (VIR_ALLOC_N(pmatch, nmatch) != 0) {
> +        virReportOOMError();

VIR_ALLOC already reports an error.

> +        goto cleanup;
> +    }
> +    while (i < (position+1)) {

You should exit the cycle on the first regexec failure, there's no point in
calling it again and again with the same arguments.

> +        if (regexec(&pregUuidBracket, sourceString, nmatch, pmatch, 0) == 0) {

All this
> +            char *substring = NULL;
> +            int start = pmatch[0].rm_so;
> +            int end = pmatch[0].rm_eo;
> +            size_t size = end - start;
> +            if (VIR_ALLOC_N(substring, (size + 1)) != 0) {
> +                virReportOOMError();
> +                goto cleanup;
> +            }
> +            if (substring) {
> +                if (virStrncpy(substring, &sourceString[start], size, size + 1)) {
> +                    substring[size] = '\0';
> +                    if (VIR_STRDUP(*ret, substring) < 0) {
> +                        VIR_DEBUG("cannot duplicate %s", substring);
> +                        goto cleanup;
> +                    }
> +                }
> +                VIR_FREE(substring);
> +            }

can be replaced by:

regoff_t start = pmatch[0].rm_so;
regoff_t end = pmatch[0].rm_eo;
if (VIR_STRNDUP(*ret, sourceString + start, end - start) < 0)
    goto cleanup;

But even then, by copying every UUID to *ret, you'd leak the string buffer
allocated for every one except the last one.

> +            sourceString = &sourceString[end];
> +        }
> +        ++i;
> +    }

These three lines are redundant:
> +    regfree(&pregUuidBracket);
> +    VIR_FREE(pmatch);
> +    return ret;

> +
> +cleanup:
> +    regfree(&pregUuidBracket);
> +    VIR_FREE(pmatch);
> +    return ret;
> +}

And I'm sorry, but I have no opinion on the rest of the patches.

Jan