[libvirt] [PATCH 03/12] Parse domain XML to generate virDomainLoaderDef & virDomainNvramDef.

[Peter Krempa]

On Mon, May 14, 2018 at 16:20:53 +0200, Marc Hartmayer wrote:
> On Mon, May 14, 2018 at 01:15 PM +0200, Prerna Saxena <saxenap.ltc at gmail.com> wrote:
> > This patch is used to interpret domain XML and store the Loader &
> > Nvram's backing definitions as virStorageSource.
> > It also identifies if input XML used old or new-style declaration.
> > (This will later be used by the formatter).
> >
> > Signed-off-by: Prerna Saxena <saxenap.ltc at gmail.com>
> > ---
> >  src/conf/domain_conf.c | 131 ++++++++++++++++++++++++++++++++++++++++++++++---
> >  1 file changed, 124 insertions(+), 7 deletions(-)
> >
> > diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> > index f678e26..be43695 100644
> > --- a/src/conf/domain_conf.c
> > +++ b/src/conf/domain_conf.c
> > @@ -2884,8 +2884,8 @@ virDomainLoaderDefFree(virDomainLoaderDefPtr loader)
> >      if (!loader)
> >          return;
> >
> > -    VIR_FREE(loader->path);
> > -    VIR_FREE(loader->nvram);
> > +    virStorageSourceFree(loader->src);
> > +    virStorageSourceFree(loader->nvram);
> >      VIR_FREE(loader->templt);
> >      VIR_FREE(loader);
> >  }
> > @@ -17986,17 +17986,62 @@ virDomainDefMaybeAddHostdevSCSIcontroller(virDomainDefPtr def)
> >
> >  static int
> >  virDomainLoaderDefParseXML(xmlNodePtr node,
> > +                           xmlXPathContextPtr ctxt,
> >                             virDomainLoaderDefPtr loader)
> >  {
> >      int ret = -1;
> >      char *readonly_str = NULL;
> >      char *secure_str = NULL;
> >      char *type_str = NULL;
> > +    char *tmp = NULL;
> > +    xmlNodePtr cur;
> > +    xmlXPathContextPtr cur_ctxt = ctxt;
> > +
> > +    if (VIR_ALLOC(loader->src)) {
>                                  ^^
>                                  Usually it is checked for < 0.
> 
> > +        goto cleanup;
> > +    }
> > +    loader->src->type = VIR_STORAGE_TYPE_LAST;
> > +    loader->oldStyleLoader = false;
> >
> >      readonly_str = virXMLPropString(node, "readonly");
> >      secure_str = virXMLPropString(node, "secure");
> >      type_str = virXMLPropString(node, "type");
> > -    loader->path = (char *) xmlNodeGetContent(node);
> > +
> > +    if ((tmp = virXMLPropString(node, "backing")) &&
> > +        (loader->src->type = virStorageTypeFromString(tmp)) <= 0) {
> 
> If virStorageTypeFromString fails there is a memory leak of @tmp.
> 
> > +        virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> > +                       _("unknown loader type '%s'"), tmp);
> > +        goto cleanup;
> > +    }
> > +    VIR_FREE(tmp);
> > +
> > +    for (cur = node->children; cur != NULL; cur = cur->next) {
> > +        if (cur->type != XML_ELEMENT_NODE) {
> > +            continue;
> > +        }
> > +
> > +        if (virXMLNodeNameEqual(cur, "source")) {
> > +            /* new-style declaration found */
> > +            if (virDomainStorageSourceParse(cur, cur_ctxt, loader->src, 0) < 0) {
> > +                virReportError(VIR_ERR_XML_DETAIL,
> > +                               _("Error parsing Loader source element"));
> > +                goto cleanup;
> > +            }
> > +            break;
> > +        }
> > +    }
> > +
> > +    /* Old-style absolute path found ? */
> > +    if (loader->src->type == VIR_STORAGE_TYPE_LAST) {
> > +        if (!(loader->src->path = (char *) xmlNodeGetContent(node))) {
> > +            virReportError(VIR_ERR_XML_ERROR, "%s",
> > +                           _("missing loader source"));
> > +            goto cleanup;
> > +        } else {
> > +            loader->src->type = VIR_STORAGE_TYPE_FILE;
> > +            loader->oldStyleLoader = true;
> > +        }
> > +    }
> >
> >      if (readonly_str &&
> >          (loader->readonly = virTristateBoolTypeFromString(readonly_str)) <= 0) {
> > @@ -18023,13 +18068,81 @@ virDomainLoaderDefParseXML(xmlNodePtr node,
> >      }
> >
> >      ret = 0;
> > - cleanup:
> > +    goto exit;
> > +cleanup:
> 
> I would replace: s/cleanup/error and s/exit/cleanup.
> 
> > +    if (loader->src)
> > +      VIR_FREE(loader->src);
> 
> Shouldn’t it be a virStorageSourceFree() here? Not sure if it’s needed
> but it’s safer.

Additionally the preferred way is to use a second virStorageSourcePtr to
hold the data while it's being parsed and then use VIR_STEAL_PTR to put
it into the structure.

The cleanup section then can call virStorageSourceFree on the temp ptr
unconditionally and it also avoids this messy labels.

> 
> > +exit:
> >      VIR_FREE(readonly_str);
> >      VIR_FREE(secure_str);
> >      VIR_FREE(type_str);
> > +
> >      return ret;
> >  }
> >
> > +static int
> > +virDomainLoaderNvramDefParseXML(xmlNodePtr node,
> > +                           xmlXPathContextPtr ctxt,
> > +                           virDomainLoaderDefPtr loader)
> > +{
> > +    int ret = -1;
> > +    char *tmp = NULL;
> > +    xmlNodePtr cur;
> > +
> > +    if (VIR_ALLOC(loader->nvram)) {
>                                    ^^
>                                    Usually it is checked for < 0.
> 
> 
> > +        goto cleanup;
> > +    }
> 
> Unneeded braces.

Actually these would fail the syntax-check. Please make sure that
syntax-check passes on every patch.

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20180514/1187f9f3/attachment-0001.sig>