Looks Good.
On Fri, Mar 25, 2011 at 10:35 AM, Ben Pfaff <b...@nicira.com> wrote:
> There's no reason that I can see to maintain this information in struct
> port and struct iface. It's redundant, since the lacp implementation
> maintains the same information.
> ---
> lib/lacp.c | 8 ++++
> lib/lacp.h | 3 +
> vswitchd/bridge.c | 122
> +++++++++++++++++++++++++----------------------------
> 3 files changed, 69 insertions(+), 64 deletions(-)
>
> diff --git a/lib/lacp.c b/lib/lacp.c
> index c01a567..094b036 100644
> --- a/lib/lacp.c
> +++ b/lib/lacp.c
> @@ -141,6 +141,14 @@ lacp_configure(struct lacp *lacp, const char *name,
> lacp->fast = fast;
> }
>
> +/* Returns true if 'lacp' is configured in active mode, false if 'lacp' is
> + * configured for passive mode. */
> +bool
> +lacp_is_active(const struct lacp *lacp)
> +{
> + return lacp->active;
> +}
> +
> /* Processes 'pdu', a parsed LACP packet received on 'slave_'. This function
> * should be called on all packets received on 'slave_' with Ethernet Type
> * ETH_TYPE_LACP and parsable by parse_lacp_packet(). */
> diff --git a/lib/lacp.h b/lib/lacp.h
> index 8d3bc78..c5f3c2f 100644
> --- a/lib/lacp.h
> +++ b/lib/lacp.h
> @@ -27,9 +27,12 @@ typedef void lacp_send_pdu(void *slave, const struct
> lacp_pdu *);
> void lacp_init(void);
> struct lacp *lacp_create(void);
> void lacp_destroy(struct lacp *);
> +
> void lacp_configure(struct lacp *, const char *name,
> const uint8_t sys_id[ETH_ADDR_LEN],
> uint16_t sys_priority, bool active, bool fast);
> +bool lacp_is_active(const struct lacp *);
> +
> void lacp_process_pdu(struct lacp *, const void *slave,
> const struct lacp_pdu *);
> bool lacp_negotiated(const struct lacp *);
> diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c
> index c9dc954..fea574d 100644
> --- a/vswitchd/bridge.c
> +++ b/vswitchd/bridge.c
> @@ -113,9 +113,6 @@ struct iface {
> bool up; /* Is the interface up? */
> const char *type; /* Usually same as cfg->type. */
> const struct ovsrec_interface *cfg;
> -
> - /* LACP information. */
> - uint16_t lacp_priority; /* LACP port priority. */
> };
>
> #define BOND_MASK 0xff
> @@ -183,9 +180,6 @@ struct port {
>
> /* LACP information. */
> struct lacp *lacp; /* LACP object. NULL if LACP is disabled. */
> - bool lacp_active; /* True if LACP is active */
> - bool lacp_fast; /* True if LACP is in fast mode. */
> - uint16_t lacp_priority; /* LACP system priority. */
>
> /* SLB specific bonding info. */
> struct bond_entry *bond_hash; /* An array of (BOND_MASK + 1) elements. */
> @@ -3510,7 +3504,7 @@ bond_unixctl_show(struct unixctl_conn *conn,
>
> if (port->lacp) {
> ds_put_format(&ds, "lacp: %s\n",
> - port->lacp_active ? "active" : "passive");
> + lacp_is_active(port->lacp) ? "active" : "passive");
> } else {
> ds_put_cstr(&ds, "lacp: off\n");
> }
> @@ -3970,7 +3964,7 @@ port_reconfigure(struct port *port, const struct
> ovsrec_port *cfg)
> {
> const char *detect_mode;
> struct shash new_ifaces;
> - long long int next_rebalance, miimon_next_update, lacp_priority;
> + long long int next_rebalance, miimon_next_update;
> bool need_flush = false;
> unsigned long *trunks;
> int vlan;
> @@ -4068,57 +4062,9 @@ port_reconfigure(struct port *port, const struct
> ovsrec_port *cfg)
> iface->type = (!strcmp(if_cfg->name, port->bridge->name) ? "internal"
> : if_cfg->type[0] ? if_cfg->type
> : "system");
> -
> - lacp_priority =
> - atoi(get_interface_other_config(if_cfg, "lacp-port-priority",
> - "0"));
> -
> - if (lacp_priority <= 0 || lacp_priority > UINT16_MAX) {
> - iface->lacp_priority = UINT16_MAX;
> - } else {
> - iface->lacp_priority = lacp_priority;
> - }
> }
> shash_destroy(&new_ifaces);
>
> - port->lacp_fast = !strcmp(get_port_other_config(cfg, "lacp-time",
> "slow"),
> - "fast");
> -
> - lacp_priority =
> - atoi(get_port_other_config(cfg, "lacp-system-priority", "0"));
> -
> - if (lacp_priority <= 0 || lacp_priority > UINT16_MAX) {
> - /* Prefer bondable links if unspecified. */
> - port->lacp_priority = port->n_ifaces > 1 ? UINT16_MAX - 1 :
> UINT16_MAX;
> - } else {
> - port->lacp_priority = lacp_priority;
> - }
> -
> - if (!port->cfg->lacp) {
> - /* XXX when LACP implementation has been sufficiently tested, enable
> by
> - * default and make active on bonded ports. */
> - lacp_destroy(port->lacp);
> - port->lacp = NULL;
> - } else if (!strcmp(port->cfg->lacp, "off")) {
> - lacp_destroy(port->lacp);
> - port->lacp = NULL;
> - } else if (!strcmp(port->cfg->lacp, "active")) {
> - if (!port->lacp) {
> - port->lacp = lacp_create();
> - }
> - port->lacp_active = true;
> - } else if (!strcmp(port->cfg->lacp, "passive")) {
> - if (!port->lacp) {
> - port->lacp = lacp_create();
> - }
> - port->lacp_active = false;
> - } else {
> - VLOG_WARN("port %s: unknown LACP mode %s",
> - port->name, port->cfg->lacp);
> - lacp_destroy(port->lacp);
> - port->lacp = NULL;
> - }
> -
> /* Get VLAN tag. */
> vlan = -1;
> if (cfg->tag) {
> @@ -4245,20 +4191,68 @@ port_lookup_iface(const struct port *port, const char
> *name)
> return iface && iface->port == port ? iface : NULL;
> }
>
> +static bool
> +enable_lacp(struct port *port, bool *activep)
> +{
> + if (!port->cfg->lacp) {
> + /* XXX when LACP implementation has been sufficiently tested, enable
> by
> + * default and make active on bonded ports. */
> + return false;
> + } else if (!strcmp(port->cfg->lacp, "off")) {
> + return false;
> + } else if (!strcmp(port->cfg->lacp, "active")) {
> + *activep = true;
> + return true;
> + } else if (!strcmp(port->cfg->lacp, "passive")) {
> + *activep = false;
> + return true;
> + } else {
> + VLOG_WARN("port %s: unknown LACP mode %s",
> + port->name, port->cfg->lacp);
> + return false;
> + }
> +}
> +
> static void
> port_update_lacp(struct port *port)
> {
> - if (port->lacp) {
> - struct iface *iface;
> + struct iface *iface;
> + int priority;
> + bool active;
> + bool fast;
>
> - lacp_configure(port->lacp, port->name,
> - port->bridge->ea, port->lacp_priority,
> - port->lacp_active, port->lacp_fast);
> + if (!enable_lacp(port, &active)) {
> + lacp_destroy(port->lacp);
> + port->lacp = NULL;
> + return;
> + }
>
> - LIST_FOR_EACH (iface, port_elem, &port->ifaces) {
> - lacp_slave_register(port->lacp, iface, iface->name,
> - iface->dp_ifidx, iface->lacp_priority);
> + if (!port->lacp) {
> + port->lacp = lacp_create();
> + }
> +
> + fast = !strcmp(get_port_other_config(port->cfg, "lacp-time", "slow"),
> + "fast");
> +
> + priority = atoi(get_port_other_config(port->cfg, "lacp-system-priority",
> + "0"));
> + if (priority <= 0 || priority > UINT16_MAX) {
> + /* Prefer bondable links if unspecified. */
> + priority = UINT16_MAX - (port->n_ifaces > 1);
> + }
> +
> + lacp_configure(port->lacp, port->name, port->bridge->ea, priority,
> + active, fast);
> +
> + LIST_FOR_EACH (iface, port_elem, &port->ifaces) {
> + priority = atoi(get_interface_other_config(
> + iface->cfg, "lacp-port-priority", "0"));
> + if (priority <= 0 || priority > UINT16_MAX) {
> + priority = UINT16_MAX;
> }
> +
> + lacp_slave_register(port->lacp, iface, iface->name,
> + iface->dp_ifidx, priority);
> }
> }
>
> --
> 1.7.1
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
>
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
