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

[Babu Shanmugam]

Hi Flavio,
Thanks for reviewing. My comments are below.

On Thursday 23 June 2016 10:33 PM, Flaviof wrote:
>
>
> On Thu, Jun 23, 2016 at 1:05 AM, <bschanmu at redhat.com 
> <mailto:bschanmu at redhat.com>> wrote:
>
>     From: Russell Bryant <russell at ovn.org <mailto:russell at ovn.org>>
>
>     +/* 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)
>
>
> - make both parameters const  (const struct sbrec_address_set *)
>
> - why not call these addr_set1 and addr_set2 ?
>
I agree. addr_set1 and addr_set2 is more readable.
>
>
>     +static void
>     +update_address_sets(struct controller_ctx *ctx)
>     +{
>     +    /* Remember the names of all address sets currently in
>     expr_address_sets
>     +     * so we can detect address sets that have been deleted. */
>     +    struct sset cur_address_sets =
>     SSET_INITIALIZER(&cur_address_sets);
>
>
> This sset is not an address_set, but address_set names (or keys).
> How about renaming this variable to cur_address_set_names ?
>
I agree.
>
>     +
>     +
>     +/* OVN_Northbound and OVN_Southbound have an identical
>     Address_Set table.
>     + * We always update OVN_Southbound to match the current data in
>     + * OVN_Northbound. */
>     +static void
>     +sync_address_sets(struct northd_context *ctx)
>     +{
>     +    struct shash sb_address_sets =
>     SHASH_INITIALIZER(&sb_address_sets);
>     +
>     +    const struct sbrec_address_set *sb_address_set;
>     +    SBREC_ADDRESS_SET_FOR_EACH (sb_address_set, ctx->ovnsb_idl) {
>     +        shash_add(&sb_address_sets, sb_address_set->name,
>     sb_address_set);
>     +    }
>     +
>     +    const struct nbrec_address_set *nb_address_set;
>     +    NBREC_ADDRESS_SET_FOR_EACH (nb_address_set, ctx->ovnnb_idl) {
>     +        sb_address_set = shash_find_and_delete(&sb_address_sets,
>     +  nb_address_set->name);
>     +        if (!sb_address_set) {
>     +            sb_address_set =
>     sbrec_address_set_insert(ctx->ovnsb_txn);
>     + sbrec_address_set_set_name(sb_address_set, nb_address_set->name);
>     +        }
>     +
>
>
> I'm still wrapping my head around ctx and txn but I thought of asking 
> a dumb question here:
> In cases when hash_find_and_delete returns a non-null sb_address_set, 
> is there a need
> to clean anything up before calling sbrec_address_set_set_addresses? 
> If so, this override may be causing a leak?
I think, shash_find_and_delete returns a db rec. It seems to me that 
there is no need for a cleanup. I will confirm it anyways. Thanks for 
pointing it out.

>
>
>     +  <table name="Address_Set" title="Address Sets">
>     +    <p>
>     +      Each row in this table represents a named set of addresses.
>     +      An address set may contain MAC, IPv4, or IPv6 addresses.
>
>
> ... and also IPv4 + IPv6 cidr (w.x.y.z/N), right?
Yes. I will update the documentation to include these as well. Thanks.
>
>     diff --git a/tests/ovn.at <http://ovn.at> b/tests/ovn.at
>     <http://ovn.at>
>     index 4f72107..59f9307 100644
>     --- a/tests/ovn.at <http://ovn.at>
>     +++ b/tests/ovn.at <http://ovn.at>
>     @@ -649,6 +649,8 @@ done
>      ovn-nbctl acl-add lsw0 from-lport 1000 'eth.type == 0x1234' drop
>      ovn-nbctl acl-add lsw0 from-lport 1000 'eth.type == 0x1235 &&
>     inport == "lp11"' drop
>      ovn-nbctl acl-add lsw0 to-lport 1000 'eth.type == 0x1236 &&
>     outport == "lp33"' drop
>     +ovn-nbctl create Address_Set name=set1
>     addresses=\"f0:00:00:00:00:11\",\"f0:00:00:00:00:21\",\"f0:00:00:00:00:31\"
>     +ovn-nbctl acl-add lsw0 to-lport 1000 'eth.type == 0x1237 &&
>     eth.src == $set1 && outport == "lp33"' drop
>
>
>
> it would be good to add some more tests to exercise the delete and 
> update codepaths.
> If you think it would help, I could take a stab at them.
I agree. I even saw your pull request. Thanks.

Thank you,
Babu