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

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(a)redhat.com&gt; > --- > 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.)