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.
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list