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

[Laine Stump]

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.