[ovs-dev] [PATCH 3/3] bridge: Update bridge to discover datapath and port types

Ben Pfaff blp at nicira.com
Fri Mar 6 21:22:28 UTC 2015


On Fri, Mar 06, 2015 at 03:37:38PM +0000, Mark D. Gray wrote:
> From: "Mark D. Gray" <mark.d.gray at intel.com>
> 
> This patch enumerates datapath and port types and adds this
> information to the datapath_types and port_types columns in the
> ovsdb.
> 
> This allows an ovsdb client to query the datapath in order to
> determine if certain datapath and port types exist. For example,
> by querying the port_types column, an ovsdb client will be able
> to determine if this instance of ovs-vswitchd was compiled with
> DPDK support.
> 
> Signed-off-by: Mark D. Gray <mark.d.gray at intel.com>

I'd fold this in with the previous patch that adds the columns.

We don't generally use double new-lines, so I'd drop this hunk:
> @@ -317,6 +319,7 @@ static ofp_port_t iface_get_requested_ofp_port(
>      const struct ovsrec_interface *);
>  static ofp_port_t iface_pick_ofport(const struct ovsrec_interface *);
>  
> +
>  /* Linux VLAN device support (e.g. "eth0.10" for VLAN 10.)
>   *
>   * This is deprecated.  It is only for compatibility with broken device drivers

CodingStyle requires {} here.  However, it's wasteful to do this on
every trip through the OVS main loop; it should not be necessary:
> @@ -2889,6 +2896,9 @@ bridge_run(void)
>      }
>      cfg = ovsrec_open_vswitch_first(idl);
>  
> +    if (cfg)
> +        discover_types(cfg);
> +
>      /* Initialize the ofproto library.  This only needs to run once, but
>       * it must be done after the configuration is set.  If the
>       * initialization has already occurred, bridge_init_ofproto()


> +/*
> + * Add registered netdev and dpif types to ovsdb to allow external
> + * applications to query the capabilities of the Open vSwitch instance
> + * running on the node.
> + */
> +static void
> +discover_types(const struct ovsrec_open_vswitch *cfg)
> +{
> +    struct sset types;
> +    const char *type;
> +    struct ovsdb_idl_txn *txn;
> +    size_t n_dp_types, n_port_types;
> +    unsigned int i;
> +    const char **datapath_types;
> +    const char **port_types;
> +
> +    txn = ovsdb_idl_txn_create(idl);
> +
> +    /* datapath types */
> +    sset_init(&types);
> +    dp_enumerate_types(&types);
> +    n_dp_types = sset_count(&types);

Please use "sizeof *datapath_types" instead of "sizeof(char *)" as
recommended by CodingStyle.  Ditto one other place below:
> +    datapath_types = xmalloc(sizeof(char *) * n_dp_types);
> +
> +    i = 0;
> +    SSET_FOR_EACH(type, &types) {

I don't think there's value in doing the xstrdup() here.  It just creates
more data to free later.  Ditto one other place later:
> +        datapath_types[i++] = xstrdup(type);
> +    }
> +
> +    ovsrec_open_vswitch_set_datapath_types(cfg, datapath_types, n_dp_types);
> +
> +    sset_destroy(&types);
> +
> +    /* port types */
> +    sset_init(&types);
> +    netdev_enumerate_types(&types);
> +    n_port_types = sset_count(&types);
> +
> +    port_types = xmalloc(sizeof(char *) * n_port_types);
> +
> +    i = 0;
> +    SSET_FOR_EACH(type, &types) {
> +        port_types[i++] = xstrdup(type);
> +    }
> +
> +    ovsrec_open_vswitch_set_port_types(cfg, port_types, n_port_types);
> +    sset_destroy(&types);
> +
> +    /* clean up any allocated memory */
> +    for (i = 0; i < n_dp_types; i++) {
> +         free((char *)datapath_types[i]);
> +    }
> +
> +    free(datapath_types);
> +    for (i = 0; i < n_port_types; i++) {
> +         free((char *)port_types[i]);
> +    }
> +    free(port_types);
> +
> +    ovsdb_idl_txn_commit(txn);
> +    ovsdb_idl_txn_destroy(txn);
> +}

Thanks,

Ben.



More information about the dev mailing list