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

Manuel VIVES manuel.vives at diateam.net
Mon Dec 23 15:47:03 UTC 2013


> 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__ */




More information about the libvir-list mailing list