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

[Laine Stump]

On 10/25/2012 05:06 PM, Peter Krempa wrote:
> 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.

Good point. I retract that suggestion.

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