From 6d57dea9649a562e0f7a97fcab936ddf36a15009 Mon Sep 17 00:00:00 2001 From: Ben Pfaff Date: Wed, 22 Aug 2018 15:12:12 -0700 Subject: [PATCH] ofproto: Consistently force off OFPPS_LIVE if port or link is down. It doesn't make sense for a port that is down to be "live" from OpenFlow's point of view, but this could happen in OVS. Signed-off-by: Ben Pfaff Acked-by: Numan Siddique --- ofproto/ofproto-dpif.c | 26 ++--------------------- ofproto/ofproto-provider.h | 1 + ofproto/ofproto.c | 43 +++++++++++++++++++++++++++++--------- 3 files changed, 36 insertions(+), 34 deletions(-) diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c index 3fec959b47..b7acfa246c 100644 --- a/ofproto/ofproto-dpif.c +++ b/ofproto/ofproto-dpif.c @@ -2003,16 +2003,6 @@ port_modified(struct ofport *port_) bfd_set_netdev(port->bfd, netdev); } - /* Set liveness, unless the link is administratively or - * operationally down or link monitoring false */ - if (!(port->up.pp.config & OFPUTIL_PC_PORT_DOWN) && - !(port->up.pp.state & OFPUTIL_PS_LINK_DOWN) && - port->up.may_enable) { - port->up.pp.state |= OFPUTIL_PS_LIVE; - } else { - port->up.pp.state &= ~OFPUTIL_PS_LIVE; - } - ofproto_dpif_monitor_port_update(port, port->bfd, port->cfm, port->lldp, &port->up.pp.hw_addr); @@ -2047,6 +2037,7 @@ port_reconfigured(struct ofport *port_, enum ofputil_port_config old_config) bundle_update(port->bundle); } } + port_run(port); } static int @@ -3609,7 +3600,7 @@ port_run(struct ofport_dpif *ofport) bool enable = may_enable_port(ofport); if (ofport->up.may_enable != enable) { - ofport->up.may_enable = enable; + ofproto_port_set_enable(&ofport->up, enable); struct ofproto_dpif *ofproto = ofproto_dpif_cast(ofport->up.ofproto); ofproto->backer->need_revalidate = REV_PORT_TOGGLED; @@ -3617,19 +3608,6 @@ port_run(struct ofport_dpif *ofport) if (ofport->rstp_port) { rstp_port_set_mac_operational(ofport->rstp_port, enable); } - - /* Propagate liveness, unless the link is administratively or - * operationally down. */ - if (!(ofport->up.pp.config & OFPUTIL_PC_PORT_DOWN) && - !(ofport->up.pp.state & OFPUTIL_PS_LINK_DOWN)) { - enum ofputil_port_state of_state = ofport->up.pp.state; - if (enable) { - of_state |= OFPUTIL_PS_LIVE; - } else { - of_state &= ~OFPUTIL_PS_LIVE; - } - ofproto_port_set_state(&ofport->up, of_state); - } } } diff --git a/ofproto/ofproto-provider.h b/ofproto/ofproto-provider.h index 28627cbd39..4fd8cb14ed 100644 --- a/ofproto/ofproto-provider.h +++ b/ofproto/ofproto-provider.h @@ -165,6 +165,7 @@ struct ofport { bool may_enable; /* May be live (OFPPS_LIVE) if link is up. */ }; +void ofproto_port_set_enable(struct ofport *, bool enable); void ofproto_port_set_state(struct ofport *, enum ofputil_port_state); /* OpenFlow table flags: diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c index 32e9c48527..8b2e3ca97a 100644 --- a/ofproto/ofproto.c +++ b/ofproto/ofproto.c @@ -2457,11 +2457,34 @@ ofport_remove_with_name(struct ofproto *ofproto, const char *name) } } +static enum ofputil_port_state +normalize_state(enum ofputil_port_config config, + enum ofputil_port_state state, + bool may_enable) +{ + return (config & OFPUTIL_PC_PORT_DOWN + || state & OFPUTIL_PS_LINK_DOWN + || !may_enable + ? state & ~OFPUTIL_PS_LIVE + : state | OFPUTIL_PS_LIVE); +} + +void +ofproto_port_set_enable(struct ofport *port, bool enable) +{ + if (enable != port->may_enable) { + port->may_enable = enable; + ofproto_port_set_state(port, normalize_state(port->pp.config, + port->pp.state, + port->may_enable)); + } +} /* Update OpenFlow 'state' in 'port' and notify controller. */ void ofproto_port_set_state(struct ofport *port, enum ofputil_port_state state) { + state = normalize_state(port->pp.config, state, port->may_enable); if (port->pp.state != state) { struct ofputil_phy_port old_pp = port->pp; port->pp.state = state; @@ -2611,16 +2634,18 @@ update_port(struct ofproto *ofproto, const char *name) port = ofproto_get_port(ofproto, ofproto_port.ofp_port); if (port && !strcmp(netdev_get_name(port->netdev), name)) { struct netdev *old_netdev = port->netdev; - struct ofputil_phy_port old_pp = port->pp; /* ofport_open() only sets OFPUTIL_PC_PORT_DOWN and - * OFPUTIL_PS_LINK_DOWN. Keep the other config and state bits. */ + * OFPUTIL_PS_LINK_DOWN. Keep the other config and state bits (but + * a port that is down cannot be live). */ pp.config |= port->pp.config & ~OFPUTIL_PC_PORT_DOWN; pp.state |= port->pp.state & ~OFPUTIL_PS_LINK_DOWN; + pp.state = normalize_state(pp.config, pp.state, port->may_enable); /* 'name' hasn't changed location. Any properties changed? */ - bool port_changed = !ofport_equal(&port->pp, &pp); - if (port_changed) { + if (!ofport_equal(&port->pp, &pp)) { + connmgr_send_port_status(port->ofproto->connmgr, NULL, + &port->pp, &pp, OFPPR_MODIFY); port->pp = pp; } @@ -2636,12 +2661,6 @@ update_port(struct ofproto *ofproto, const char *name) port->ofproto->ofproto_class->port_modified(port); } - /* Send status update, if any port property changed */ - if (port_changed) { - connmgr_send_port_status(port->ofproto->connmgr, NULL, - &old_pp, &port->pp, OFPPR_MODIFY); - } - netdev_close(old_netdev); } else { /* If 'port' is nonnull then its name differs from 'name' and thus @@ -3636,7 +3655,11 @@ update_port_config(struct ofconn *ofconn, struct ofport *port, if (toggle) { struct ofputil_phy_port old_pp = port->pp; + port->pp.config ^= toggle; + port->pp.state = normalize_state(port->pp.config, port->pp.state, + port->may_enable); + port->ofproto->ofproto_class->port_reconfigured(port, old_pp.config); connmgr_send_port_status(port->ofproto->connmgr, ofconn, &old_pp, &port->pp, OFPPR_MODIFY);