Re: [libvirt] [PATCH 03/12] Parse domain XML to generate virDomainLoaderDef & virDomainNvramDef.
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 > 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 > > --- > > 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
Re: [libvirt] [PATCH 03/12] Parse domain XML to generate virDomainLoaderDef & virDomainNvramDef.
On Mon, May 14, 2018 at 01:15 PM +0200, Prerna Saxena 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 > --- > 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. > + > +loader->nvram->type = VIR_STORAGE_TYPE_LAST; > +loader->nvram->oldStyleNvram = false; > + > +if ((tmp = virXMLPropString(node, "backing")) && > +(loader->nvram->type = virStorageTypeFromString(tmp)) <= 0) { If virStorageTypeFromString fails there is a memory leak of @tmp. > +virReportError(VIR_ERR_CONFIG_UNSUPPORTED, > + _("unknown nvram type '%s'"), tmp); > +goto cleanup; > +} > +VIR_FREE(tmp); > + > +for (cur = node->children; cur != NULL; cur = cur->next) { > +if (cur->type != XML_ELEMENT_NODE) { > +c
[libvirt] [PATCH 03/12] Parse domain XML to generate virDomainLoaderDef & virDomainNvramDef.
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 --- 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)) { +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) { +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: +if (loader->src) + VIR_FREE(loader->src); +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)) { +goto cleanup; +} + +loader->nvram->type = VIR_STORAGE_TYPE_LAST; +loader->nvram->oldStyleNvram = false; + +if ((tmp = virXMLPropString(node, "backing")) && +(loader->nvram->type = virStorageTypeFromString(tmp)) <= 0) { +virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("unknown nvram 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, "template")) { +loader->templt = virXPathString("string(./os/nvram[1]/@template)", ctxt); +continue; +} + +if (virXMLNodeNameEqual(cur, "source")) { +if (virDomainStorageSourceParse(cur, ctxt, loader->nvram, 0) < 0) { +virReportError(VIR_ERR_XML_DETAIL, + _("Error parsing nvram source element")); +goto cleanup; +} +ret = 0; +} +} + +/* Old-style declaration found */ +if (loader->nvram->type == VIR_STORAGE_TYPE_LAST) { +if (!(loader->nvram->path = (char *) xmlNodeGetContent(node))) { +virReportError(VIR_ERR_XML_ERROR, "%s", + _("missing nvram source")); +