On Fri, Apr 13, 2018 at 12:54 PM, Ben Pfaff <b...@ovn.org> wrote:
>
> On Wed, Apr 04, 2018 at 05:51:48PM -0700, Han Zhou wrote:
> > Address sets are automatically generated from corresponding port
> > groups, and can be used directly in ACL match conditions.
>
> Thanks!
>
> I think that sync_address_sets() has a memory leak, because I don't see
> any free() calls that match up with the xstrdup() calls.  (I'm not sure
> that the xstrdup() calls are actually needed.)

Oops! Yes, before freeing the ipv4_addrs and ipv6_addrs, I forgot to free
the strings in them. I will fix with V3.
The xstrdup() calls are still needed, since the strings there are from char
arrays (rather than dynamic char *) in the local variable laddrs, which is
destroyed in every iteration.

>
> I don't think the xcalloc() calls are needed.  I think you can just
> initialize ipv4_addrs and ipv6_addrs to NULL.

Sure!
>
> I think that sync_address_sets() is O(n**2) in n_ipv4_addrs and
> n_ipv6_addrs, because of the allocation strategy.  If port groups get
> big (and they will eventually even if they do not today, because scale)
> I think that it would be better to expand the allocations exponentially.

Sorry that I didn't find it a problem. For each group, there are i ports,
and for each port there are j address groups (e.g. mac ip1 ip2 ip3 ...),
and for each address group there are k addresses (some of them can be ipv4
and some can be ipv6).
Although there are nested loops, this implementation iterates each ipv4 and
ipv6 address only once, so if the total number of ipv4 and ipv6 addresses
are n, then it should be O(n). Please correct me if I missed something here.

Thanks for the review!
Han
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Reply via email to