On 10/25/2012 11:18 AM, Peter Krempa wrote:
The network driver didn't care about config files when a network was destroyed, just when it was undefined leaving behind files for transient networks.
This patch splits out the cleanup code to a helper function that handles the cleanup if the inactive network object is being removed and re-uses this code when getting rid of inactive networks. --- src/network/bridge_driver.c | 133 +++++++++++++++++++++++++------------------- 1 file changed, 76 insertions(+), 57 deletions(-)
diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index 976c132..45fca0e 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -155,6 +155,67 @@ networkRadvdConfigFileName(const char *netname) return configfile; }
+/* do needed cleanup steps and remove the network from the list */ +static int +networkRemoveInactive(struct network_driver *driver, + virNetworkObjPtr net) +{ + char *leasefile = NULL; + char *radvdconfigfile = NULL; + char *radvdpidbase = NULL; + dnsmasqContext *dctx = NULL; + virNetworkIpDefPtr ipdef; + virNetworkDefPtr def = virNetworkObjGetPersistentDef(net); + + int ret = -1; + int i; + + /* we only support dhcp on one IPv4 address per defined network */ + for (i = 0; + (ipdef = virNetworkDefGetIpByIndex(def, AF_UNSPEC, i)); + i++) { + if (VIR_SOCKET_ADDR_IS_FAMILY(&ipdef->address, AF_INET) && + (ipdef->nranges || ipdef->nhosts)) { + /* clean up files left over by dnsmasq */ + if (!(dctx = dnsmasqContextNew(def->name, DNSMASQ_STATE_DIR))) + goto cleanup; + + if (!(leasefile = networkDnsmasqLeaseFileName(def->name))) + goto cleanup; + + dnsmasqDelete(dctx); + unlink(leasefile);
A couple of things about this: 1) I think it would be fine to do all of these deletes anytime a network is destroyed, not just when it's undefined (although I guess it's possible someone might rely on this behavior, it's not documented and not part of the API (and I don't know why they would rely on it anyway). 2) Since we're not checking for errors anyway, I think we should just always try the deletes/unlinks - it's possible that the configuration of the network changed during its lifetime and the IP addresses/ranges/hosts/whatever were deleted, so that now the network isn't doing dhcp, but it was in the past and has stale files left around.
+ + } else if (VIR_SOCKET_ADDR_IS_FAMILY(&ipdef->address, AF_INET6)) { + /* clean up files left over by radvd */ + + if (!(radvdconfigfile = networkRadvdConfigFileName(def->name))) + goto no_memory; + + if (!(radvdpidbase = networkRadvdPidfileBasename(def->name))) + goto no_memory; + + unlink(radvdconfigfile); + virPidFileDelete(NETWORK_PID_DIR, radvdpidbase);
Same here. I think we should just always attempt these operations, whether there are any ipv6 addresses or not. (anyway, the way it's written now, if you had 5 IPv6 addresses on the network, you would be attempting the same unlinks 5 times.)
+ } + } + + virNetworkRemoveInactive(&driver->networks, net); + + ret = 0; + +cleanup: + VIR_FREE(leasefile); + VIR_FREE(radvdconfigfile); + VIR_FREE(radvdpidbase); + dnsmasqContextFree(dctx); + return ret; + +no_memory: + virReportOOMError(); + goto cleanup; +} + static char * networkBridgeDummyNicName(const char *brname) { @@ -2806,12 +2867,11 @@ cleanup: return ret; }
-static int networkUndefine(virNetworkPtr net) { +static int +networkUndefine(virNetworkPtr net) { struct network_driver *driver = net->conn->networkPrivateData; virNetworkObjPtr network; - virNetworkIpDefPtr ipdef; - bool dhcp_present = false, v6present = false; - int ret = -1, ii; + int ret = -1;
networkDriverLock(driver);
@@ -2833,58 +2893,12 @@ static int networkUndefine(virNetworkPtr net) { network) < 0) goto cleanup;
- /* we only support dhcp on one IPv4 address per defined network */ - for (ii = 0; - (ipdef = virNetworkDefGetIpByIndex(network->def, AF_UNSPEC, ii)); - ii++) { - if (VIR_SOCKET_ADDR_IS_FAMILY(&ipdef->address, AF_INET)) { - if (ipdef->nranges || ipdef->nhosts) - dhcp_present = true; - } else if (VIR_SOCKET_ADDR_IS_FAMILY(&ipdef->address, AF_INET6)) { - v6present = true; - } - } - - if (dhcp_present) { - char *leasefile; - dnsmasqContext *dctx = dnsmasqContextNew(network->def->name, DNSMASQ_STATE_DIR); - if (dctx == NULL) - goto cleanup; - - dnsmasqDelete(dctx); - dnsmasqContextFree(dctx); - - leasefile = networkDnsmasqLeaseFileName(network->def->name); - if (!leasefile) - goto cleanup; - unlink(leasefile); - VIR_FREE(leasefile); - } - - if (v6present) { - char *configfile = networkRadvdConfigFileName(network->def->name); - - if (!configfile) { - virReportOOMError(); - goto cleanup; - } - unlink(configfile); - VIR_FREE(configfile); - - char *radvdpidbase = networkRadvdPidfileBasename(network->def->name); - - if (!(radvdpidbase)) { - virReportOOMError(); - goto cleanup; - } - virPidFileDelete(NETWORK_PID_DIR, radvdpidbase); - VIR_FREE(radvdpidbase); - + VIR_INFO("Undefining network '%s'", network->def->name); + if (networkRemoveInactive(driver, network) < 0) { + network = NULL; + goto cleanup; }
- VIR_INFO("Undefining network '%s'", network->def->name); - virNetworkRemoveInactive(&driver->networks, - network); network = NULL; ret = 0;
@@ -3085,10 +3099,15 @@ static int networkDestroy(virNetworkPtr net) { goto cleanup; }
- ret = networkShutdownNetwork(driver, network); + if ((ret = networkShutdownNetwork(driver, network)) < 0) + goto cleanup; + if (!network->persistent) { - virNetworkRemoveInactive(&driver->networks, - network); + if (networkRemoveInactive(driver, network) < 0) { + network = NULL; + ret = -1; + goto cleanup; + } network = NULL; }
So, definitely you should do (2) above, and I think you should also do (1), but if you want to do that separately/later, that's okay too.