On Thu, Sep 20, 2012 at 10:25:40PM -0400, Laine Stump wrote:

The dhcp range element is contained in the <dhcp> element of one of a network's <ip> elements. There can be multiple <range> elements. Because there are only two attributes (start and end), and those are exactly what you would use to identify a particular range, it doesn't really make sense to modify an existing element, so VIR_NETWORK_UPDATE_COMMAND_MODIFY isn't supported for this section, only ADD_FIRST, ADD_LAST, and DELETE.

Since virsh already has support for understanding all the defined sections, this new backend is automatically supported by virsh. You would use it like this:

virsh net-update mynet add ip-dhcp-range \ "<range start='1.2.3.4' end='1.2.3.20'/>" --live --config

The bridge driver also already supports all sections, so it's doing the correct thing in this case as well - since the dhcp range is placed on the dnsmasq commandline, the bridge driver recreates the dnsmasq commandline, and re-runs dnsmasq whenever a range is added/deleted (and AFFECT_LIVE is specified in the flags). --- src/conf/network_conf.c | 92 ++++++++++++++++++++++++++++++++++++++++++++++--- 1 file changed, 88 insertions(+), 4 deletions(-)

diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index f33834f..7fc559f 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c @@ -2494,14 +2494,98 @@ cleanup:

static int virNetworkDefUpdateIPDHCPRange(virNetworkDefPtr def, - unsigned int command ATTRIBUTE_UNUSED, + unsigned int command, int parentIndex ATTRIBUTE_UNUSED, - xmlXPathContextPtr ctxt ATTRIBUTE_UNUSED, + xmlXPathContextPtr ctxt, /* virNetworkUpdateFlags */ unsigned int fflags ATTRIBUTE_UNUSED) { - virNetworkDefUpdateNoSupport(def, "ip dhcp range"); - return -1; + int ii, ret = -1; + virNetworkIpDefPtr ipdef = virNetworkIpDefByIndex(def, parentIndex); + virNetworkDHCPRangeDef range; + + memset(&range, 0, sizeof(range)); + + if (virNetworkDefUpdateCheckElementName(def, ctxt->node, "range") < 0) + goto cleanup; + + /* ipdef is the ip element that needs its range array updated */ + if (!ipdef) + goto cleanup; + + /* parse the xml into a virNetworkDHCPRangeDef */ + if (command == VIR_NETWORK_UPDATE_COMMAND_MODIFY) { + + virReportError(VIR_ERR_NO_SUPPORT, "%s", + _("dhcp ranges cannot be modified, " + "only added or deleted")); + goto cleanup; + } + + if (virNetworkDHCPRangeDefParse(def->name, ctxt->node, &range) < 0) + goto cleanup; + + /* check if an entry with same name/address/ip already exists */ + for (ii = 0; ii < ipdef->nranges; ii++) { + if (virSocketAddrEqual(&range.start, &ipdef->ranges[ii].start) && + virSocketAddrEqual(&range.end, &ipdef->ranges[ii].end)) { + break; + } + } + + if ((command == VIR_NETWORK_UPDATE_COMMAND_ADD_FIRST) || + (command == VIR_NETWORK_UPDATE_COMMAND_ADD_LAST)) { + + if (ii < ipdef->nranges) { + char *startip = virSocketAddrFormat(&range.start); + char *endip = virSocketAddrFormat(&range.end); + + virReportError(VIR_ERR_OPERATION_INVALID, + _("there is an existing dhcp range entry in " + "network '%s' that matches " + "\"<range start='%s' end='%s'/>\""), + def->name, + startip ? startip : "unknown", + endip ? endip : "unknown"); + goto cleanup; + } + + /* add to beginning/end of list */ + if (VIR_REALLOC_N(ipdef->ranges, ipdef->nranges +1) < 0) { + virReportOOMError(); + goto cleanup; + } + + if (command == VIR_NETWORK_UPDATE_COMMAND_ADD_LAST) { + ipdef->ranges[ipdef->nranges] = range; + } else { /* implied (command == VIR_NETWORK_UPDATE_COMMAND_ADD_FIRST) */ + memmove(ipdef->ranges + 1, ipdef->ranges, + sizeof(*ipdef->ranges) * ipdef->nranges); + ipdef->ranges[0] = range; + } + ipdef->nranges++; + memset(&range, 0, sizeof(range)); + + } else if (command == VIR_NETWORK_UPDATE_COMMAND_DELETE) { + + if (ii == ipdef->nranges) { + virReportError(VIR_ERR_OPERATION_INVALID, + _("couldn't locate a matching dhcp range entry " + "in network '%s'"), def->name); + goto cleanup; + } + + /* remove it */ + /* NB: nothing to clear from a RangeDef that's being freed */ + memmove(ipdef->ranges + ii, ipdef->ranges + ii + 1, + sizeof(*ipdef->ranges) * (ipdef->nranges - ii - 1)); + ipdef->nranges--; + ignore_value(VIR_REALLOC_N(ipdef->ranges, ipdef->nranges)); + } + + ret = 0; +cleanup: + return ret; }

static int

That's okay, ACK, but we will need to fix the fact that passing a random unsupported command value returns 0 and not an error. i will push for rc2 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 Thu, Sep 20, 2012 at 10:25:41PM -0400, Laine Stump wrote:

portgroup elements are located in the toplevel of <network> objects. There can be multiple <portgroup> elements, and they each have a unique name attribute.

