9:47 a.m.
On 11/25/2013 07:23 PM, Manuel VIVES wrote:
> ---
>
> po/POTFILES.in | 1 +
> src/libvirt_private.syms | 1 +
> src/util/viruuid.c | 81
> ++++++++++++++++++++++++++++++++++++++++++++++ src/util/viruuid.h
> | 1 +
> 4 files changed, 84 insertions(+)
>
> diff --git a/po/POTFILES.in b/po/POTFILES.in
> index 15afdec..451a6fc 100644
> --- a/po/POTFILES.in
> +++ b/po/POTFILES.in
> @@ -196,6 +196,7 @@ src/util/virtypedparam.c
>
> src/util/viruri.c
> src/util/virusb.c
> src/util/virutil.c
>
> +src/util/viruuid.c
>
> src/util/virxml.c
> src/vbox/vbox_MSCOMGlue.c
> src/vbox/vbox_XPCOMCGlue.c
>
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index a705c56..99cc32a 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -1915,6 +1915,7 @@ virValidateWWN;
>
> # util/viruuid.h
> virGetHostUUID;
>
> +virSearchUuid;
>
> virSetHostUUIDStr;
> virUUIDFormat;
> virUUIDGenerate;
>
> diff --git a/src/util/viruuid.c b/src/util/viruuid.c
> index c5fa9a8..2cda4ac 100644
> --- a/src/util/viruuid.c
> +++ b/src/util/viruuid.c
> @@ -34,6 +34,7 @@
>
> #include <sys/stat.h>
> #include <time.h>
> #include <unistd.h>
>
> +#include <regex.h>
>
> #include "c-ctype.h"
> #include "internal.h"
>
> @@ -43,11 +44,14 @@
>
> #include "viralloc.h"
> #include "virfile.h"
> #include "virrandom.h"
>
> +#include "virstring.h"
>
> #ifndef ENODATA
> # define ENODATA EIO
> #endif
>
> +#define VIR_FROM_THIS VIR_FROM_NONE
> +
>
> static unsigned char host_uuid[VIR_UUID_BUFLEN];
>
> static int
>
> @@ -333,3 +337,80 @@ int virGetHostUUID(unsigned char *uuid)
>
> return ret;
>
> }
>
> +
> +
> +/**
> + * virSearchUuid:
> + * Allows you to get the nth occurrence of a substring in sourceString
> which matches an uuid pattern. + * If there is no substring, ret is not
> modified.
> + * return -1 on error, 0 if not found and 1 if found.
Hah! I accidentally started reviewing an earlier version of this patch,
and lack of a way to indicate error to the caller was one of my
criticisms :-)
> + *
> + * @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)?
> + * @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.
> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("Error while compiling regular expression"));
You may as well collect the return value from regcomp and include it in
the error message. Even if just as %d, it may help to debug in the
unlikely case an error is ever encountered.
> + return -1;
> + }
> + nmatch = pregUuidBracket.re_nsub;
> + if (VIR_ALLOC_N(pmatch, nmatch) != 0) {
For consistency, we try to check the return from all libvirt functions
with "< 0" instead of "!= 0", so:
if (VIR_ALLOC_N(pmatch, nmatch) < 0) ...
> + retCode = -1;
> + goto cleanup;
> + }
(here, for example, if you had initialized retCode (ret) to -1, you
could just do "goto cleanup" and wouldn't even need the brackets.
> +
> + while (i < (position+1)) {
> + if (regexec(&pregUuidBracket, sourceString, nmatch, pmatch, 0)
> == 0) { + regoff_t start = pmatch[0].rm_so;
> + regoff_t end = pmatch[0].rm_eo;
> + if (i == position || (position > i &&
> regexec(&pregUuidBracket, &sourceString[end], nmatch, pmatch, 0) != 0))
> { + /* We copy only if i == position (so that it is the
> uuid we're looking for), or position > i AND + * there
> is no matches left in the rest of the string (this is the case where we
> + * give a biggest @occurence than the number of matches
> and we want to return the last + * one)
> + */
Please reformat the comments to fit within 80 colums (this same comment
applies to the comments at the top of the function, as well as for code,
whenever practically possible).
> + if (VIR_STRNDUP(*ret, sourceString + start, end - start)
> < 0) { + retCode = -1;
> + goto cleanup;
> + }
> + retCode = 1;
> + goto cleanup;
> + }
> +
> + sourceString = &sourceString[end];
> + } else {
> + break;
> + retCode = 0;
> + goto cleanup;
> + }
> + ++i;
> + }
> +
> +cleanup:
> + regfree(&pregUuidBracket);
> + VIR_FREE(pmatch);
> + return retCode;
> +}
> diff --git a/src/util/viruuid.h b/src/util/viruuid.h
> index bebd338..2ce4fce 100644
> --- a/src/util/viruuid.h
> +++ b/src/util/viruuid.h
> @@ -40,4 +40,5 @@ int virUUIDParse(const char *uuidstr,
>
> const char *virUUIDFormat(const unsigned char *uuid,
>
> char *uuidstr) ATTRIBUTE_NONNULL(1)
> ATTRIBUTE_NONNULL(2);
>
> +int virSearchUuid(const char *sourceString, int occurrence, char **ret);
>
> #endif /* __VIR_UUID_H__ */