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

[Babu Shanmugam]

Hi Zong Kai LI,
My comments are in-lined below.

On Monday 27 June 2016 08:50 PM, Zong Kai LI wrote:
>
>     +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_addr_set_names =
>     SSET_INITIALIZER(&cur_addr_set_names);
>     +
>     +    struct shash_node *node;
>     +    SHASH_FOR_EACH (node, &local_address_sets) {
>     +        sset_add(&cur_addr_set_names, node->name);
>     +    }
>     +
>     +    /* Iterate address sets in the southbound database. Create
>     and update the
>     +     * corresponding symtab entries as necessary. */
>     +    const struct sbrec_address_set *addr_set_rec;
>     +    SBREC_ADDRESS_SET_FOR_EACH (addr_set_rec, ctx->ovnsb_idl) {
>     +        node = shash_find(&local_address_sets, addr_set_rec->name);
>     +        struct address_set *addr_set = node ? node->data : NULL;
>     +
>
> How about using shash_find_data here?
> struct address_set *addr_set = shash_find_data(&local_address_sets, 
> addr_set_rec->name);

I agree.

>
>     +
>     +    /* Anything remaining in cur_addr_set_names refers to an
>     address set that
>     +     * has been deleted from the southbound database.  We should
>     delete
>     +     * the corresponding symtab entry. */
>     +    const char *cur_node, *next_node;
>     +    SSET_FOR_EACH_SAFE (cur_node, next_node, &cur_addr_set_names) {
>     + expr_macros_remove(&expr_address_sets, cur_node);
>     +
>     +        struct address_set *addr_set
>     +            = shash_find_and_delete(&local_address_sets, cur_node);
>     +        address_set_destroy(addr_set);
>     +
>     +        struct sset_node *sset_node = SSET_NODE_FROM_NAME(cur_node);
>     +        sset_delete(&cur_addr_set_names, sset_node);
>
>
> Trivial: this seems unnecessary, sset_destroy will call sset_clear, 
> and sset_clear will call sset_delete.

sset_destroy will have to loop the set from the beginning to delete the 
entries. Since we are anyway looping it before destroy(), it might save 
some minimal computing time. What do you think?

>     +   }
>     +
>     +    sset_destroy(&cur_addr_set_names);
>     +}
>
>     +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);
>     +        }
>     +
>     + sbrec_address_set_set_addresses(sb_address_set,
>     +                /* "char **" is not compatible with "const char
>     **" */
>     +                (const char **) nb_address_set->addresses,
>     +                nb_address_set->n_addresses);
>     +    }
>     +
>     +    struct shash_node *node, *next;
>     +    SHASH_FOR_EACH_SAFE (node, next, &sb_address_sets) {
>     +        sbrec_address_set_delete(node->data);
>
>
> Trivial: this seems unnecessary, shash_destroy will delete and free 
> node in sb_address_sets.
>

Same as the previous answer. Your suggestions are most welcome.

>     +        shash_delete(&sb_address_sets, node);
>     +    }
>     +    shash_destroy(&sb_address_sets);
>     +}
>
>
>     +  <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
>     cidrs.
>     +      The address set will ultimately be used in ACLs, where a
>     certain
>     +      type of field such as ip4.src or ip6.src will be compared
>     with the
>     +      address set. So, a single address set must contain
>     addresses of the
>     +      same type.
>     +    </p>
>     +
>
>
> Thanks for updating this.
Thank you for reporting :)

--
Babu