On 12/23/2010 12:41 PM, Eric Blake wrote:
On 12/22/2010 11:58 AM, Laine Stump wrote:
At this point everything is already in place to make IPv6 happen, we just need to add a few rules, remove some checks for IPv4-only, and document the changes to the XML on the website. --- No changes from V1.
docs/formatnetwork.html.in | 35 +++++++-- Yeah - a patch series with documentation updates!
Heh. Truthfully I was scared you'd NAK it if I didn't update the docs :-P
static int networkAddGeneralIptablesRules(struct network_driver *driver, virNetworkObjPtr network) @@ -926,6 +985,11 @@ networkAddGeneralIptablesRules(struct network_driver *driver, goto err8; }
+ /* add IPv6 general rules, if needed */ + if (networkAddGeneralIp6tablesRules(driver, network)< 0) { + goto err8; Should this be err9, with a step that undoes the previous action when you get past the err8 failure point?
I had somehow convinced myself not (because networkAddGeneralIp6tablesRules() undoes itself if it fails), but of course that logic is wrong - it's the *previous* step that needs undoing, so you are absolutely correct. I've squashed in the appropriate call.
+ if (virAsprintf(&field, SYSCTL_PATH "/net/ipv6/conf/%s/disable_ipv6", + network->def->bridge)< 0) { ... + if (virFileWriteStr(field, "1", 0)< 0) { + virReportSystemError(errno, + _("cannot enable %s"), field); Misleading message; maybe "cannot write to %s to disable IPv6".
Yup.
@@ -1755,7 +1845,8 @@ cleanup: static int networkUndefine(virNetworkPtr net) { struct network_driver *driver = net->conn->networkPrivateData; virNetworkObjPtr network; - virNetworkIpDefPtr ipv4def; + virNetworkIpDefPtr ipdef; + int v4present = 0, v6present = 0; s/int/bool/
Okay.
@@ -1780,12 +1871,17 @@ static int networkUndefine(virNetworkPtr net) {
/* we only support dhcp on one IPv4 address per defined network */ for (ii = 0; - (ipv4def = virNetworkDefGetIpByIndex(network->def, AF_INET, ii)); + (ipdef = virNetworkDefGetIpByIndex(network->def, AF_UNSPEC, ii)); ii++) { - if (ipv4def->nranges || ipv4def->nhosts) - break; + if (VIR_SOCKET_IS_FAMILY(&ipdef->address, AF_INET)) { + if (ipdef->nranges || ipdef->nhosts) + v4present = 1; At first glance, this logic didn't sound right. You only set v4present if you found a dhcp interface, ignoring other ipv4 interfaces. Then again,...
+ } else if (VIR_SOCKET_IS_FAMILY(&ipdef->address, AF_INET6)) { + v6present = 1; + } } - if (ipv4def) { + + if (v4present) { dnsmasqContext *dctx = dnsmasqContextNew(network->def->name, DNSMASQ_STATE_DIR); you only use it to disable dnsmasq rather than all things related to IPv4, so maybe it would be better to rename the variable to dhcp_present instead of v4present.
Sure. I almost did that originally, and can't say why I didn't (I guess my left brain liked the symmetry of the names or something).
ACK with those nits addressed.