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@xxxxxxxxxx>
> ---
> 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
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list