On Fri, Oct 16, 2009 at 10:56:17AM -0400, laine@laine.org wrote:

From: Laine Stump <laine@laine.org>

The minimal XML returned from ncf_if_xml_state() doesn't contain this attribute (which makes no sense in the case of reporting current status of the interface), and it was preventing it from passing through the parse/format step. --- src/conf/interface_conf.c | 11 +++++------ src/conf/interface_conf.h | 3 ++- 2 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/src/conf/interface_conf.c b/src/conf/interface_conf.c index e646351..540c931 100644 --- a/src/conf/interface_conf.c +++ b/src/conf/interface_conf.c @@ -128,12 +128,9 @@ virInterfaceDefParseStartMode(virConnectPtr conn, virInterfaceDefPtr def, char *tmp;

tmp = virXPathString(conn, "string(./start/@mode)", ctxt); - if (tmp == NULL) { - virInterfaceReportError(conn, VIR_ERR_XML_ERROR, - "%s", _("interface misses the start mode attribute")); - return(-1); - } - if (STREQ(tmp, "onboot")) + if (tmp == NULL) + def->startmode = VIR_INTERFACE_START_UNSPECIFIED; + else if (STREQ(tmp, "onboot")) def->startmode = VIR_INTERFACE_START_ONBOOT; else if (STREQ(tmp, "hotplug")) def->startmode = VIR_INTERFACE_START_HOTPLUG; @@ -1039,6 +1036,8 @@ virInterfaceStartmodeDefFormat(virConnectPtr conn, virBufferPtr buf, enum virInterfaceStartMode startmode) { const char *mode; switch (startmode) { + case VIR_INTERFACE_START_UNSPECIFIED: + return 0; case VIR_INTERFACE_START_NONE: mode = "none"; break; diff --git a/src/conf/interface_conf.h b/src/conf/interface_conf.h index bb9dce4..20d9771 100644 --- a/src/conf/interface_conf.h +++ b/src/conf/interface_conf.h @@ -48,7 +48,8 @@ VIR_ENUM_DECL(virInterface) /* types of start mode */

enum virInterfaceStartMode { - VIR_INTERFACE_START_NONE = 0, /* not defined */ + VIR_INTERFACE_START_UNSPECIFIED = 0, /* not given in config */ + VIR_INTERFACE_START_NONE, /* specified as not defined */ VIR_INTERFACE_START_ONBOOT, /* startup at boot */ VIR_INTERFACE_START_HOTPLUG, /* on hotplug */ };

Okay, I usually prefer when enum numbering is not shuffled in patches, but in that case that should have no consequence, so ACK, I'm pushing this, thanks ! 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/

On Fri, Oct 16, 2009 at 10:56:18AM -0400, laine@laine.org wrote:

From: Laine Stump <laine@laine.org>

This patch adds the flag VIR_INTERFACE_XML_INACTIVE to virInterfaceGetXMLDesc's flags. When it is*not* set (the default), the live interface info will be returned in the XML (in particular, the IP address(es) and netmask(s) will be retrieved by querying the interface directly, rather than reporting what's in the config file). The backend of this is in netcf's ncf_if_xml_state() function.

Note that after this patch is applied, netcf support will be omitted from the build unless you have netcf-devel-0.1.3 or better installed. --- configure.in | 2 +- include/libvirt/libvirt.h.in | 4 +++ libvirt.spec.in | 2 +- src/conf/interface_conf.c | 53 +++++++++++++++++++----------------------- src/interface/netcf_driver.c | 8 ++++- src/libvirt.c | 15 ++++++++--- tools/virsh.c | 10 ++++++- 7 files changed, 55 insertions(+), 39 deletions(-)

diff --git a/configure.in b/configure.in index 2f9db72..3402b4f 100644 --- a/configure.in +++ b/configure.in @@ -33,7 +33,7 @@ GNUTLS_REQUIRED="1.0.25" AVAHI_REQUIRED="0.6.0" POLKIT_REQUIRED="0.6" PARTED_REQUIRED="1.8.0" -NETCF_REQUIRED="0.0.1" +NETCF_REQUIRED="0.1.3"

dnl Checks for C compiler. AC_PROG_CC diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in index 6186d4e..7e75bee 100644 --- a/include/libvirt/libvirt.h.in +++ b/include/libvirt/libvirt.h.in @@ -930,6 +930,10 @@ virInterfacePtr virInterfaceLookupByMACString (virConnectPtr conn, const char* virInterfaceGetName (virInterfacePtr iface); const char* virInterfaceGetMACString (virInterfacePtr iface);

+typedef enum { + VIR_INTERFACE_XML_INACTIVE = 1 /* dump inactive interface information */ +} virInterfaceXMLFlags; + char * virInterfaceGetXMLDesc (virInterfacePtr iface, unsigned int flags); virInterfacePtr virInterfaceDefineXML (virConnectPtr conn, diff --git a/libvirt.spec.in b/libvirt.spec.in index 6cd0888..32596d4 100644 --- a/libvirt.spec.in +++ b/libvirt.spec.in @@ -308,7 +308,7 @@ BuildRequires: libcap-ng-devel >= 0.5.0 BuildRequires: libssh2-devel %endif %if %{with_netcf} -BuildRequires: netcf-devel +BuildRequires: netcf-devel >= 0.1.3 %endif

# Fedora build root suckage diff --git a/src/conf/interface_conf.c b/src/conf/interface_conf.c index 540c931..d46f7ac 100644 --- a/src/conf/interface_conf.c +++ b/src/conf/interface_conf.c @@ -279,22 +279,19 @@ virInterfaceDefParseIp(virConnectPtr conn, virInterfaceDefPtr def, static int virInterfaceDefParseProtoIPv4(virConnectPtr conn, virInterfaceDefPtr def, xmlXPathContextPtr ctxt) { - xmlNodePtr cur; - int ret; + xmlNodePtr dhcp, ip; + int ret = 0;

- cur = virXPathNode(conn, "./dhcp", ctxt); - if (cur != NULL) - ret = virInterfaceDefParseDhcp(conn, def, cur, ctxt); - else { - cur = virXPathNode(conn, "./ip", ctxt); - if (cur != NULL) - ret = virInterfaceDefParseIp(conn, def, cur, ctxt); - else { - virInterfaceReportError(conn, VIR_ERR_XML_ERROR, - "%s", _("interface miss dhcp or ip adressing")); - ret = -1; - } - } + dhcp = virXPathNode(conn, "./dhcp", ctxt); + if (dhcp != NULL) + ret = virInterfaceDefParseDhcp(conn, def, dhcp, ctxt); + + if (ret != 0) + return(ret); + + ip = virXPathNode(conn, "./ip", ctxt); + if (ip != NULL) + ret = virInterfaceDefParseIp(conn, def, ip, ctxt); return(ret); }

@@ -1012,20 +1009,18 @@ virInterfaceProtocolDefFormat(virConnectPtr conn ATTRIBUTE_UNUSED, virBufferAddLit(buf, " <dhcp peerdns='yes'/>\n"); else virBufferAddLit(buf, " <dhcp/>\n"); - } else { - /* theorically if we don't have dhcp we should have an address */ - if (def->proto.address != NULL) { - if (def->proto.prefix != 0) - virBufferVSprintf(buf, " <ip address='%s' prefix='%d'/>\n", - def->proto.address, def->proto.prefix); - else - virBufferVSprintf(buf, " <ip address='%s'/>\n", - def->proto.address); - } - if (def->proto.gateway != NULL) { - virBufferVSprintf(buf, " <route gateway='%s'/>\n", - def->proto.gateway); - } + } + if (def->proto.address != NULL) { + if (def->proto.prefix != 0) + virBufferVSprintf(buf, " <ip address='%s' prefix='%d'/>\n", + def->proto.address, def->proto.prefix); + else + virBufferVSprintf(buf, " <ip address='%s'/>\n", + def->proto.address); + } + if (def->proto.gateway != NULL) { + virBufferVSprintf(buf, " <route gateway='%s'/>\n", + def->proto.gateway); } virBufferAddLit(buf, " </protocol>\n"); return(0); diff --git a/src/interface/netcf_driver.c b/src/interface/netcf_driver.c index ca14fb0..b5c3664 100644 --- a/src/interface/netcf_driver.c +++ b/src/interface/netcf_driver.c @@ -326,7 +326,7 @@ cleanup: }

static char *interfaceGetXMLDesc(virInterfacePtr ifinfo, - unsigned int flags ATTRIBUTE_UNUSED) + unsigned int flags) { struct interface_driver *driver = ifinfo->conn->interfacePrivateData; struct netcf_if *iface = NULL; @@ -342,7 +342,11 @@ static char *interfaceGetXMLDesc(virInterfacePtr ifinfo, goto cleanup; }

- xmlstr = ncf_if_xml_desc(iface); + if ((flags & VIR_INTERFACE_XML_INACTIVE)) { + xmlstr = ncf_if_xml_desc(iface); + } else { + xmlstr = ncf_if_xml_state(iface); + } if (!xmlstr) { const char *errmsg, *details; int errcode = ncf_error(driver->netcf, &errmsg, &details); diff --git a/src/libvirt.c b/src/libvirt.c index 92a1eaa..c7d6bcc 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -6198,10 +6198,17 @@ virInterfaceGetMACString(virInterfacePtr iface) /** * virInterfaceGetXMLDesc: * @iface: an interface object - * @flags: an OR'ed set of extraction flags, not used yet + * @flags: an OR'ed set of extraction flags. Current valid bits: + * + * VIR_INTERFACE_XML_INACTIVE - return the static configuration, + * suitable for use redefining the + * interface via virInterfaceDefineXML() * - * Provide an XML description of the interface. The description may be reused - * later to redefine the interface with virInterfaceDefineXML(). + * Provide an XML description of the interface. If + * VIR_INTERFACE_XML_INACTIVE is set, the description may be reused + * later to redefine the interface with virInterfaceDefineXML(). If it + * is not set, the ip address and netmask will be the current live + * setting of the interface, not the settings from the config files. * * Returns a 0 terminated UTF-8 encoded XML instance, or NULL in case of error. * the caller must free() the returned value. @@ -6218,7 +6225,7 @@ virInterfaceGetXMLDesc(virInterfacePtr iface, unsigned int flags) virLibInterfaceError(NULL, VIR_ERR_INVALID_INTERFACE, __FUNCTION__); return (NULL); } - if (flags != 0) { + if ((flags & ~VIR_INTERFACE_XML_INACTIVE) != 0) { virLibInterfaceError(iface, VIR_ERR_INVALID_ARG, __FUNCTION__); goto error; } diff --git a/tools/virsh.c b/tools/virsh.c index 47122d5..322de30 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -2792,7 +2792,7 @@ cmdInterfaceEdit (vshControl *ctl, const vshCmd *cmd) char *doc = NULL; char *doc_edited = NULL; char *doc_reread = NULL; - int flags = 0; + int flags = VIR_INTERFACE_XML_INACTIVE;

if (!vshConnectionUsability(ctl, ctl->conn, TRUE)) goto cleanup; @@ -3326,6 +3326,7 @@ static const vshCmdInfo info_interface_dumpxml[] = {

static const vshCmdOptDef opts_interface_dumpxml[] = { {"interface", VSH_OT_DATA, VSH_OFLAG_REQ, gettext_noop("interface name or MAC address")}, + {"inactive", VSH_OT_BOOL, 0, gettext_noop("show inactive defined XML")}, {NULL, 0, 0, NULL} };

@@ -3335,6 +3336,11 @@ cmdInterfaceDumpXML(vshControl *ctl, const vshCmd *cmd) virInterfacePtr iface; int ret = TRUE; char *dump; + int flags = 0; + int inactive = vshCommandOptBool(cmd, "inactive"); + + if (inactive) + flags |= VIR_INTERFACE_XML_INACTIVE;

if (!vshConnectionUsability(ctl, ctl->conn, TRUE)) return FALSE; @@ -3342,7 +3348,7 @@ cmdInterfaceDumpXML(vshControl *ctl, const vshCmd *cmd) if (!(iface = vshCommandOptInterface(ctl, cmd, NULL))) return FALSE;

- dump = virInterfaceGetXMLDesc(iface, 0); + dump = virInterfaceGetXMLDesc(iface, flags); if (dump != NULL) { printf("%s", dump); free(dump); --

ACK Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|

(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); +} + +static +void virInterfaceProtocolDefFree(virInterfaceProtocolDefPtr def) { + int ii; + + if (def == NULL) + return; + for (ii = 0; ii < def->nips; ii++) { + virInterfaceIpDefFree(def->ips[ii]); + } + VIR_FREE(def->ips); + VIR_FREE(def->family); + VIR_FREE(def->gateway); + VIR_FREE(def); +} + void virInterfaceDefFree(virInterfaceDefPtr def) { - int i; + int i, pp; if (def == NULL) return; @@ -89,10 +111,11 @@ void virInterfaceDefFree(virInterfaceDefPtr def) break; } - VIR_FREE(def->proto.family); - VIR_FREE(def->proto.address); - VIR_FREE(def->proto.gateway); - + /* free all protos */ + for (pp = 0; pp < def->nprotos; pp++) { + virInterfaceProtocolDefFree(def->protos[pp]); + } + VIR_FREE(def->protos); VIR_FREE(def); } @@ -222,22 +245,22 @@ virInterfaceDefParseBondArpValid(virConnectPtr conn, xmlXPathContextPtr ctxt) { } static int -virInterfaceDefParseDhcp(virConnectPtr conn, virInterfaceDefPtr def, +virInterfaceDefParseDhcp(virConnectPtr conn, virInterfaceProtocolDefPtr def, xmlNodePtr dhcp, xmlXPathContextPtr ctxt) { xmlNodePtr save; char *tmp; int ret = 0; - def->proto.dhcp = 1; + def->dhcp = 1; save = ctxt->node; ctxt->node = dhcp; /* Not much to do in the current version */ tmp = virXPathString(conn, "string(./@peerdns)", ctxt); if (tmp) { if (STREQ(tmp, "yes")) - def->proto.peerdns = 1; + def->peerdns = 1; else if (STREQ(tmp, "no")) - def->proto.peerdns = 0; + def->peerdns = 0; else { virInterfaceReportError(conn, VIR_ERR_XML_ERROR, _("unknown dhcp peerdns value %s"), tmp); @@ -245,86 +268,208 @@ virInterfaceDefParseDhcp(virConnectPtr conn, virInterfaceDefPtr def, } VIR_FREE(tmp); } else - def->proto.peerdns = -1; + def->peerdns = -1; ctxt->node = save; return(ret); } static int -virInterfaceDefParseIp(virConnectPtr conn, virInterfaceDefPtr def, - xmlNodePtr ip ATTRIBUTE_UNUSED, xmlXPathContextPtr ctxt) { +virInterfaceDefParseIp(virConnectPtr conn, virInterfaceIpDefPtr def, + xmlXPathContextPtr ctxt) { int ret = 0; char *tmp; long l; - tmp = virXPathString(conn, "string(./ip[1]/@address)", ctxt); - def->proto.address = tmp; + tmp = virXPathString(conn, "string(./@address)", ctxt); + def->address = tmp; if (tmp != NULL) { - ret = virXPathLong(conn, "string(./ip[1]/@prefix)", ctxt, &l); + ret = virXPathLong(conn, "string(./@prefix)", ctxt, &l); if (ret == 0) - def->proto.prefix = (int) l; + def->prefix = (int) l; else if (ret == -2) { virInterfaceReportError(conn, VIR_ERR_XML_ERROR, "%s", _("Invalid ip address prefix value")); return(-1); } } - tmp = virXPathString(conn, "string(./route[1]/@gateway)", ctxt); - def->proto.gateway = tmp; return(0); } static int -virInterfaceDefParseProtoIPv4(virConnectPtr conn, virInterfaceDefPtr def, +virInterfaceDefParseProtoIPv4(virConnectPtr conn, virInterfaceProtocolDefPtr def, xmlXPathContextPtr ctxt) { - xmlNodePtr dhcp, ip; - int ret = 0; + xmlNodePtr dhcp; + xmlNodePtr *ipNodes = NULL; + int nIpNodes, ii, ret = -1; + char *tmp; + + tmp = virXPathString(conn, "string(./route[1]/@gateway)", ctxt); + def->gateway = tmp; dhcp = virXPathNode(conn, "./dhcp", ctxt); if (dhcp != NULL) ret = virInterfaceDefParseDhcp(conn, def, dhcp, ctxt); + if (ret != 0) + return(ret); + + nIpNodes = virXPathNodeSet(conn, "./ip", ctxt, &ipNodes); + if (ipNodes == NULL) + return 0; + + if (VIR_ALLOC_N(def->ips, nIpNodes) < 0) { + virReportOOMError(conn); + goto error; + } + + def->nips = 0; + for (ii = 0; ii < nIpNodes; ii++) { + + virInterfaceIpDefPtr ip; + if (VIR_ALLOC(ip) < 0) { + virReportOOMError(conn); + goto error; + } + + ctxt->node = ipNodes[ii]; + ret = virInterfaceDefParseIp(conn, ip, ctxt); + if (ret != 0) { + virInterfaceIpDefFree(ip); + goto error; + } + def->ips[def->nips++] = ip; + } + + ret = 0; + +error: + VIR_FREE(ipNodes); + return(ret); +} + +static int +virInterfaceDefParseProtoIPv6(virConnectPtr conn, virInterfaceProtocolDefPtr def, + xmlXPathContextPtr ctxt) { + xmlNodePtr dhcp, autoconf; + xmlNodePtr *ipNodes = NULL; + int nIpNodes, ii, ret = -1; + char *tmp; + + tmp = virXPathString(conn, "string(./route[1]/@gateway)", ctxt); + def->gateway = tmp; + + autoconf = virXPathNode(conn, "./autoconf", ctxt); + if (autoconf != NULL) + def->autoconf = 1; + + dhcp = virXPathNode(conn, "./dhcp", ctxt); + if (dhcp != NULL) + ret = virInterfaceDefParseDhcp(conn, def, dhcp, ctxt); if (ret != 0) return(ret); - ip = virXPathNode(conn, "./ip", ctxt); - if (ip != NULL) - ret = virInterfaceDefParseIp(conn, def, ip, ctxt); + nIpNodes = virXPathNodeSet(conn, "./ip", ctxt, &ipNodes); + if (ipNodes == NULL) + return 0; + + if (VIR_ALLOC_N(def->ips, nIpNodes) < 0) { + virReportOOMError(conn); + goto error; + } + + def->nips = 0; + for (ii = 0; ii < nIpNodes; ii++) { + + virInterfaceIpDefPtr ip; + + if (VIR_ALLOC(ip) < 0) { + virReportOOMError(conn); + goto error; + } + + ctxt->node = ipNodes[ii]; + ret = virInterfaceDefParseIp(conn, ip, ctxt); + if (ret != 0) { + virInterfaceIpDefFree(ip); + goto error; + } + def->ips[def->nips++] = ip; + } + + ret = 0; + +error: + VIR_FREE(ipNodes); return(ret); } static int virInterfaceDefParseIfAdressing(virConnectPtr conn, virInterfaceDefPtr def, xmlXPathContextPtr ctxt) { - xmlNodePtr cur, save; - int ret; + xmlNodePtr save; + xmlNodePtr *protoNodes = NULL; + int nProtoNodes, pp, ret = -1; char *tmp; - cur = virXPathNode(conn, "./protocol[1]", ctxt); - if (cur == NULL) - return(0); save = ctxt->node; - ctxt->node = cur; - tmp = virXPathString(conn, "string(./@family)", ctxt); - if (tmp == NULL) { - virInterfaceReportError(conn, VIR_ERR_XML_ERROR, - "%s", _("protocol misses the family attribute")); - ret = -1; - goto done; + + nProtoNodes = virXPathNodeSet(conn, "./protocol", ctxt, &protoNodes); + if (nProtoNodes <= 0) { + /* no protocols is an acceptable outcome */ + return 0; } - if (STREQ(tmp, "ipv4")) { - def->proto.family = tmp; - ret = virInterfaceDefParseProtoIPv4(conn, def, ctxt); - } else { - virInterfaceReportError(conn, VIR_ERR_XML_ERROR, - _("unsupported protocol family '%s'"), tmp); - ret = -1; - VIR_FREE(tmp); + + if (VIR_ALLOC_N(def->protos, nProtoNodes) < 0) { + virReportOOMError(conn); + goto error; } -done: + 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); @@ -999,30 +1144,44 @@ virInterfaceVlanDefFormat(virConnectPtr conn, virBufferPtr buf, static int virInterfaceProtocolDefFormat(virConnectPtr conn ATTRIBUTE_UNUSED, virBufferPtr buf, const virInterfaceDefPtr def) { - if (def->proto.family == NULL) - return(0); - virBufferVSprintf(buf, " <protocol family='%s'>\n", def->proto.family); - if (def->proto.dhcp) { - if (def->proto.peerdns == 0) - virBufferAddLit(buf, " <dhcp peerdns='no'/>\n"); - else if (def->proto.peerdns == 1) - virBufferAddLit(buf, " <dhcp peerdns='yes'/>\n"); - else - virBufferAddLit(buf, " <dhcp/>\n"); - } - if (def->proto.address != NULL) { - if (def->proto.prefix != 0) - virBufferVSprintf(buf, " <ip address='%s' prefix='%d'/>\n", - def->proto.address, def->proto.prefix); - else - virBufferVSprintf(buf, " <ip address='%s'/>\n", - def->proto.address); - } - if (def->proto.gateway != NULL) { - virBufferVSprintf(buf, " <route gateway='%s'/>\n", - def->proto.gateway); + int pp, ii; + + for (pp = 0; pp < def->nprotos; pp++) { + + virBufferVSprintf(buf, " <protocol family='%s'>\n", def->protos[pp]->family); + + if (def->protos[pp]->autoconf) { + virBufferAddLit(buf, " <autoconf/>\n"); + } + + if (def->protos[pp]->dhcp) { + if (def->protos[pp]->peerdns == 0) + virBufferAddLit(buf, " <dhcp peerdns='no'/>\n"); + else if (def->protos[pp]->peerdns == 1) + virBufferAddLit(buf, " <dhcp peerdns='yes'/>\n"); + else + virBufferAddLit(buf, " <dhcp/>\n"); + } + + for (ii = 0; ii < def->protos[pp]->nips; ii++) { + if (def->protos[pp]->ips[ii]->address != NULL) { + + virBufferVSprintf(buf, " <ip address='%s'", + def->protos[pp]->ips[ii]->address); + if (def->protos[pp]->ips[ii]->prefix != 0) { + virBufferVSprintf(buf, " prefix='%d'", + def->protos[pp]->ips[ii]->prefix); + } + virBufferAddLit(buf, "/>\n"); + } + } + if (def->protos[pp]->gateway != NULL) { + virBufferVSprintf(buf, " <route gateway='%s'/>\n", + def->protos[pp]->gateway); + } + + virBufferAddLit(buf, " </protocol>\n"); } - virBufferAddLit(buf, " </protocol>\n"); return(0); } diff --git a/src/conf/interface_conf.h b/src/conf/interface_conf.h index 20d9771..c5f1023 100644 --- a/src/conf/interface_conf.h +++ b/src/conf/interface_conf.h @@ -124,14 +124,23 @@ struct _virInterfaceVlanDef { char *devname; /* device name for vlan */ }; +typedef struct _virInterfaceIpDef virInterfaceIpDef; +typedef virInterfaceIpDef *virInterfaceIpDefPtr; +struct _virInterfaceIpDef { + char *address; /* ip address */ + int prefix; /* ip prefix */ +}; + + typedef struct _virInterfaceProtocolDef virInterfaceProtocolDef; typedef virInterfaceProtocolDef *virInterfaceProtocolDefPtr; struct _virInterfaceProtocolDef { - char *family; /* ipv4 only right now */ + char *family; /* ipv4 or ipv6 */ int dhcp; /* use dhcp */ int peerdns; /* dhcp peerdns ? */ - char *address; /* ip address */ - int prefix; /* ip prefix */ + int autoconf; /* only useful if family is ipv6 */ + int nips; + virInterfaceIpDefPtr *ips; /* ptr to array of ips[nips] */ char *gateway; /* route gateway */ }; @@ -152,8 +161,8 @@ struct _virInterfaceDef { virInterfaceBondDef bond; } data; - /* separated as we may allow multiple of those in the future */ - virInterfaceProtocolDef proto; + int nprotos; + virInterfaceProtocolDefPtr *protos; /* ptr to array of protos[nprotos] */ }; typedef struct _virInterfaceObj virInterfaceObj; -- 1.6.5.15.gc274d

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/

This is the only update to the patchset already sent. All other problems were either in netcf code, or will be cleaned up later. A patch has been sent to netcf-devel which, when applied, allows make check of libvirt plus the patch series to complete with no errors. There is still one issue, affecting bond interfaces, which is not revealed by make check. I'll describe that in a separate email. --- src/conf/interface_conf.c | 1 + 1 files changed, 1 insertions(+), 0 deletions(-) diff --git a/src/conf/interface_conf.c b/src/conf/interface_conf.c index fc18eba..31af957 100644 --- a/src/conf/interface_conf.c +++ b/src/conf/interface_conf.c @@ -58,6 +58,7 @@ void virInterfaceIpDefFree(virInterfaceIpDefPtr def) { if (def == NULL) return; VIR_FREE(def->address); + VIR_FREE(def); } static -- 1.6.5.15.gc274d

On Mon, Nov 02, 2009 at 08:34:50AM -0500, Laine Stump wrote:

This is the only update to the patchset already sent. All other problems were either in netcf code, or will be cleaned up later.

A patch has been sent to netcf-devel which, when applied, allows make check of libvirt plus the patch series to complete with no errors.

There is still one issue, affecting bond interfaces, which is not revealed by make check. I'll describe that in a separate email. --- src/conf/interface_conf.c | 1 + 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/src/conf/interface_conf.c b/src/conf/interface_conf.c index fc18eba..31af957 100644 --- a/src/conf/interface_conf.c +++ b/src/conf/interface_conf.c @@ -58,6 +58,7 @@ void virInterfaceIpDefFree(virInterfaceIpDefPtr def) { if (def == NULL) return; VIR_FREE(def->address); + VIR_FREE(def); }

Okay, applied ! I have the single netcf patch to avoid a crash, but when running the regression tests I still see: 9) Interface XML-2-XML bond ... FAILED 10) Interface XML-2-XML bond-arp ... FAILED FAIL: interfacexml2xmltest ===================== 12 of 43 tests failed ===================== make[3]: *** [check-TESTS] Error 1 So what am I missing ? 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/