Re: [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  
> 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 

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

[Marc Hartmayer]

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 

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

[Prerna Saxena]

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