[libvirt] [PATCH 3/3] (REVISED) Support for IPv6 / multiple addresses per interface in virInterface

[Daniel Veillard]

On Fri, Oct 23, 2009 at 01:31:19PM -0400, Laine Stump wrote:
> (How's this?)
> 
> This patch updates the xml parsing and formatting, and the associated
> virInterfaceDef data structure to support IPv6, along the way adding
> support for multiple protocols per interface, and multiple IP
> addresses per protocol.
> ---
>  src/conf/interface_conf.c |  295 ++++++++++++++++++++++++++++++++++-----------
>  src/conf/interface_conf.h |   19 ++-
>  2 files changed, 241 insertions(+), 73 deletions(-)
> 
> diff --git a/src/conf/interface_conf.c b/src/conf/interface_conf.c
> index d46f7ac..7cb71ed 100644
> --- a/src/conf/interface_conf.c
> +++ b/src/conf/interface_conf.c
> @@ -53,9 +53,31 @@ void virInterfaceBareDefFree(virInterfaceBareDefPtr def) {
>      VIR_FREE(def);
>  }
>  
> +static
> +void virInterfaceIpDefFree(virInterfaceIpDefPtr def) {
> +    if (def == NULL)
> +        return;
> +    VIR_FREE(def->address);
> +}

  Hum ... shouldn't def be freed too there ???

[...]
> +    def->nips = 0;
> +    for (ii = 0; ii < nIpNodes; ii++) {
> +
> +        virInterfaceIpDefPtr ip;
>  
> +        if (VIR_ALLOC(ip) < 0) {
> +            virReportOOMError(conn);
> +            goto error;
> +        }

  hum, yes I really think the matching FREE is missing, but let's do
  that as a separate patch

> +        ctxt->node = ipNodes[ii];
> +        ret = virInterfaceDefParseIp(conn, ip, ctxt);
> +        if (ret != 0) {
> +            virInterfaceIpDefFree(ip);
> +            goto error;

  Hum ....
> +        }
> +        def->ips[def->nips++] = ip;
> +    }
> +
> +    ret = 0;
> +
> +error:
> +    VIR_FREE(ipNodes);
> +    return(ret);
> +}

  It's hard to be sure we aren't leaking there too. On second reading
  this looks okay because def is passed by the caller and we are storing
  the data there.

[...]
> +    nProtoNodes = virXPathNodeSet(conn, "./protocol", ctxt, &protoNodes);
> +    if (nProtoNodes <= 0) {
> +        /* no protocols is an acceptable outcome */
> +        return 0;
>      }
> +
> +    if (VIR_ALLOC_N(def->protos, nProtoNodes) < 0) {
> +        virReportOOMError(conn);
> +        goto error;
>      }
>  
> +    def->nprotos = 0;
> +    for (pp = 0; pp < nProtoNodes; pp++) {
> +
> +        virInterfaceProtocolDefPtr proto;
> +
> +        if (VIR_ALLOC(proto) < 0) {
> +            virReportOOMError(conn);
> +            goto error;
> +        }
> +
> +        ctxt->node = protoNodes[pp];
> +        tmp = virXPathString(conn, "string(./@family)", ctxt);
> +        if (tmp == NULL) {
> +            virInterfaceReportError(conn, VIR_ERR_XML_ERROR,
> +                                    "%s", _("protocol misses the family attribute"));
> +            virInterfaceProtocolDefFree(proto);
> +            goto error;
> +        }
> +        proto->family = tmp;
> +        if (STREQ(tmp, "ipv4")) {
> +            ret = virInterfaceDefParseProtoIPv4(conn, proto, ctxt);
> +            if (ret != 0) {
> +                virInterfaceProtocolDefFree(proto);
> +                goto error;
> +            }
> +        } else if (STREQ(tmp, "ipv6")) {
> +            ret = virInterfaceDefParseProtoIPv6(conn, proto, ctxt);
> +            if (ret != 0) {
> +                virInterfaceProtocolDefFree(proto);
> +                goto error;
> +            }
> +        } else {
> +            virInterfaceReportError(conn, VIR_ERR_XML_ERROR,
> +                                    _("unsupported protocol family '%s'"), tmp);
> +            virInterfaceProtocolDefFree(proto);
> +            goto error;
> +        }
> +        def->protos[def->nprotos++] = proto;
> +    }
> +
> +    ret = 0;
> +
> +error:
> +    VIR_FREE(protoNodes);
>      ctxt->node = save;
>      return(ret);

    Hum, we don't check there that an ipv6/4 protocol is not defined
    multiple time. Maybe this could be slightly refactored to search
for 1 protocol node with type IPv4 and the one protocol node for type
Ipv6 something like
   v4 = virXPathNode(conn, "./protocol[@family = 'ipv4']", ctxt)
   v6 = virXPathNode(conn, "./protocol[@family = 'ipv6']", ctxt)

   and drop the loop.
   But current code works too, just does less checking IMHO.


I think we need to patch the previous leak.
But the main problem I faced when running the regression tests.
I had installed netcf-0.1.3 and a number of thing just break with
this patch. For example when libvirtd exits in the 
  "testing with corrupted config" libvirtd it just segfaults:

Starting program: /u/veillard/libvirt/daemon/libvirtd --config=in1.conf
[...]
Program received signal SIGSEGV, Segmentation fault.
drv_close (ncf=0x7f0740) at drv_initscripts.c:511
511     xsltFreeStylesheet(ncf->driver->get);
(gdb) p ncf->driver
$2 = (struct driver *) 0x0
(gdb) p *ncf
$3 = {ref = 1, root = 0x7f0780 "/", 
  data_dir = 0x7ffff77626b8 "/usr/share/netcf", errcode = NETCF_NOERROR, 
  errdetails = 0x0, driver = 0x0, debug = 0}
(gdb) 

  IMHO it's a bug in netcf since it should protect against acessing
uninitialized fields. The libvirtd side is:

    /* open netcf */
    if (ncf_init(&driverState->netcf, NULL) != 0)
    {
        /* what error to report? */
        goto netcf_error;
    }

    conn->interfacePrivateData = driverState;
    return 0;

netcf_error:
    if (driverState->netcf)
    {
        ncf_close(driverState->netcf);
    }

  maybe if ncf_init() failed we should not call ncf_close()
but we can see that there is at least a string in that driver which need
to be deallocated.

  I ran the test on Fedora 11 updated with netcf-0.1.3:

What I don't understand is why that patch affecting only
src/conf/interface_conf.[ch] is the one breaking this...
Hum, I reverted that patch and still get the error, maybe it's just
the netcf update ! I will try to clarify where things went wrong.
I also noted that the interface XML parsing regression tests also failed
which might be normal though since no reg test was updated.

Daniel

-- 
Daniel Veillard      | libxml Gnome XML XSLT toolkit  http://xmlsoft.org/
daniel at veillard.com  | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library  http://libvirt.org/