[ovs-dev] [PATCH] bridge: Clear out all Interface fields when an interface cannot be created.

[Ethan Jackson]

Looks good.

Ethan

On Wed, Sep 28, 2011 at 09:37, Ben Pfaff <blp at nicira.com> wrote:
> When an Interface record is invalid (for example, when the interface that
> it specifies does not exist and cannot be created), ovs-vswitchd would
> leave any pre-existing data in its columns, except that it would set the
> ofport column to -1 to indicate the error.  This was sometimes confusing
> because, for example, the lacp_current field could still be set to "true"
> if LACP has previously been active and up-to-date.
>
> This commit changes ovs-vswitchd to reset all such data to its default
> values when an interface is invalid.
>
> Bug #7450.
> Reported-by: Duffie Cooley <dcooley at nicira.com>
> Bug #7491.
> Reported-by: Ethan Jackson <ethan at nicira.com>
> Release Notes #7500.
> Reported-by: Keith Amidon <keith at nicira.com>
> ---
> So far this is compile-tested only, but I'll test it before pushing.
>
> diff --git a/AUTHORS b/AUTHORS
> index da82f10..20a53d3 100644
> --- a/AUTHORS
> +++ b/AUTHORS
> @@ -61,6 +61,7 @@ Bryan Osoro             bosoro at nicira.com
>  Cedric Hobbs            cedric at nicira.com
>  Dave Walker             DaveWalker at ubuntu.com
>  Derek Cormier           derek.cormier at lab.ntt.co.jp
> +Duffie Cooley           dcooley at nicira.com
>  DK Moon                 dkmoon at nicira.com
>  Gaetano Catalli         gaetano.catalli at gmail.com
>  George Shuklin          amarao at desunote.ru
> diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c
> index a576844..46d0618 100644
> --- a/vswitchd/bridge.c
> +++ b/vswitchd/bridge.c
> @@ -195,6 +195,7 @@ static struct iface *iface_from_ofp_port(const struct bridge *,
>                                          uint16_t ofp_port);
>  static void iface_set_mac(struct iface *);
>  static void iface_set_ofport(const struct ovsrec_interface *, int64_t ofport);
> +static void iface_clear_db_record(const struct ovsrec_interface *if_cfg);
>  static void iface_configure_qos(struct iface *, const struct ovsrec_qos *);
>  static void iface_configure_cfm(struct iface *);
>  static void iface_refresh_cfm_stats(struct iface *);
> @@ -939,7 +940,7 @@ bridge_add_ofproto_ports(struct bridge *br)
>                     /* We already reported a related error, don't bother
>                      * duplicating it. */
>                 }
> -                iface_set_ofport(iface->cfg, -1);
> +                iface_clear_db_record(iface->cfg);
>                 iface_destroy(iface);
>             }
>         }
> @@ -2162,7 +2163,7 @@ port_add_ifaces(struct port *port)
>             && !shash_add_once(&new_ifaces, cfg->name, cfg)) {
>             VLOG_WARN("port %s: %s specified twice as port interface",
>                       port->name, cfg->name);
> -            iface_set_ofport(cfg, -1);
> +            iface_clear_db_record(cfg);
>         }
>     }
>
> @@ -2515,6 +2516,29 @@ iface_set_ofport(const struct ovsrec_interface *if_cfg, int64_t ofport)
>     }
>  }
>
> +/* Clears all of the fields in 'if_cfg' that indicate interface status, and
> + * sets the "ofport" field to -1.
> + *
> + * This is appropriate when 'if_cfg''s interface cannot be created or is
> + * otherwise invalid. */
> +static void
> +iface_clear_db_record(const struct ovsrec_interface *if_cfg)
> +{
> +    if (!ovsdb_idl_row_is_synthetic(&if_cfg->header_)) {
> +        iface_set_ofport(if_cfg, -1);
> +        ovsrec_interface_set_status(if_cfg, NULL, NULL, 0);
> +        ovsrec_interface_set_admin_state(if_cfg, NULL);
> +        ovsrec_interface_set_duplex(if_cfg, NULL);
> +        ovsrec_interface_set_link_speed(if_cfg, NULL, 0);
> +        ovsrec_interface_set_link_state(if_cfg, NULL);
> +        ovsrec_interface_set_mtu(if_cfg, NULL, 0);
> +        ovsrec_interface_set_cfm_fault(if_cfg, NULL, 0);
> +        ovsrec_interface_set_cfm_remote_mpids(if_cfg, NULL, 0);
> +        ovsrec_interface_set_lacp_current(if_cfg, NULL, 0);
> +        ovsrec_interface_set_statistics(if_cfg, NULL, NULL, 0);
> +    }
> +}
> +
>  /* Adds the 'n' key-value pairs in 'keys' in 'values' to 'shash'.
>  *
>  * The value strings in '*shash' are taken directly from values[], not copied,
> --
> 1.7.2.5
>
>