On Wednesday, April 6, 2016, Russell Bryant <russ...@ovn.org
<javascript:_e(%7B%7D,'cvml','russ...@ovn.org');>> wrote:
>
>
> 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.
>
Sorry, I misinterpreted the code :(
>
> 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.
>
I meant both neutron plugin and in ovn itself. I think it would help in
both cases. But in ovn more changes are required to make it O(1): physical
flow translation for address-set needs to be incremental, which is not a
small change.
>
> 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.
>
Completely understand!
> --
> Russell Bryant
>
--
Best regards,
Han
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
