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

[Peter Krempa]

On 10/25/12 22:56, Laine Stump wrote:
> 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).

That was the way I implemented it at first. I know it's not documented 
in any way but deleting the lease file has one implication: Every time 
you destroy the network you are risking that your guests are being 
re-addressed. It can be fully expected while using DHCP without static 
leases, but I think of it as a pretty sweet feature and I remember 
addresses of some of my guests and I'm glad they get the same addresses 
every time. On the other hand, the static hosts file can be deleted when 
destroying the net every time without any problems.

>
> 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.
>
>

Agreed, I didn't think about this option.


Peter