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@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)) {
+ 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"));
+ goto cleanup;
+ } else {
+ loader->nvram->type = VIR_STORAGE_TYPE_FILE;
+ loader->oldStyleNvram = true;
+ ret = 0;
+ }
+ }
+ return ret;
+
+cleanup:
+ if (loader->nvram)
+ VIR_FREE(loader->nvram);
+ return ret;
+}
static virBitmapPtr
virDomainSchedulerParse(xmlNodePtr node,
@@ -18422,11 +18535,15 @@ virDomainDefParseBootOptions(virDomainDefPtr def,
if (VIR_ALLOC(def->os.loader) < 0)
goto error;
- if (virDomainLoaderDefParseXML(loader_node, def->os.loader) < 0)
+ def->os.loader->src = NULL;
+ def->os.loader->nvram = NULL;
+ if (virDomainLoaderDefParseXML(loader_node, ctxt, def->os.loader) < 0)
goto error;
- def->os.loader->nvram = virXPathString("string(./os/nvram[1])", ctxt);
- def->os.loader->templt = virXPathString("string(./os/nvram[1]/@template)", ctxt);
+ if ((loader_node = virXPathNode("./os/nvram[1]", ctxt)) &&
+ (virDomainLoaderNvramDefParseXML(loader_node, ctxt,
+ def->os.loader) < 0))
+ goto error;
}
}
--
1.8.1.2
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On Mon, May 14, 2018 at 01:15 PM +0200, Prerna Saxena <saxenap.ltc@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@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.
> +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) {
> + continue;
> + }
> +
> + if (virXMLNodeNameEqual(cur, "template")) {
> + loader->templt = virXPathString("string(./os/nvram[1]/@template)", ctxt);
Shouldn’t this be cleaned up in case of an error?
> + 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"));
> + goto cleanup;
> + } else {
> + loader->nvram->type = VIR_STORAGE_TYPE_FILE;
> + loader->oldStyleNvram = true;
> + ret = 0;
> + }
> + }
> + return ret;
> +
> +cleanup:
> + if (loader->nvram)
> + VIR_FREE(loader->nvram);
virStorageSourceFree…
> + return ret;
> +}
>
> static virBitmapPtr
> virDomainSchedulerParse(xmlNodePtr node,
> @@ -18422,11 +18535,15 @@ virDomainDefParseBootOptions(virDomainDefPtr def,
> if (VIR_ALLOC(def->os.loader) < 0)
> goto error;
>
> - if (virDomainLoaderDefParseXML(loader_node, def->os.loader) < 0)
> + def->os.loader->src = NULL;
> + def->os.loader->nvram = NULL;
Should be unneeded as VIR_ALLOC used calloc.
> + if (virDomainLoaderDefParseXML(loader_node, ctxt, def->os.loader) < 0)
> goto error;
>
> - def->os.loader->nvram = virXPathString("string(./os/nvram[1])", ctxt);
> - def->os.loader->templt = virXPathString("string(./os/nvram[1]/@template)", ctxt);
> + if ((loader_node = virXPathNode("./os/nvram[1]", ctxt)) &&
> + (virDomainLoaderNvramDefParseXML(loader_node, ctxt,
> + def->os.loader) < 0))
> + goto error;
> }
> }
Note: I didn't take a closer look.
>
> --
> 1.8.1.2
>
> --
> libvir-list mailing list
> libvir-list@redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
Beste Grüße / Kind regards
Marc Hartmayer
IBM Deutschland Research & Development GmbH
Vorsitzende des Aufsichtsrats: Martina Koederitz
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
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@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@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.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
© 2016 - 2026 Red Hat, Inc.