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

Zong Kai LI zealokii at gmail.com
Mon Jun 27 15:20:43 UTC 2016


>
> +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);

+
> +    /* 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(&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.

+        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.



More information about the dev mailing list