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