[ovs-dev] [PATCH] bridge: Tolerate missing Port and Interface records for local port.

Ethan Jackson ethan at nicira.com
Thu Apr 21 20:16:51 UTC 2011


This patch looks good to me.

Out of curiosity, why not just create the bridge port record manually
ourselves when it doesn't exist?

Ethan.

On Wed, Apr 13, 2011 at 11:10 AM, Ben Pfaff <blp at nicira.com> wrote:
> Until now, ovs-vswitchd has been unable to configure IP addresses and
> routes for bridges whose Bridge records lack a Port and an Interface
> record for the bridge's local port (e.g. OFPP_LOCAL, the port with the
> same name as the bridge itself).  When such a bridge was reconfigured,
> ovs-vswitchd would output a log message that worried people.
>
> This commit fixes the internal limitation that led to the message being
> printed.
>
> Bug #5385.
> ---
>  lib/ovsdb-idl.c   |    9 ++++++++
>  lib/ovsdb-idl.h   |    4 ++-
>  vswitchd/bridge.c |   59 +++++++++++++++++++++++++++++++++++++++-------------
>  3 files changed, 56 insertions(+), 16 deletions(-)
>
> diff --git a/lib/ovsdb-idl.c b/lib/ovsdb-idl.c
> index e1b53f4..fd15ea9 100644
> --- a/lib/ovsdb-idl.c
> +++ b/lib/ovsdb-idl.c
> @@ -1108,6 +1108,15 @@ ovsdb_idl_get(const struct ovsdb_idl_row *row,
>
>     return ovsdb_idl_read(row, column);
>  }
> +
> +/* Returns false if 'row' was obtained from the IDL, true if it was initialized
> + * to all-zero-bits by some other entity.  If 'row' was set up some other way
> + * then the return value is indeterminate. */
> +bool
> +ovsdb_idl_row_is_synthetic(const struct ovsdb_idl_row *row)
> +{
> +    return row->table == NULL;
> +}
>
>  /* Transactions. */
>
> diff --git a/lib/ovsdb-idl.h b/lib/ovsdb-idl.h
> index 302abd6..d11fb0e 100644
> --- a/lib/ovsdb-idl.h
> +++ b/lib/ovsdb-idl.h
> @@ -1,4 +1,4 @@
> -/* Copyright (c) 2009, 2010 Nicira Networks.
> +/* Copyright (c) 2009, 2010, 2011 Nicira Networks.
>  *
>  * Licensed under the Apache License, Version 2.0 (the "License");
>  * you may not use this file except in compliance with the License.
> @@ -105,6 +105,8 @@ const struct ovsdb_datum *ovsdb_idl_get(const struct ovsdb_idl_row *,
>                                         const struct ovsdb_idl_column *,
>                                         enum ovsdb_atomic_type key_type,
>                                         enum ovsdb_atomic_type value_type);
> +
> +bool ovsdb_idl_row_is_synthetic(const struct ovsdb_idl_row *);
>
>  /* Transactions. */
>
> diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c
> index 7997402..7ade176 100644
> --- a/vswitchd/bridge.c
> +++ b/vswitchd/bridge.c
> @@ -183,6 +183,11 @@ struct bridge {
>
>     /* Port mirroring. */
>     struct mirror *mirrors[MAX_MIRRORS];
> +
> +    /* Synthetic local port if necessary. */
> +    struct ovsrec_port synth_local_port;
> +    struct ovsrec_interface synth_local_iface;
> +    struct ovsrec_interface *synth_local_ifacep;
>  };
>
>  /* List of all bridges. */
> @@ -260,6 +265,7 @@ static void iface_update_qos(struct iface *, const struct ovsrec_qos *);
>  static void iface_update_cfm(struct iface *);
>  static bool iface_refresh_cfm_stats(struct iface *iface);
>  static bool iface_get_carrier(const struct iface *);
> +static bool iface_is_synthetic(const struct iface *);
>
>  static void shash_from_ovs_idl_map(char **keys, char **values, size_t n,
>                                    struct shash *);
> @@ -1136,6 +1142,10 @@ iface_refresh_status(struct iface *iface)
>     int64_t mtu_64;
>     int error;
>
> +    if (iface_is_synthetic(iface)) {
> +        return;
> +    }
> +
>     shash_init(&sh);
>
>     if (!netdev_get_status(iface->netdev, &sh)) {
> @@ -1257,6 +1267,10 @@ iface_refresh_stats(struct iface *iface)
>
>     struct netdev_stats stats;
>
> +    if (iface_is_synthetic(iface)) {
> +        return;
> +    }
> +
>     /* Intentionally ignore return value, since errors will set 'stats' to
>      * all-1s, and we will deal with that correctly below. */
>     netdev_get_stats(iface->netdev, &stats);
> @@ -1686,6 +1700,7 @@ bridge_destroy(struct bridge *br)
>         hmap_destroy(&br->ifaces);
>         hmap_destroy(&br->ports);
>         shash_destroy(&br->iface_by_name);
> +        free(br->synth_local_iface.type);
>         free(br->name);
>         free(br);
>     }
> @@ -1811,22 +1826,28 @@ bridge_reconfigure_one(struct bridge *br)
>                       br->name, name);
>         }
>     }
> +    if (!shash_find(&new_ports, br->name)) {
> +        struct dpif_port dpif_port;
> +        char *type;
>
> -    /* If we have a controller, then we need a local port.  Complain if the
> -     * user didn't specify one.
> -     *
> -     * XXX perhaps we should synthesize a port ourselves in this case. */
> -    if (bridge_get_controllers(br, NULL)) {
> -        char local_name[IF_NAMESIZE];
> -        int error;
> +        VLOG_WARN("bridge %s: no port named %s, synthesizing one",
> +                  br->name, br->name);
>
> -        error = dpif_port_get_name(br->dpif, ODPP_LOCAL,
> -                                   local_name, sizeof local_name);
> -        if (!error && !shash_find(&new_ports, local_name)) {
> -            VLOG_WARN("bridge %s: controller specified but no local port "
> -                      "(port named %s) defined",
> -                      br->name, local_name);
> -        }
> +        dpif_port_query_by_number(br->dpif, ODPP_LOCAL, &dpif_port);
> +        type = xstrdup(dpif_port.type ? dpif_port.type : "internal");
> +        dpif_port_destroy(&dpif_port);
> +
> +        br->synth_local_port.interfaces = &br->synth_local_ifacep;
> +        br->synth_local_port.n_interfaces = 1;
> +        br->synth_local_port.name = br->name;
> +
> +        br->synth_local_iface.name = br->name;
> +        free(br->synth_local_iface.type);
> +        br->synth_local_iface.type = type;
> +
> +        br->synth_local_ifacep = &br->synth_local_iface;
> +
> +        shash_add(&new_ports, br->name, &br->synth_local_port);
>     }
>
>     /* Get rid of deleted ports.
> @@ -3264,7 +3285,7 @@ iface_set_mac(struct iface *iface)
>  static void
>  iface_set_ofport(const struct ovsrec_interface *if_cfg, int64_t ofport)
>  {
> -    if (if_cfg) {
> +    if (if_cfg && !ovsdb_idl_row_is_synthetic(&if_cfg->header_)) {
>         ovsrec_interface_set_ofport(if_cfg, &ofport, 1);
>     }
>  }
> @@ -3424,6 +3445,14 @@ iface_get_carrier(const struct iface *iface)
>     /* XXX */
>     return netdev_get_carrier(iface->netdev);
>  }
> +
> +/* Returns true if 'iface' is synthetic, that is, if we constructed it locally
> + * instead of obtaining it from the database. */
> +static bool
> +iface_is_synthetic(const struct iface *iface)
> +{
> +    return ovsdb_idl_row_is_synthetic(&iface->cfg->header_);
> +}
>
>  /* Port mirroring. */
>
> --
> 1.7.1
>
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
>



More information about the dev mailing list