[libvirt] [PATCHv3 1/2] network: added waiting for DAD to finish for bridge address.

Maxim Perevedentsev mperevedentsev at virtuozzo.com
Mon Oct 19 13:15:06 UTC 2015


Some more comments.

On 10/15/2015 09:03 PM, Laine Stump wrote:
>>   static int
>> +networkWaitDadFinish(virNetworkObjPtr network)
>> +{
>> +    virNetworkIpDefPtr ipdef;
>> +    virSocketAddrPtr *addrs = NULL;
>> +    size_t i;
>> +    int ret;
>> +    for (i = 0;
>> +         (ipdef = virNetworkDefGetIpByIndex(network->def, AF_INET6, 
>> i));
>> +         i++) {}
>> +
>> +    if (i == 0)
>> +        return 0;
>> +    if (VIR_ALLOC_N(addrs, i))
>> +        return -1;
>> +
>> +    for (i = 0;
>> +         (ipdef = virNetworkDefGetIpByIndex(network->def, AF_INET6, 
>> i));
>> +         i++) {
>> +        addrs[i] = &ipdef->address;
>> +    }
>
> This code could be much more compact (and only very slightly less 
> efficient) by using something like:
>
>    virNetworkIpDefPtr ipdef;
>    virSocketAddrPtr addrs = NULL;
>    size_t i, naddrs = 0;
>    for (i = 0; (ipdef == virNetworkDefGetIpByIndex(...); i++) {
>        if (VIR_APPEND_ELEMENT_COPY(addrs, naddrs, ipdef->address) < 0)
>            return -1;
>    }
>    if (naddrs == 0)
>        return 0;
>
>
> addrs would then be a pointer to an array of addresses rather than a 
> pointer to an array of pointers to addresses, so the lower functions 
> would need to be adjusted accordingly.
>
> (NB: due to the switch from stuff** to stuff*, there would be fewer 
> mallocs than your existing code, although at each malloc you could 
> potentially incur the penalty of a copy of all existing elements. For 
> the size of array we're talking about though, this is inconsequential 
> (especially since any good malloc implementation will carve out memory 
> in larger chunks, and so will not need to do a copy each time the 
> region is realloced).
The only malloc is allocating the array of pointers. I don't actually 
copy virSocketAddr's with in6_addr inside, I just store pointers to IPv6 
addresses in external structures.

In your snippet, we copy virSocketAddr on each iteration and, possibly, 
everything copied as far on relocation.
Isn't it better to copy pointers?

virSocketAddrPtr *addrs = NULL, addr = NULL;

for (i = 0; (ipdef == virNetworkDefGetIpByIndex(...); i++) {
     addr = &ipdef->address;
     if (VIR_APPEND_ELEMENT_COPY(addrs, naddrs, addr) < 0)
         return -1;
}
...

>> +    VIR_FREE(addrs);
>
> The way you have this now, the above leaks the memory pointed to by 
> each element in the array. (once you switch from virSocketAddrPtr* to 
> virSocketAddr* this would no longer be a problem).
The addresses themselves are external structures. We do not want to 
destroy them, just remove pointers.

-- 
Your sincerely,
Maxim Perevedentsev




More information about the libvir-list mailing list