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