[libvirt] [PATCH 0/2] Fix a couple problems encountered during virInterface testing
When testing all the different possibilities of interfaces with the new netcf release + my latest libvirt patches (ACKed, but not yet committed), I came across some problems, addressed by the following two patches. The first patch is for a bug that crept in due to a last minute change suggested in danpb's review - I missed changing a couple places to account for changing the initial value of "ret". It could alternately be squashed into this patch, which hasn't yet been committed: https://www.redhat.com/archives/libvir-list/2009-October/msg00689.html The second patch loosens up the parsing of vlan, bond, and bridge interface XML because the live config XML doesn't know how to get those items yet. I believe the necessary items can be learned and included by the time for the *next* libvirt release, but think that the new functionality there already is too useful (and too needy of more hands to test) to delay the entire set for that long.
--- src/conf/interface_conf.c | 14 ++++++++------ 1 files changed, 8 insertions(+), 6 deletions(-) diff --git a/src/conf/interface_conf.c b/src/conf/interface_conf.c index 7cb71ed..fc18eba 100644 --- a/src/conf/interface_conf.c +++ b/src/conf/interface_conf.c @@ -309,10 +309,11 @@ virInterfaceDefParseProtoIPv4(virConnectPtr conn, virInterfaceProtocolDefPtr def def->gateway = tmp; dhcp = virXPathNode(conn, "./dhcp", ctxt); - if (dhcp != NULL) + if (dhcp != NULL) { ret = virInterfaceDefParseDhcp(conn, def, dhcp, ctxt); - if (ret != 0) - return(ret); + if (ret != 0) + return(ret); + } nIpNodes = virXPathNodeSet(conn, "./ip", ctxt, &ipNodes); if (ipNodes == NULL) @@ -365,10 +366,11 @@ virInterfaceDefParseProtoIPv6(virConnectPtr conn, virInterfaceProtocolDefPtr def def->autoconf = 1; dhcp = virXPathNode(conn, "./dhcp", ctxt); - if (dhcp != NULL) + if (dhcp != NULL) { ret = virInterfaceDefParseDhcp(conn, def, dhcp, ctxt); - if (ret != 0) - return(ret); + if (ret != 0) + return(ret); + } nIpNodes = virXPathNodeSet(conn, "./ip", ctxt, &ipNodes); if (ipNodes == NULL) -- 1.6.5.15.gc274d
On Wed, Oct 28, 2009 at 03:25:43AM -0400, Laine Stump wrote:
--- src/conf/interface_conf.c | 14 ++++++++------ 1 files changed, 8 insertions(+), 6 deletions(-)
diff --git a/src/conf/interface_conf.c b/src/conf/interface_conf.c index 7cb71ed..fc18eba 100644 --- a/src/conf/interface_conf.c +++ b/src/conf/interface_conf.c @@ -309,10 +309,11 @@ virInterfaceDefParseProtoIPv4(virConnectPtr conn, virInterfaceProtocolDefPtr def def->gateway = tmp;
dhcp = virXPathNode(conn, "./dhcp", ctxt); - if (dhcp != NULL) + if (dhcp != NULL) { ret = virInterfaceDefParseDhcp(conn, def, dhcp, ctxt); - if (ret != 0) - return(ret); + if (ret != 0) + return(ret); + }
nIpNodes = virXPathNodeSet(conn, "./ip", ctxt, &ipNodes); if (ipNodes == NULL) @@ -365,10 +366,11 @@ virInterfaceDefParseProtoIPv6(virConnectPtr conn, virInterfaceProtocolDefPtr def def->autoconf = 1;
dhcp = virXPathNode(conn, "./dhcp", ctxt); - if (dhcp != NULL) + if (dhcp != NULL) { ret = virInterfaceDefParseDhcp(conn, def, dhcp, ctxt); - if (ret != 0) - return(ret); + if (ret != 0) + return(ret); + }
nIpNodes = virXPathNodeSet(conn, "./ip", ctxt, &ipNodes); if (ipNodes == NULL)
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 :|
This is necessary because netcf does not yet know how to get this information from the kernel, so the live config XML doesn't contain it, leading to errors when passing it through the current libvirt parsers. --- src/conf/interface_conf.c | 86 ++++++++++++++++++-------------------------- 1 files changed, 35 insertions(+), 51 deletions(-) diff --git a/src/conf/interface_conf.c b/src/conf/interface_conf.c index fc18eba..8926804 100644 --- a/src/conf/interface_conf.c +++ b/src/conf/interface_conf.c @@ -782,30 +782,27 @@ virInterfaceDefParseXML(virConnectPtr conn, xmlXPathContextPtr ctxt) { if (virInterfaceDefParseIfAdressing(conn, def, ctxt) < 0) goto error; - bridge = virXPathNode(conn, "./bridge[1]", ctxt); - if (bridge == NULL) { - virInterfaceReportError(conn, VIR_ERR_XML_ERROR, - "%s", _("bridge interface misses the bridge element")); - goto error; - } - tmp = virXMLPropString(bridge, "stp"); def->data.bridge.stp = -1; - if (tmp != NULL) { - if (STREQ(tmp, "on")) { - def->data.bridge.stp = 1; - } else if (STREQ(tmp, "off")) { - def->data.bridge.stp = 0; - } else { - virInterfaceReportError(conn, VIR_ERR_XML_ERROR, - _("bridge interface stp should be on or off got %s"), - tmp); + bridge = virXPathNode(conn, "./bridge[1]", ctxt); + if (bridge != NULL) { + tmp = virXMLPropString(bridge, "stp"); + if (tmp != NULL) { + if (STREQ(tmp, "on")) { + def->data.bridge.stp = 1; + } else if (STREQ(tmp, "off")) { + def->data.bridge.stp = 0; + } else { + virInterfaceReportError(conn, VIR_ERR_XML_ERROR, + _("bridge interface stp should be on or off got %s"), + tmp); + VIR_FREE(tmp); + goto error; + } VIR_FREE(tmp); - goto error; } - VIR_FREE(tmp); + ctxt->node = bridge; + virInterfaceDefParseBridge(conn, def, ctxt); } - ctxt->node = bridge; - virInterfaceDefParseBridge(conn, def, ctxt); break; } case VIR_INTERFACE_TYPE_BOND: { @@ -818,14 +815,11 @@ virInterfaceDefParseXML(virConnectPtr conn, xmlXPathContextPtr ctxt) { if (virInterfaceDefParseIfAdressing(conn, def, ctxt) < 0) goto error; bond = virXPathNode(conn, "./bond[1]", ctxt); - if (bond == NULL) { - virInterfaceReportError(conn, VIR_ERR_XML_ERROR, - "%s", _("bond interface misses the bond element")); - goto error; + if (bond != NULL) { + ctxt->node = bond; + if (virInterfaceDefParseBond(conn, def, ctxt) < 0) + goto error; } - ctxt->node = bond; - if (virInterfaceDefParseBond(conn, def, ctxt) < 0) - goto error; break; } case VIR_INTERFACE_TYPE_VLAN: { @@ -839,14 +833,11 @@ virInterfaceDefParseXML(virConnectPtr conn, xmlXPathContextPtr ctxt) { if (virInterfaceDefParseIfAdressing(conn, def, ctxt) < 0) goto error; vlan = virXPathNode(conn, "./vlan[1]", ctxt); - if (vlan == NULL) { - virInterfaceReportError(conn, VIR_ERR_XML_ERROR, - "%s", _("vlan interface misses the vlan element")); - goto error; + if (vlan != NULL) { + ctxt->node = vlan; + if (virInterfaceDefParseVlan(conn, def, ctxt) < 0) + goto error; } - ctxt->node = vlan; - if (virInterfaceDefParseVlan(conn, def, ctxt) < 0) - goto error; break; } @@ -1108,12 +1099,8 @@ virInterfaceBondDefFormat(virConnectPtr conn, virBufferPtr buf, else if (def->data.bond.validate == VIR_INTERFACE_BOND_ARP_ALL) virBufferAddLit(buf, " validate='all'"); virBufferAddLit(buf, "/>\n"); - } else { - virInterfaceReportError(conn, VIR_ERR_INTERNAL_ERROR, - _("bond monitoring type %d unknown"), - def->data.bond.monit); - return(-1); } + for (i = 0;i < def->data.bond.nbItf;i++) { if (virInterfaceBareDevDefFormat(conn, buf, def->data.bond.itf[i]) < 0) ret = -1; @@ -1126,20 +1113,17 @@ virInterfaceBondDefFormat(virConnectPtr conn, virBufferPtr buf, static int virInterfaceVlanDefFormat(virConnectPtr conn, virBufferPtr buf, const virInterfaceDefPtr def) { - if (def->data.vlan.tag == NULL) { - virInterfaceReportError(conn, VIR_ERR_INTERNAL_ERROR, - "%s", _("vlan misses the tag name")); - return(-1); + if (def->data.vlan.tag != NULL) { + virBufferVSprintf(buf, " <vlan tag='%s'", def->data.vlan.tag); + if (def->data.vlan.devname != NULL) { + virBufferAddLit(buf, ">\n"); + virBufferVSprintf(buf, " <interface name='%s'/>\n", + def->data.vlan.devname); + virBufferAddLit(buf, " </vlan>\n"); + } else + virBufferAddLit(buf, "/>\n"); } - virBufferVSprintf(buf, " <vlan tag='%s'", def->data.vlan.tag); - if (def->data.vlan.devname != NULL) { - virBufferAddLit(buf, ">\n"); - virBufferVSprintf(buf, " <interface name='%s'/>\n", - def->data.vlan.devname); - virBufferAddLit(buf, " </vlan>\n"); - } else - virBufferAddLit(buf, "/>\n"); return(0); } -- 1.6.5.15.gc274d
On Wed, Oct 28, 2009 at 03:25:44AM -0400, Laine Stump wrote:
This is necessary because netcf does not yet know how to get this information from the kernel, so the live config XML doesn't contain it, leading to errors when passing it through the current libvirt parsers.
Omitting this information makes the XML pretty useless. It is easy to get the name of the enslaved interface for a bond/bridge out of the sysfs, so I'd rather we just fixed netcf than make this stuff optional. 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 :|
participants (2)
-
Daniel P. Berrange -
Laine Stump