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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Mon, May 14, 2018 at 01:15 PM +0200, Prerna Saxena <saxenap.ltc@xxxxxxxxx> 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@xxxxxxxxx>
> ---
>  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@xxxxxxxxxx
> 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@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list




[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]

  Powered by Linux