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@xxxxxxxxxxxx | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library http://libvirt.org/
--
Libvir-list mailing list
Libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list