From: Ben Pfaff Date: Tue, 24 Apr 2012 23:53:01 +0000 (-0700) Subject: vswitchd: Clean up iface_create(). X-Git-Url: https://pintos-os.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=cc0864659fc924b1632754168bfa939bf1942492;p=openvswitch vswitchd: Clean up iface_create(). 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 --- diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c index a5e8b466..61bf21c6 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)) {