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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list



[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]