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@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
