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

[Laine Stump]

On 10/19/2015 09:15 AM, Maxim Perevedentsev wrote:
> 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.


Derp. You're right. I wasn't thinking straight when I wrote that, just 
looking at what the data structure was.


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


I still prefer the simpler code though, because there are less lines for 
a future maintainer to understand, and it's not going to add much to 
execution time unless there are hundreds of IPv6 addresses set for a 
single network. As it seems unlikely there will be more than one or two, 
the extra code complexity doesn't seem worthwhile.


(If we thought it would be common to have hundreds or thousands of IP 
addresses for each network, then I would rethink my position).


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

Yep. See above.