Re: [libvirt] [PATCH 4/6] net: Remove dnsmasq and radvd files also when destroying transient nets

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.