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@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/