Add, delete, and modify are all supported for portgroup. When deleting a portgroup, only the name must be specified in the provided xml - all other attributes and subelements are ignored for the purposes of matching and existing portgroup.

The bridge driver and virsh already know about the portgroup element, so providing this backend should cause the entire stack to work. Note that in the case of portgroup, there is no external daemon based on the portgroup config, so nothing must be restarted.

It is important to note that guests make a copy of the appropriate network's portgroup data when they are started, so although an updated portgroup's configuration will have an affect on new guests started after the cahange, existing guests won't magically have their bandwidth changed, for example. If something like that is desired, it will take a lot of redesign work in the way network devices are setup (there is currently no link from the network back to the individual interfaces using it, much less from a portgroup within a network back to the individual interfaces). --- src/conf/network_conf.c | 85 +++++++++++++++++++++++++++++++++++++++++++++---- 1 file changed, 79 insertions(+), 6 deletions(-)

diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index 7fc559f..34487dd 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c @@ -2625,15 +2625,88 @@ virNetworkDefUpdateForwardPF(virNetworkDefPtr def, }

static int -virNetworkDefUpdatePortgroup(virNetworkDefPtr def, - unsigned int command ATTRIBUTE_UNUSED, +virNetworkDefUpdatePortGroup(virNetworkDefPtr def, + unsigned int command, int parentIndex ATTRIBUTE_UNUSED, - xmlXPathContextPtr ctxt ATTRIBUTE_UNUSED, + xmlXPathContextPtr ctxt, /* virNetworkUpdateFlags */ unsigned int fflags ATTRIBUTE_UNUSED) { - virNetworkDefUpdateNoSupport(def, "portgroup"); - return -1; + int ii, ret = -1; + virPortGroupDef portgroup; + + memset(&portgroup, 0, sizeof(portgroup)); + + if (virNetworkDefUpdateCheckElementName(def, ctxt->node, "portgroup") < 0) + goto cleanup; + + if (virNetworkPortGroupParseXML(&portgroup, ctxt->node, ctxt) < 0) + goto cleanup; + + /* check if a portgroup with same name already exists */ + for (ii = 0; ii < def->nPortGroups; ii++) { + if (STREQ(portgroup.name, def->portGroups[ii].name)) + break; + } + if (ii == def->nPortGroups && + ((command == VIR_NETWORK_UPDATE_COMMAND_MODIFY) || + (command == VIR_NETWORK_UPDATE_COMMAND_DELETE))) { + virReportError(VIR_ERR_OPERATION_INVALID, + _("couldn't find a portgroup entry " + "in network '%s' matching <portgroup name='%s'>"), + def->name, portgroup.name); + goto cleanup; + } else if (ii < def->nPortGroups && + ((command == VIR_NETWORK_UPDATE_COMMAND_ADD_FIRST) || + (command == VIR_NETWORK_UPDATE_COMMAND_ADD_LAST))) { + virReportError(VIR_ERR_OPERATION_INVALID, + _("there is an existing portgroup entry in " + "network '%s' that matches " + "\"<portgroup name='%s'>\""), + def->name, portgroup.name); + goto cleanup; + } + + if (command == VIR_NETWORK_UPDATE_COMMAND_MODIFY) { + + /* replace existing entry */ + virPortGroupDefClear(&def->portGroups[ii]); + def->portGroups[ii] = portgroup; + memset(&portgroup, 0, sizeof(portgroup)); + + } else if ((command == VIR_NETWORK_UPDATE_COMMAND_ADD_FIRST) || + (command == VIR_NETWORK_UPDATE_COMMAND_ADD_LAST)) { + + /* add to beginning/end of list */ + if (VIR_REALLOC_N(def->portGroups, def->nPortGroups +1) < 0) { + virReportOOMError(); + goto cleanup; + } + + if (command == VIR_NETWORK_UPDATE_COMMAND_ADD_LAST) { + def->portGroups[def->nPortGroups] = portgroup; + } else { /* implied (command == VIR_NETWORK_UPDATE_COMMAND_ADD_FIRST) */ + memmove(def->portGroups + 1, def->portGroups, + sizeof(*def->portGroups) * def->nPortGroups); + def->portGroups[0] = portgroup; + } + def->nPortGroups++; + memset(&portgroup, 0, sizeof(portgroup)); + + } else if (command == VIR_NETWORK_UPDATE_COMMAND_DELETE) { + + /* remove it */ + virPortGroupDefClear(&def->portGroups[ii]); + memmove(def->portGroups + ii, def->portGroups + ii + 1, + sizeof(*def->portGroups) * (def->nPortGroups - ii - 1)); + def->nPortGroups--; + ignore_value(VIR_REALLOC_N(def->portGroups, def->nPortGroups)); + } + + ret = 0; +cleanup: + virPortGroupDefClear(&portgroup); + return ret; }

static int @@ -2719,7 +2792,7 @@ virNetworkDefUpdateSection(virNetworkDefPtr def, parentIndex, ctxt, flags); break; case VIR_NETWORK_SECTION_PORTGROUP: - ret = virNetworkDefUpdatePortgroup(def, command, + ret = virNetworkDefUpdatePortGroup(def, command, parentIndex, ctxt, flags); break; case VIR_NETWORK_SECTION_DNS_HOST:

ACK too, but same remark as for patch 1 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/