[ovs-dev] [port reconf 7/7] vswitchd: Clean up iface_create().
Ben Pfaff
blp at nicira.com
Wed Apr 25 01:10:26 UTC 2012
Thanks, I pushed this to master. When I get a clean build from the
autobuilder I'll cherry-pick to branch-1.6.
On Tue, Apr 24, 2012 at 05:52:00PM -0700, Ethan Jackson wrote:
> Much cleaner, looks good, thanks.
>
> Assuming patch 6/7 is the one I sent out to the list, I consider this
> series reviewed.
>
> Ethan
>
> On Tue, Apr 24, 2012 at 17:08, Ben Pfaff <blp at nicira.com> wrote:
> > iface_create() did its work in an order that meant it had to do a lot more
> > cleanup on error paths than is otherwise needed. This commit reorders the
> > work to avoid this extra cleanup.
> >
> > bridge_ofproto_port_del() is no longer used after the refactoring so this
> > commit deletes it.
> >
> > Signed-off-by: Ben Pfaff <blp at nicira.com>
> > ---
> > vswitchd/bridge.c | 192 ++++++++++++++++++++++++-----------------------------
> > 1 files changed, 88 insertions(+), 104 deletions(-)
> >
> > diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c
> > index 94cb494..1a1abc9 100644
> > --- a/vswitchd/bridge.c
> > +++ b/vswitchd/bridge.c
> > @@ -1167,19 +1167,6 @@ iface_set_netdev_config(const struct ovsrec_interface *iface_cfg,
> > return error;
> > }
> >
> > -static void
> > -bridge_ofproto_port_del(struct bridge *br, struct ofproto_port ofproto_port)
> > -{
> > - int error = ofproto_port_del(br->ofproto, ofproto_port.ofp_port);
> > - if (error) {
> > - VLOG_WARN("bridge %s: failed to remove %s interface (%s)",
> > - br->name, ofproto_port.name, strerror(error));
> > - } else {
> > - VLOG_INFO("bridge %s: removed interface %s (%d)", br->name,
> > - ofproto_port.name, ofproto_port.ofp_port);
> > - }
> > -}
> > -
> > /* This function determines whether 'ofproto_port', which is attached to
> > * br->ofproto's datapath, is one that we want in 'br'.
> > *
> > @@ -1288,6 +1275,64 @@ bridge_refresh_ofp_port(struct bridge *br)
> > }
> > }
> >
> > +/* Opens a network device for 'iface_cfg' and configures it. If '*ofp_portp'
> > + * is negative, adds the network device to br->ofproto and stores the OpenFlow
> > + * port number in '*ofp_portp'; otherwise leaves br->ofproto and '*ofp_portp'
> > + * untouched.
> > + *
> > + * If successful, returns 0 and stores the network device in '*netdevp'. On
> > + * failure, returns a positive errno value and stores NULL in '*netdevp'. */
> > +static int
> > +iface_do_create(const struct bridge *br,
> > + const struct ovsrec_interface *iface_cfg,
> > + const struct ovsrec_port *port_cfg,
> > + int *ofp_portp, struct netdev **netdevp)
> > +{
> > + struct netdev *netdev;
> > + int error;
> > +
> > + error = netdev_open(iface_cfg->name,
> > + iface_get_type(iface_cfg, br->cfg), &netdev);
> > + if (error) {
> > + VLOG_WARN("could not open network device %s (%s)",
> > + iface_cfg->name, strerror(error));
> > + goto error;
> > + }
> > +
> > + error = iface_set_netdev_config(iface_cfg, netdev);
> > + if (error) {
> > + goto error;
> > + }
> > +
> > + if (*ofp_portp < 0) {
> > + uint16_t ofp_port;
> > +
> > + error = ofproto_port_add(br->ofproto, netdev, &ofp_port);
> > + if (error) {
> > + goto error;
> > + }
> > + *ofp_portp = ofp_port;
> > +
> > + VLOG_INFO("bridge %s: added interface %s on port %d",
> > + br->name, iface_cfg->name, *ofp_portp);
> > + } else {
> > + VLOG_DBG("bridge %s: interface %s is on port %d",
> > + br->name, iface_cfg->name, *ofp_portp);
> > + }
> > +
> > + if (port_cfg->vlan_mode && !strcmp(port_cfg->vlan_mode, "splinter")) {
> > + netdev_turn_flags_on(netdev, NETDEV_UP, true);
> > + }
> > +
> > + *netdevp = netdev;
> > + return 0;
> > +
> > +error:
> > + *netdevp = NULL;
> > + netdev_close(netdev);
> > + return error;
> > +}
> > +
> > /* Creates a new iface on 'br' based on 'if_cfg'. The new iface has OpenFlow
> > * port number 'ofp_port'. If ofp_port is negative, an OpenFlow port is
> > * automatically allocated for the iface. Takes ownership of and
> > @@ -1297,111 +1342,50 @@ bridge_refresh_ofp_port(struct bridge *br)
> > static bool
> > iface_create(struct bridge *br, struct if_cfg *if_cfg, int ofp_port)
> > {
> > + const struct ovsrec_interface *iface_cfg = if_cfg->cfg;
> > + const struct ovsrec_port *port_cfg = if_cfg->parent;
> > +
> > + struct netdev *netdev;
> > struct iface *iface;
> > struct port *port;
> > int error;
> > - bool ok;
> > -
> > - assert(!iface_lookup(br, if_cfg->cfg->name));
> > -
> > - port = port_lookup(br, if_cfg->parent->name);
> > - if (!port) {
> > - port = port_create(br, if_cfg->parent);
> > - }
> > -
> > - iface = xzalloc(sizeof *iface);
> > - iface->port = port;
> > - iface->name = xstrdup(if_cfg->cfg->name);
> > - iface->ofp_port = -1;
> > - iface->netdev = NULL;
> > - iface->cfg = if_cfg->cfg;
> > - hmap_insert(&br->iface_by_name, &iface->name_node,
> > - hash_string(iface->name, 0));
> > - list_push_back(&port->ifaces, &iface->port_elem);
> > - iface->type = iface_get_type(iface->cfg, br->cfg);
> > - if (ofp_port >= 0) {
> > - iface_set_ofp_port(iface, ofp_port);
> > - }
> >
> > + /* Get rid of 'if_cfg' itself. We already copied out the interesting
> > + * bits. */
> > hmap_remove(&br->if_cfg_todo, &if_cfg->hmap_node);
> > free(if_cfg);
> > - if_cfg = NULL;
> >
> > - error = netdev_open(iface->name, iface->type, &iface->netdev);
> > + /* Do the bits that can fail up front. */
> > + assert(!iface_lookup(br, iface_cfg->name));
> > + error = iface_do_create(br, iface_cfg, port_cfg, &ofp_port, &netdev);
> > if (error) {
> > - VLOG_WARN("could not open network device %s (%s)", iface->name,
> > - strerror(error));
> > - }
> > -
> > - if (iface->netdev
> > - && port->cfg->vlan_mode
> > - && !strcmp(port->cfg->vlan_mode, "splinter")) {
> > - netdev_turn_flags_on(iface->netdev, NETDEV_UP, true);
> > - }
> > -
> > - /* Configure the netdev. */
> > - if (iface->netdev) {
> > - int error = iface_set_netdev_config(iface->cfg, iface->netdev);
> > - if (error) {
> > - netdev_close(iface->netdev);
> > - iface->netdev = NULL;
> > - }
> > - }
> > -
> > - /* Add the port, if necessary. */
> > - if (iface->netdev && iface->ofp_port < 0) {
> > - uint16_t new_ofp_port;
> > - int error;
> > -
> > - error = ofproto_port_add(br->ofproto, iface->netdev, &new_ofp_port);
> > - if (!error) {
> > - VLOG_INFO("bridge %s: added interface %s (%d)", br->name,
> > - iface->name, new_ofp_port);
> > - iface_set_ofp_port(iface, new_ofp_port);
> > - } else {
> > - netdev_close(iface->netdev);
> > - iface->netdev = NULL;
> > - }
> > + iface_clear_db_record(iface_cfg);
> > + return false;
> > }
> >
> > - /* Initially populate stats columns. */
> > - if (iface->netdev) {
> > - iface_refresh_stats(iface);
> > - iface_refresh_status(iface);
> > + /* Get or create the port structure. */
> > + port = port_lookup(br, port_cfg->name);
> > + if (!port) {
> > + port = port_create(br, port_cfg);
> > }
> >
> > - /* Delete the iface if we failed. */
> > - ok = iface->netdev && iface->ofp_port >= 0;
> > - if (ok) {
> > - VLOG_DBG("bridge %s: interface %s is on port %d",
> > - br->name, iface->name, iface->ofp_port);
> > - } else {
> > - struct ofproto_port ofproto_port;
> > + /* Create the iface structure. */
> > + iface = xzalloc(sizeof *iface);
> > + list_push_back(&port->ifaces, &iface->port_elem);
> > + hmap_insert(&br->iface_by_name, &iface->name_node,
> > + hash_string(iface_cfg->name, 0));
> > + iface->port = port;
> > + iface->name = xstrdup(iface_cfg->name);
> > + iface->ofp_port = -1;
> > + iface->netdev = netdev;
> > + iface->type = iface_get_type(iface_cfg, br->cfg);
> > + iface->cfg = iface_cfg;
> >
> > - if (iface->netdev) {
> > - VLOG_ERR("bridge %s: missing %s interface, dropping",
> > - br->name, iface->name);
> > - } else {
> > - /* We already reported a related error, don't bother
> > - * duplicating it. */
> > - }
> > - if (!ofproto_port_query_by_name(br->ofproto, port->name,
> > - &ofproto_port)) {
> > - VLOG_INFO("bridge %s: removed interface %s (%d)",
> > - br->name, port->name, ofproto_port.ofp_port);
> > - bridge_ofproto_port_del(br, ofproto_port);
> > - ofproto_port_destroy(&ofproto_port);
> > - }
> > - iface_clear_db_record(iface->cfg);
> > - iface_destroy(iface);
> > - }
> > + iface_set_ofp_port(iface, ofp_port);
> >
> > - if (!ok) {
> > - if (list_is_empty(&port->ifaces)) {
> > - port_destroy(port);
> > - }
> > - return false;
> > - }
> > + /* Populate initial status in database. */
> > + iface_refresh_stats(iface);
> > + iface_refresh_status(iface);
> >
> > /* Add bond fake iface if necessary. */
> > if (port_is_bond_fake_iface(port)) {
> > --
> > 1.7.2.5
> >
> > _______________________________________________
> > dev mailing list
> > dev at openvswitch.org
> > http://openvswitch.org/mailman/listinfo/dev
More information about the dev
mailing list