Re: [PATCH v2 06/10] conf: Extract UUID parsing into its own function

[Martin Kletzander <mkletzan@xxxxxxxxxx>]

On Thu, Dec 10, 2015 at 03:17:01PM -0500, John Ferlan wrote:
> On 12/01/2015 12:35 PM, Martin Kletzander wrote:
> > Create virDomainDefParseUUID that parses only the UUID from XML
> > definition.  This will be used in future patch(es).
> >
> > Signed-off-by: Martin Kletzander <mkletzan@xxxxxxxxxx>
> > ---
> >  src/conf/domain_conf.c | 64 +++++++++++++++++++++++++++++++++++---------------
> >  1 file changed, 45 insertions(+), 19 deletions(-)
> >
> > diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> > index de6853a4dbd0..7a295da507c4 100644
> > --- a/src/conf/domain_conf.c
> > +++ b/src/conf/domain_conf.c
> > @@ -14446,6 +14446,49 @@ virDomainVcpuParse(virDomainDefPtr def,
> >      return ret;
> >  }
> >
> > +/*
> > + * Parse domain's UUID, optionally generating it if missing.
> > + */
> > +static int
> > +virDomainDefParseUUID(virDomainDefPtr def,
> > +                      xmlXPathContextPtr ctxt,
> > +                      bool required,
> > +                      bool *generated)
> > +{
> > +    char *tmp = virXPathString("string(./uuid[1])", ctxt);
> > +
> > +    if (generated)
> > +        *generated = false;
> > +
> > +    if (tmp) {
> > +        if (virUUIDParse(tmp, def->uuid) < 0) {
> > +            virReportError(VIR_ERR_INTERNAL_ERROR,
> > +                           "%s", _("malformed uuid element"));
> > +            VIR_FREE(tmp);
> > +            return -1;
> > +        }
> > +        VIR_FREE(tmp);
> > +        return 0;
>
> since you're returning anyways...
>
>    int ret;
>    if ((ret = virUUIDParse(...)) < 0)
>        virReportError(...)
>    VIR_FREE(tmp);
>    return ret;
>
> Your code works - IDC either way.

I like your way a bit more.

> > +    }
> > +
> > +    if (required) {
> > +        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> > +                       _("Missing required UUID"));
> > +        return -1;
> > +    }
> > +
> > +    if (virUUIDGenerate(def->uuid)) {
>
> if (virUUIDGenerate(...) < 0)

And this one is just my fault, of course it needs to be "< 0".

> ACK with at least this one fixed - just to conform with other error path
> uses...
>
> John
> > +        virReportError(VIR_ERR_INTERNAL_ERROR,
> > +                       "%s", _("Failed to generate UUID"));
> > +        return -1;
> > +    }
> > +
> > +    if (generated)
> > +        *generated = true;
> > +
> > +    return 0;
> > +}
> > +
> >  static int
> >  virDomainDefParseName(virDomainDefPtr def,
> >                        xmlXPathContextPtr ctxt)
> > @@ -14587,25 +14630,8 @@ virDomainDefParseXML(xmlDocPtr xml,
> >      if (virDomainDefParseName(def, ctxt) < 0)
> >          goto error;
> >
> > -    /* Extract domain uuid. If both uuid and sysinfo/system/entry/uuid
> > -     * exist, they must match; and if only the latter exists, it can
> > -     * also serve as the uuid. */
> > -    tmp = virXPathString("string(./uuid[1])", ctxt);
> > -    if (!tmp) {
> > -        if (virUUIDGenerate(def->uuid)) {
> > -            virReportError(VIR_ERR_INTERNAL_ERROR,
> > -                           "%s", _("Failed to generate UUID"));
> > -            goto error;
> > -        }
> > -        uuid_generated = true;
> > -    } else {
> > -        if (virUUIDParse(tmp, def->uuid) < 0) {
> > -            virReportError(VIR_ERR_INTERNAL_ERROR,
> > -                           "%s", _("malformed uuid element"));
> > -            goto error;
> > -        }
> > -        VIR_FREE(tmp);
> > -    }
> > +    if (virDomainDefParseUUID(def, ctxt, false, &uuid_generated) < 0)
> > +        goto error;
> >
> >      /* Extract short description of domain (title) */
> >      def->title = virXPathString("string(./title[1])", ctxt);

Attachment: signature.asc
Description: PGP signature

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list