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

On Mon, May 14, 2018 at 01:15 PM +0200, Prerna Saxena <saxenap.ltc(a)gmail.com&gt; 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(a)gmail.com&gt; > --- > 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.

> +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.