3:56 p.m.
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.