6:06 a.m.
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(a)veillard.com | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library http://libvirt.org/