[ovs-dev] [PATCH] bridge: Consistently use miimon status if miimon is configured.

[Ethan Jackson]

Looks Good.  Minor changes may be required since the LACP series was
been merged, but the approach looks fine.  Go ahead and merge.

Ethan

On Thu, Mar 17, 2011 at 10:48 AM, Ben Pfaff <blp at nicira.com> wrote:
> A port can be configured to use miimon reporting as the criterion for
> enabling or disabling an interface, but in some cases (such as for reading
> the initial link status) the code was reading the carrier status instead.
> This commit fixes the problem.
>
> This changes the meaning of the link_status column in the Interface table.
> I don't think that the old meaning was useful to the controller in the
> case of a bond configured for miimon monitoring, because the controller
> could not use it to detect which interfaces the bond considered to be up
> or down.
> ---
>  vswitchd/bridge.c    |   28 +++++++++++++++++++---------
>  vswitchd/vswitch.xml |    6 ++++--
>  2 files changed, 23 insertions(+), 11 deletions(-)
>
> diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c
> index 9fb2579..6238252 100644
> --- a/vswitchd/bridge.c
> +++ b/vswitchd/bridge.c
> @@ -330,6 +330,7 @@ static uint8_t iface_get_lacp_state(const struct iface *);
>  static void iface_get_lacp_priority(struct iface *, struct lacp_info *);
>  static void iface_set_lacp_defaulted(struct iface *);
>  static void iface_set_lacp_expired(struct iface *);
> +static bool iface_get_carrier(const struct iface *);
>
>  static void shash_from_ovs_idl_map(char **keys, char **values, size_t n,
>                                    struct shash *);
> @@ -747,7 +748,7 @@ bridge_reconfigure(const struct ovsrec_open_vswitch *ovs_cfg)
>                 /* Update 'iface'. */
>                 if (iface) {
>                     iface->netdev = netdev;
> -                    iface->enabled = netdev_get_carrier(iface->netdev);
> +                    iface->enabled = iface_get_carrier(iface);
>                     iface->up = iface->enabled;
>                 }
>             } else if (iface && iface->netdev) {
> @@ -1207,8 +1208,7 @@ iface_refresh_status(struct iface *iface)
>
>
>     ovsrec_interface_set_link_state(iface->cfg,
> -                                    netdev_get_carrier(iface->netdev)
> -                                    ? "up" : "down");
> +                                    iface_get_carrier(iface) ? "up" : "down");
>
>     error = netdev_get_mtu(iface->netdev, &mtu);
>     if (!error && mtu != INT_MAX) {
> @@ -2444,8 +2444,9 @@ bond_update_fake_iface_stats(struct port *port)
>  }
>
>  static void
> -bond_link_carrier_update(struct iface *iface, bool carrier)
> +bond_link_carrier_update(struct iface *iface)
>  {
> +    bool carrier = iface_get_carrier(iface);
>     if (carrier == iface->up) {
>         return;
>     }
> @@ -2477,8 +2478,7 @@ bond_run(struct port *port)
>
>             iface = port_lookup_iface(port, devname);
>             if (iface) {
> -                bool up = netdev_get_carrier(iface->netdev);
> -                bond_link_carrier_update(iface, up);
> +                bond_link_carrier_update(iface);
>             }
>             free(devname);
>         }
> @@ -2487,9 +2487,7 @@ bond_run(struct port *port)
>
>         if (time_msec() >= port->bond_miimon_next_update) {
>             for (i = 0; i < port->n_ifaces; i++) {
> -                struct iface *iface = port->ifaces[i];
> -                bool up = netdev_get_miimon(iface->netdev);
> -                bond_link_carrier_update(iface, up);
> +                bond_link_carrier_update(port->ifaces[i]);
>             }
>             port->bond_miimon_next_update = time_msec() +
>                 port->bond_miimon_interval;
> @@ -5079,6 +5077,18 @@ iface_update_cfm(struct iface *iface)
>         iface->cfm = NULL;
>     }
>  }
> +
> +/* Read carrier or miimon status directly from 'iface''s netdev, according to
> + * how 'iface''s port is configured.
> + *
> + * Returns true if 'iface' is up, false otherwise. */
> +static bool
> +iface_get_carrier(const struct iface *iface)
> +{
> +    return (iface->port->miimon
> +            ? netdev_get_miimon(iface->netdev)
> +            : netdev_get_carrier(iface->netdev));
> +}
>
>  /* Port mirroring. */
>
> diff --git a/vswitchd/vswitch.xml b/vswitchd/vswitch.xml
> index ad1bffa..cbe4371 100644
> --- a/vswitchd/vswitch.xml
> +++ b/vswitchd/vswitch.xml
> @@ -1029,8 +1029,10 @@
>
>       <column name="link_state">
>         <p>
> -          The observed state of the physical network link;
> -          i.e. whether a carrier is detected by the interface.
> +          The observed state of the physical network link.  This is ordinarily
> +          the link's carrier status.  If the interface's <ref table="Port"/> is
> +          a bond configured for miimon monitoring, it is instead the network
> +          link's miimon status.
>         </p>
>       </column>
>
> --
> 1.7.1
>
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
>