On Tue, Apr 5, 2016 at 10:03 PM, Han Zhou <zhou...@gmail.com> wrote:
>
>
> On Tue, Apr 5, 2016 at 2:24 PM, Russell Bryant <russ...@ovn.org> wrote:
> >> +/* Return true if the address sets match, false otherwise. */
> > +static bool
> > +address_sets_match(struct address_set *addr_set,
> > + const struct sbrec_address_set *addr_set_rec)
> > +{
> > + char **addrs1;
> > + char **addrs2;
> > +
> > + if (addr_set->n_addresses != addr_set_rec->n_addresses) {
> > + return false;
> > + }
> > + size_t n_addresses = addr_set->n_addresses;
> > +
> > + addrs1 = xmemdup(addr_set->addresses,
> > + n_addresses * sizeof addr_set->addresses[0]);
> > + addrs2 = xmemdup(addr_set_rec->addresses,
> > + n_addresses * sizeof addr_set_rec->addresses[0]);
>
> This xmemdup might have some problems. Firstly, IPv6 address string size
> is variant, so we cannot use the size of the first element to calculate the
> total size. Secondly, since it is configurable, there can be mistakes in
> the address format, or someone can even attack purposely.
>
It's not really obvious what's going on here, but it's not duplicating the
actual strings. it's duplicating an array of pointers so that we can sort
them. It's still pointing to the original strings.
The same pattern is used in ovn-nbctl and ovn-sbctl.
> Thanks Russell. I haven't yet completed the review, just some comments
> inlined. One additional thing came to my mind is that, even if it is much
> more efficent than having IP addresses on each ACL, it is still O(nLogn)
> considering the comparisons when handling the address updates. Ideally it
> should be O(n) for adding/removing an IP in a set, but I understand that we
> don't have the semantics for adding/deleting an element in a Set column.
> Not sure if this would still create scale problems when the list grows to
> hundreds or thousands of addresses. I just raise the concern, but no good
> ideas yet.
>
Do you mean from the Neutron plugin? Being able to say "add this element
to a set" would be great. It has come up before. I was expecting it would
get added at some point. I copied Andy on this point.
The code in ovn-controller could certainly be a lot more efficient as well
since we're having to do a full comparison of sets to see if anything has
changed. Change tracking would help here, especially since it's well
isolated. I opted for the less efficient route to start with to make sure
we at least started with something that we knew worked.
--
Russell Bryant
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
