[libvirt] [PATCH v2 1/4] conf: Make virDomainLoaderDefParseXML parse <nvram/> too

[Laszlo Ersek]

On 01/13/15 18:20, Michal Privoznik wrote:
> This is pure code movement without any functional change.
> The code simply reads better this way, that's all.
> 
> Signed-off-by: Michal Privoznik <mprivozn at redhat.com>
> ---
>  src/conf/domain_conf.c | 23 ++++++++++++++---------
>  1 file changed, 14 insertions(+), 9 deletions(-)
> 
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 3cbb93d..ae18255 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -12617,16 +12617,17 @@ virDomainDefMaybeAddHostdevSCSIcontroller(virDomainDefPtr def)
>  }
>  
>  static int
> -virDomainLoaderDefParseXML(xmlNodePtr node,
> +virDomainLoaderDefParseXML(xmlNodePtr loader_node,
> +                           xmlNodePtr nvram_node,
>                             virDomainLoaderDefPtr loader)
>  {
>      int ret = -1;
>      char *readonly_str = NULL;
>      char *type_str = NULL;
>  
> -    readonly_str = virXMLPropString(node, "readonly");
> -    type_str = virXMLPropString(node, "type");
> -    loader->path = (char *) xmlNodeGetContent(node);
> +    readonly_str = virXMLPropString(loader_node, "readonly");
> +    type_str = virXMLPropString(loader_node, "type");
> +    loader->path = (char *) xmlNodeGetContent(loader_node);
>  
>      if (readonly_str &&
>          (loader->readonly = virTristateBoolTypeFromString(readonly_str)) <= 0) {
> @@ -12645,6 +12646,11 @@ virDomainLoaderDefParseXML(xmlNodePtr node,
>          loader->type = type;
>      }
>  
> +    if (nvram_node) {
> +        loader->nvram = (char *)xmlNodeGetContent(nvram_node);
> +        loader->templt = virXMLPropString(nvram_node, "template");
> +    }
> +
>      ret = 0;
>   cleanup:
>      VIR_FREE(readonly_str);
> @@ -13735,7 +13741,7 @@ virDomainDefParseXML(xmlDocPtr xml,
>      if (STREQ(def->os.type, "xen") ||
>          STREQ(def->os.type, "hvm") ||
>          STREQ(def->os.type, "uml")) {
> -        xmlNodePtr loader_node;
> +        xmlNodePtr loader_node, nvram_node;
>  
>          def->os.kernel = virXPathString("string(./os/kernel[1])", ctxt);
>          def->os.initrd = virXPathString("string(./os/initrd[1])", ctxt);
> @@ -13746,11 +13752,10 @@ virDomainDefParseXML(xmlDocPtr xml,
>              if (VIR_ALLOC(def->os.loader) < 0)
>                  goto error;
>  
> -            if (virDomainLoaderDefParseXML(loader_node, def->os.loader) < 0)
> -                goto error;
> +            nvram_node = virXPathNode("./os/nvram[1]", ctxt);
>  
> -            def->os.loader->nvram = virXPathString("string(./os/nvram[1])", ctxt);
> -            def->os.loader->templt = virXPathString("string(./os/nvram[1]/@template)", ctxt);
> +            if (virDomainLoaderDefParseXML(loader_node, nvram_node, def->os.loader) < 0)
> +                goto error;
>          }
>      }
>  
> 

You turned the two invocations of virXPathString() into a call to
xmlNodeGetContent() and another to virXMLPropString(). I tried to verify
if the cases when the former function returns NULL are identical to the
cases when the latter functions return NULL. I'm not 100% sure, but quite.

In particular, virXPathString() returns NULL for an empty XPath string
(see (obj->stringval[0] == 0) in it), which quite explains the string()
conversion inside the XPath expression (pre-patch).

Post-patch, I'm unsure. The nvram_node assignment looks okay I guess.

The other two calls:
- http://xmlsoft.org/html/libxml-tree.html#xmlNodeGetContent
- http://xmlsoft.org/html/libxml-tree.html#xmlGetProp
  (wrapped by virXMLPropString())

are dubious, especially for the template assignment. Pre-patch, an empty
string in the XML stored a NULL, post-patch, I'm unsure if an empty
string in the XML can return "" rather than NULL.

I can ACK this if you clean up my doubts :)

(You can justify the code simply by testing empty content and empty
attribute value too.)

Thanks
Laszlo