[ovs-dev] [port-renumbering 7/8] bridge: Reconfigure in single pass.

[Jarno Rajahalme]

On Dec 10, 2013, at 11:20 PM, Ben Pfaff <blp at nicira.com> wrote:

> This greatly simplifies the reconfiguration code, making it much easier
> to understand and modify.  The old multi-pass configuration had the
> property that it didn't delay block packet processing as much, but that's
> not much of a worry anymore now that latency critical activities have
> been moved outside the main thread.
> 

You’ll find two minor comments below, but otherwise seems good to me. I have no history with this code, though, so with that caveat:

Acked-by: Jarno Rajahalme <jrajahalme at nicira.com>

…
> +static void
> +bridge_delete_ports(struct bridge *br)

IMO this function should be renamed, as it only deletes ports that should not exists, are of wrong type, or cannot be successfully configured. How about “bridge_conf_or_del_ports” etc.?

> +{
> +    struct ofproto_port ofproto_port;
> +    struct ofproto_port_dump dump;
> 
> -        /* Remove ports from any datapath with the same name as 'br'.  If we
> -         * don't do this, creating 'br''s ofproto will fail because a port with
> -         * the same name as its local port already exists. */
> -        HMAP_FOR_EACH (br2, node, &all_bridges) {
> -            struct ofproto_port ofproto_port;
> +    ofp_port_t *del;
> +    size_t n, allocated;
> +    size_t i;
> +
…

> @@ -2824,44 +2625,23 @@ bridge_get_controllers(const struct bridge *br,
> }
> 
> static void
> -bridge_queue_if_cfg(struct bridge *br,
> -                    const struct ovsrec_interface *cfg,
> -                    const struct ovsrec_port *parent)
> +bridge_collect_wanted_ports(struct bridge *br,
> +                            const unsigned long int *splinter_vlans,
> +                            struct shash *wanted_ports)
> {
> -    struct if_cfg *if_cfg = xmalloc(sizeof *if_cfg);
> -
> -    if_cfg->cfg = cfg;
> -    if_cfg->parent = parent;
> -    if_cfg->ofport = iface_pick_ofport(cfg);
> -    hmap_insert(&br->if_cfg_todo, &if_cfg->hmap_node,
> -                hash_string(if_cfg->cfg->name, 0));
> -}
> -
> -/* Deletes "struct port"s and "struct iface"s under 'br' which aren't
> - * consistent with 'br->cfg'.  Updates 'br->if_cfg_queue' with interfaces which
> - * 'br' needs to complete its configuration. */
> -static void
> -bridge_add_del_ports(struct bridge *br,
> -                     const unsigned long int *splinter_vlans)
> -{
> -    struct shash_node *port_node;
> -    struct port *port, *next;
> -    struct shash new_ports;
>     size_t i;
> +    

Extra trailing whitespace on this line.

> +    shash_init(wanted_ports);
> 
> -    ovs_assert(hmap_is_empty(&br->if_cfg_todo));
> -
...