[ovs-dev] [PATCH 3/3] ovn: Add address_set() support for ACLs.

[Han Zhou]

On Wednesday, April 6, 2016, Russell Bryant <russell at ovn.org
<javascript:_e(%7B%7D,'cvml','russell at ovn.org');>> wrote:

>
>
> On Tue, Apr 5, 2016 at 10:03 PM, Han Zhou <zhouhan at gmail.com> wrote:
>
>>
>>
>> On Tue, Apr 5, 2016 at 2:24 PM, Russell Bryant <russell at 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