[ovs-dev] [InBandOVSDB 4/4] vswitchd: Enable in-band control to managers.

Justin Pettit jpettit at nicira.com
Mon Apr 26 08:24:08 UTC 2010


On Apr 20, 2010, at 4:37 PM, Ben Pfaff wrote:

> static void bridge_reconfigure_controller(const struct ovsrec_open_vswitch *,
> -                                          struct bridge *);
> +                                          struct bridge *,
> +                                          const struct sockaddr_in *managers,
> +                                          size_t n_managers);

To be consistently vague, do you think we should rename this function "bridge_reconfigure_controller"?

> +/* Looks at the list of managers in 'ovs_cfg' and extracts their remote IP
> + * addresses and ports into '*managersp' and '*n_managersp'.  The caller is
> + * responsible for freeing '*managersp' (with free()).
> + *
> + * You may be asking yourself "WTF does ovs-vswitchd care?", because

:-)  I've been trying to think how you can work more masked profanity into our relatively dry mailing lists.  I know you're fond of saying "patches accepted" when people get demandy of new features, but I just came up with a new one you can try: "Bits or GTFO".

> + * ovsdb-server is responsible for connecting to the managers, and ovs-vswitchd
> + * should not be and in fact is not directly involved in that.  But
> + * ovs-vswitchd needs to make sure that ovsdb-server can reach the managers, so
> + * it has to tell in-band control where the managers are to enable that.
> + */
> +static void
> +collect_managers(const struct ovsrec_open_vswitch *ovs_cfg,
> +                 struct sockaddr_in **managersp, size_t *n_managersp)
> +{
> +    struct sockaddr_in *managers = NULL;
> +    size_t n_managers = 0;
> +
> +    if (ovs_cfg->n_managers > 0) {
> +        size_t i;
> +
> +        managers = xmalloc(ovs_cfg->n_managers * sizeof *managersp);

Should that be "sizeof *managers"?

> +        for (i = 0; i < ovs_cfg->n_managers; i++) {
> +            const char *name = ovs_cfg->managers[i];
> +            struct sockaddr_in *sin = &managers[i];
> +
> +            if ((!strncmp(name, "tcp:", 4)
> +                 && inet_parse_active(name + 4, JSONRPC_TCP_PORT, sin)) ||
> +                (!strncmp(name, "ssl:", 4)
> +                 && inet_parse_active(name + 4, JSONRPC_SSL_PORT, sin))) {
> +                n_managers++;
> +            }
> +        }
> +    }
> +
> +    *managersp = managers;
> +    *n_managersp = n_managers;
> +}
> +
> void
> bridge_reconfigure(const struct ovsrec_open_vswitch *ovs_cfg)
> {
> @@ -520,6 +561,8 @@ bridge_reconfigure(const struct ovsrec_open_vswitch *ovs_cfg)
>     struct shash old_br, new_br;
>     struct shash_node *node;
>     struct bridge *br, *next;
> +    struct sockaddr_in *managers;
> +    size_t n_managers;
>     size_t i;
>     int sflow_bridge_number;
> 
> @@ -527,6 +570,8 @@ bridge_reconfigure(const struct ovsrec_open_vswitch *ovs_cfg)
> 
>     txn = ovsdb_idl_txn_create(ovs_cfg->header_.table->idl);
> 
> +    collect_managers(ovs_cfg, &managers, &n_managers);
> +
>     /* Collect old and new bridges. */
>     shash_init(&old_br);
>     shash_init(&new_br);
> @@ -792,7 +837,7 @@ bridge_reconfigure(const struct ovsrec_open_vswitch *ovs_cfg)
>          * yet; when a controller is configured, resetting the datapath ID will
>          * immediately disconnect from the controller, so it's better to set
>          * the datapath ID before the controller. */
> -        bridge_reconfigure_controller(ovs_cfg, br);
> +        bridge_reconfigure_controller(ovs_cfg, br, managers, n_managers);
>     }

I don't see any place where "managers" gets freed.

--Justin






More information about the dev mailing list