[libvirt] [PATCH v2 3/5] conf: Make virNetworkIPDefParseXML a little bit saner

[Laine Stump]

On 12/13/2016 08:52 AM, Jiri Denemark wrote:
> Iterating over all child nodes when we only support one instance of each
> child is pretty weired. And it would even cause memory leaks if more
> than one <tftp> element was specified.

ACK, but could you also look for dhcp[2]/tftp[2] and log an error if 
found? I know there are *lots* of places we ignore extra elements in the 
XML, but in this case there would be a silent behavior change if someone 
had (erroneous) multiple tftp or dhcp elements - previously we would 
have honored the final occurence of each element, but now we honor the 
first. So even though it's their own fault, it would be nice to

(BTW, I've always disliked that some of our XML parsing code iterates 
through the raw nodes like this used to, and some uses XPath to get 
specific nodes. It would be so much easier for newcomers to understand 
if we picked on method and used it consistently...)

>
> Signed-off-by: Jiri Denemark <jdenemar at redhat.com>
> ---
>   src/conf/network_conf.c | 36 ++++++++++++++----------------------
>   1 file changed, 14 insertions(+), 22 deletions(-)
>
> diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c
> index aabf315c9..b6849ceab 100644
> --- a/src/conf/network_conf.c
> +++ b/src/conf/network_conf.c
> @@ -1501,7 +1501,8 @@ virNetworkIPDefParseXML(const char *networkName,
>        * On failure clear it out, but don't free it.
>        */
>   
> -    xmlNodePtr cur, save;
> +    xmlNodePtr save;
> +    xmlNodePtr dhcp;
>       char *address = NULL, *netmask = NULL;
>       unsigned long prefix = 0;
>       int prefixRc;
> @@ -1603,29 +1604,20 @@ virNetworkIPDefParseXML(const char *networkName,
>           goto cleanup;
>       }
>   
> -    cur = node->children;
> -    while (cur != NULL) {
> -        if (cur->type == XML_ELEMENT_NODE &&
> -            xmlStrEqual(cur->name, BAD_CAST "dhcp")) {
> -            if (virNetworkDHCPDefParseXML(networkName, cur, def) < 0)
> -                goto cleanup;
> -        } else if (cur->type == XML_ELEMENT_NODE &&
> -                   xmlStrEqual(cur->name, BAD_CAST "tftp")) {
> -            char *root;
> +    if ((dhcp = virXPathNode("./dhcp[1]", ctxt)) &&
> +        virNetworkDHCPDefParseXML(networkName, dhcp, def) < 0)
> +        goto cleanup;
>   
> -            if (!VIR_SOCKET_ADDR_IS_FAMILY(&def->address, AF_INET)) {
> -                virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> -                               _("Unsupported <tftp> element in an IPv6 element in network '%s'"),
> -                               networkName);
> -                goto cleanup;
> -            }
> -            if (!(root = virXMLPropString(cur, "root"))) {
> -                cur = cur->next;
> -                continue;
> -            }
> -            def->tftproot = (char *)root;
> +    if (virXPathNode("./tftp[1]", ctxt)) {
> +        if (!VIR_SOCKET_ADDR_IS_FAMILY(&def->address, AF_INET)) {
> +            virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> +                           _("Unsupported <tftp> element in an IPv6 element "
> +                             "in network '%s'"),
> +                           networkName);
> +            goto cleanup;
>           }
> -        cur = cur->next;
> +
> +        def->tftproot = virXPathString("string(./tftp[1]/@root)", ctxt);
>       }
>   
>       result = 0;