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@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