From b5827b24c7c136b088e719bfe2020de31032ae37 Mon Sep 17 00:00:00 2001 From: Ben Pfaff Date: Wed, 4 May 2011 10:15:31 -0700 Subject: [PATCH] bridge: Eliminate direct dependencies on dpif. The 'ofp_portp' argument of the new function ofproto_port_add() is always set to NULL in this commit, but a future commit will use nonnull values. --- ofproto/ofproto.c | 167 ++++++++++++++++++++++++++++++++++++-- ofproto/ofproto.h | 45 +++++++++- utilities/ovs-openflowd.c | 17 ++-- vswitchd/bridge.c | 106 +++++++++++------------- 4 files changed, 259 insertions(+), 76 deletions(-) diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c index 6aceedb7..45c51253 100644 --- a/ofproto/ofproto.c +++ b/ofproto/ofproto.c @@ -358,7 +358,7 @@ ofproto_create(const char *datapath, const char *datapath_type, ofproto_unixctl_init(); /* Connect to datapath and start listening for messages. */ - error = dpif_open(datapath, datapath_type, &dpif); + error = dpif_create_and_open(datapath, datapath_type, &dpif); if (error) { VLOG_ERR("failed to open datapath %s: %s", datapath, strerror(error)); return error; @@ -675,8 +675,8 @@ ofproto_get_snoops(const struct ofproto *ofproto, struct sset *snoops) connmgr_get_snoops(ofproto->connmgr, snoops); } -void -ofproto_destroy(struct ofproto *p) +static void +ofproto_destroy__(struct ofproto *p, bool delete) { struct ofport *ofport, *next_ofport; @@ -691,7 +691,15 @@ ofproto_destroy(struct ofproto *p) classifier_destroy(&p->cls); hmap_destroy(&p->facets); + if (delete) { + int error = dpif_delete(p->dpif); + if (error && error != ENOENT) { + VLOG_ERR("bridge %s: failed to destroy (%s)", + dpif_name(p->dpif), strerror(error)); + } + } dpif_close(p->dpif); + netdev_monitor_destroy(p->netdev_monitor); HMAP_FOR_EACH_SAFE (ofport, next_ofport, hmap_node, &p->ports) { hmap_remove(&p->ports, &ofport->hmap_node); @@ -715,6 +723,18 @@ ofproto_destroy(struct ofproto *p) free(p); } +void +ofproto_destroy(struct ofproto *p) +{ + ofproto_destroy__(p, false); +} + +void +ofproto_destroy_and_delete(struct ofproto *p) +{ + ofproto_destroy__(p, true); +} + int ofproto_run(struct ofproto *p) { @@ -893,7 +913,134 @@ ofproto_free_ofproto_controller_info(struct shash *info) shash_destroy(info); } -/* Deletes port number 'odp_port' from the datapath for 'ofproto'. +/* Makes a deep copy of 'old' into 'port'. */ +void +ofproto_port_clone(struct ofproto_port *port, const struct ofproto_port *old) +{ + port->name = xstrdup(old->name); + port->type = xstrdup(old->type); + port->ofp_port = old->ofp_port; +} + +/* Frees memory allocated to members of 'ofproto_port'. + * + * Do not call this function on an ofproto_port obtained from + * ofproto_port_dump_next(): that function retains ownership of the data in the + * ofproto_port. */ +void +ofproto_port_destroy(struct ofproto_port *ofproto_port) +{ + free(ofproto_port->name); + free(ofproto_port->type); +} + +/* Converts a dpif_port into an ofproto_port. + * + * This only makes a shallow copy, so make sure that the dpif_port doesn't get + * freed while the ofproto_port is still in use. You can choose to free the + * ofproto_port instead of the dpif_port. */ +static void +ofproto_port_from_dpif_port(struct ofproto_port *ofproto_port, + struct dpif_port *dpif_port) +{ + ofproto_port->name = dpif_port->name; + ofproto_port->type = dpif_port->type; + ofproto_port->ofp_port = odp_port_to_ofp_port(dpif_port->port_no); +} + +/* Initializes 'dump' to begin dumping the ports in an ofproto. + * + * This function provides no status indication. An error status for the entire + * dump operation is provided when it is completed by calling + * ofproto_port_dump_done(). + */ +void +ofproto_port_dump_start(struct ofproto_port_dump *dump, + const struct ofproto *ofproto) +{ + struct dpif_port_dump *dpif_dump; + + dump->state = dpif_dump = xmalloc(sizeof *dpif_dump); + dpif_port_dump_start(dpif_dump, ofproto->dpif); +} + +/* Attempts to retrieve another port from 'dump', which must have been created + * with ofproto_port_dump_start(). On success, stores a new ofproto_port into + * 'port' and returns true. On failure, returns false. + * + * Failure might indicate an actual error or merely that the last port has been + * dumped. An error status for the entire dump operation is provided when it + * is completed by calling ofproto_port_dump_done(). + * + * The ofproto owns the data stored in 'port'. It will remain valid until at + * least the next time 'dump' is passed to ofproto_port_dump_next() or + * ofproto_port_dump_done(). */ +bool +ofproto_port_dump_next(struct ofproto_port_dump *dump, + struct ofproto_port *port) +{ + struct dpif_port_dump *dpif_dump = dump->state; + struct dpif_port dpif_port; + bool ok; + + ok = dpif_port_dump_next(dpif_dump, &dpif_port); + if (ok) { + ofproto_port_from_dpif_port(port, &dpif_port); + } + return ok; +} + +/* Completes port table dump operation 'dump', which must have been created + * with ofproto_port_dump_start(). Returns 0 if the dump operation was + * error-free, otherwise a positive errno value describing the problem. */ +int +ofproto_port_dump_done(struct ofproto_port_dump *dump) +{ + struct dpif_port_dump *dpif_dump = dump->state; + int error = dpif_port_dump_done(dpif_dump); + free(dpif_dump); + return error; +} + +/* Attempts to add 'netdev' as a port on 'ofproto'. If successful, returns 0 + * and sets '*ofp_portp' to the new port's port OpenFlow number (if 'ofp_portp' + * is non-null). On failure, returns a positive errno value and sets + * '*ofp_portp' to OFPP_NONE (if 'ofp_portp' is non-null). */ +int +ofproto_port_add(struct ofproto *ofproto, struct netdev *netdev, + uint16_t *ofp_portp) +{ + uint16_t odp_port; + int error; + + error = dpif_port_add(ofproto->dpif, netdev, &odp_port); + if (ofp_portp) { + *ofp_portp = error ? OFPP_NONE : odp_port_to_ofp_port(odp_port); + } + return error; +} + +/* Looks up a port named 'devname' in 'ofproto'. On success, returns 0 and + * initializes '*port' appropriately; on failure, returns a positive errno + * value. + * + * The caller owns the data in 'port' and must free it with + * ofproto_port_destroy() when it is no longer needed. */ +int +ofproto_port_query_by_name(const struct ofproto *ofproto, const char *devname, + struct ofproto_port *port) +{ + struct dpif_port dpif_port; + int error; + + error = dpif_port_query_by_name(ofproto->dpif, devname, &dpif_port); + if (!error) { + ofproto_port_from_dpif_port(port, &dpif_port); + } + return error; +} + +/* Deletes port number 'ofp_port' from the datapath for 'ofproto'. * * This is almost the same as calling dpif_port_del() directly on the * datapath, but it also makes 'ofproto' close its open netdev for the port @@ -904,8 +1051,9 @@ ofproto_free_ofproto_controller_info(struct shash *info) * * Returns 0 if successful, otherwise a positive errno. */ int -ofproto_port_del(struct ofproto *ofproto, uint16_t odp_port) +ofproto_port_del(struct ofproto *ofproto, uint16_t ofp_port) { + uint32_t odp_port = ofp_port_to_odp_port(ofp_port); struct ofport *ofport = get_port(ofproto, odp_port); const char *name = ofport ? netdev_get_name(ofport->netdev) : ""; int error; @@ -3022,6 +3170,15 @@ ofproto_get_all_flows(struct ofproto *p, struct ds *results) } } +/* Obtains the NetFlow engine type and engine ID for 'ofproto' into + * '*engine_type' and '*engine_id', respectively. */ +void +ofproto_get_netflow_ids(const struct ofproto *ofproto, + uint8_t *engine_type, uint8_t *engine_id) +{ + dpif_get_netflow_ids(ofproto->dpif, engine_type, engine_id); +} + static void query_aggregate_stats(struct ofproto *ofproto, struct cls_rule *target, ovs_be16 out_port, uint8_t table_id, diff --git a/ofproto/ofproto.h b/ofproto/ofproto.h index de5b92d1..4860ba36 100644 --- a/ofproto/ofproto.h +++ b/ofproto/ofproto.h @@ -33,6 +33,8 @@ extern "C" { struct cfm; struct cls_rule; +struct dpif_port; +struct netdev; struct nlattr; struct ofhooks; struct ofproto; @@ -97,15 +99,54 @@ int ofproto_create(const char *datapath, const char *datapath_type, const struct ofhooks *, void *aux, struct ofproto **ofprotop); void ofproto_destroy(struct ofproto *); +void ofproto_destroy_and_delete(struct ofproto *); int ofproto_run(struct ofproto *); int ofproto_run1(struct ofproto *); int ofproto_run2(struct ofproto *, bool revalidate_all); void ofproto_wait(struct ofproto *); bool ofproto_is_alive(const struct ofproto *); -int ofproto_port_del(struct ofproto *, uint16_t odp_port); +/* A port within an OpenFlow switch. + * + * 'name' and 'type' are suitable for passing to netdev_open(). */ +struct ofproto_port { + char *name; /* Network device name, e.g. "eth0". */ + char *type; /* Network device type, e.g. "system". */ + uint16_t ofp_port; /* OpenFlow port number. */ +}; +void ofproto_port_clone(struct ofproto_port *, const struct ofproto_port *); +void ofproto_port_destroy(struct ofproto_port *); + +struct ofproto_port_dump { + const struct ofproto *ofproto; + int error; + void *state; +}; +void ofproto_port_dump_start(struct ofproto_port_dump *, + const struct ofproto *); +bool ofproto_port_dump_next(struct ofproto_port_dump *, struct ofproto_port *); +int ofproto_port_dump_done(struct ofproto_port_dump *); + +/* Iterates through each DPIF_PORT in OFPROTO, using DUMP as state. + * + * Arguments all have pointer type. + * + * If you break out of the loop, then you need to free the dump structure by + * hand using ofproto_port_dump_done(). */ +#define OFPROTO_PORT_FOR_EACH(OFPROTO_PORT, DUMP, OFPROTO) \ + for (ofproto_port_dump_start(DUMP, OFPROTO); \ + (ofproto_port_dump_next(DUMP, OFPROTO_PORT) \ + ? true \ + : (ofproto_port_dump_done(DUMP), false)); \ + ) + +int ofproto_port_add(struct ofproto *, struct netdev *, uint16_t *ofp_portp); +int ofproto_port_del(struct ofproto *, uint16_t ofp_port); bool ofproto_port_is_floodable(struct ofproto *, uint16_t odp_port); +int ofproto_port_query_by_name(const struct ofproto *, const char *devname, + struct ofproto_port *); + /* Top-level configuration. */ void ofproto_set_datapath_id(struct ofproto *, uint64_t datapath_id); void ofproto_set_controllers(struct ofproto *, @@ -138,6 +179,8 @@ void ofproto_get_listeners(const struct ofproto *, struct sset *); bool ofproto_has_snoops(const struct ofproto *); void ofproto_get_snoops(const struct ofproto *, struct sset *); void ofproto_get_all_flows(struct ofproto *p, struct ds *); +void ofproto_get_netflow_ids(const struct ofproto *, + uint8_t *engine_type, uint8_t *engine_id); /* Functions for use by ofproto implementation modules, not by clients. */ void ofproto_add_flow(struct ofproto *, const struct cls_rule *, diff --git a/utilities/ovs-openflowd.c b/utilities/ovs-openflowd.c index 86f5ca59..f54eb355 100644 --- a/utilities/ovs-openflowd.c +++ b/utilities/ovs-openflowd.c @@ -93,7 +93,6 @@ main(int argc, char *argv[]) struct ofproto *ofproto; struct ofsettings s; int error; - struct dpif *dpif; struct netflow_options nf_options; const char *port; bool exiting; @@ -116,9 +115,10 @@ main(int argc, char *argv[]) VLOG_INFO("Open vSwitch version %s", VERSION BUILDNR); VLOG_INFO("OpenFlow protocol version 0x%02x", OFP_VERSION); - error = dpif_create_and_open(s.dp_name, s.dp_type, &dpif); + error = ofproto_create(s.dp_name, s.dp_type, NULL, NULL, &ofproto); if (error) { - VLOG_FATAL("could not create datapath (%s)", strerror(error)); + VLOG_FATAL("could not initialize OpenFlow switch (%s)", + strerror(error)); } /* Add ports to the datapath if requested by the user. */ @@ -131,7 +131,7 @@ main(int argc, char *argv[]) port, strerror(error)); } - error = dpif_port_add(dpif, netdev, NULL); + error = ofproto_port_add(ofproto, netdev, NULL); if (error) { VLOG_FATAL("failed to add %s as a port (%s)", port, strerror(error)); @@ -140,12 +140,7 @@ main(int argc, char *argv[]) netdev_close(netdev); } - /* Start OpenFlow processing. */ - error = ofproto_create(s.dp_name, s.dp_type, NULL, NULL, &ofproto); - if (error) { - VLOG_FATAL("could not initialize openflow switch (%s)", - strerror(error)); - } + /* Configure OpenFlow switch. */ if (s.datapath_id) { ofproto_set_datapath_id(ofproto, s.datapath_id); } @@ -188,7 +183,7 @@ main(int argc, char *argv[]) poll_block(); } - dpif_close(dpif); + ofproto_destroy(ofproto); return 0; } diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c index ecde7160..898c455c 100644 --- a/vswitchd/bridge.c +++ b/vswitchd/bridge.c @@ -171,12 +171,9 @@ struct bridge { /* OpenFlow switch processing. */ struct ofproto *ofproto; /* OpenFlow switch. */ - /* Kernel datapath information. */ - struct dpif *dpif; /* Datapath. */ - struct hmap ifaces; /* "struct iface"s indexed by dp_ifidx. */ - /* Bridge ports. */ struct hmap ports; /* "struct port"s indexed by name. */ + struct hmap ifaces; /* "struct iface"s indexed by dp_ifidx. */ struct hmap iface_by_name; /* "struct iface"s indexed by name. */ /* Bonding. */ @@ -542,18 +539,19 @@ bridge_reconfigure(const struct ovsrec_open_vswitch *ovs_cfg) * that port already belongs to a different datapath, so we must do all * port deletions before any port additions. */ LIST_FOR_EACH (br, node, &all_bridges) { - struct dpif_port_dump dump; + struct ofproto_port ofproto_port; + struct ofproto_port_dump dump; struct shash want_ifaces; - struct dpif_port dpif_port; bridge_get_all_ifaces(br, &want_ifaces); - DPIF_PORT_FOR_EACH (&dpif_port, &dump, br->dpif) { - if (!shash_find(&want_ifaces, dpif_port.name) - && strcmp(dpif_port.name, br->name)) { - int retval = dpif_port_del(br->dpif, dpif_port.port_no); + OFPROTO_PORT_FOR_EACH (&ofproto_port, &dump, br->ofproto) { + if (!shash_find(&want_ifaces, ofproto_port.name) + && strcmp(ofproto_port.name, br->name)) { + int retval = ofproto_port_del(br->ofproto, + ofproto_port.ofp_port); if (retval) { VLOG_WARN("bridge %s: failed to remove %s interface (%s)", - br->name, dpif_port.name, strerror(retval)); + br->name, ofproto_port.name, strerror(retval)); } } } @@ -561,15 +559,15 @@ bridge_reconfigure(const struct ovsrec_open_vswitch *ovs_cfg) } LIST_FOR_EACH (br, node, &all_bridges) { struct shash cur_ifaces, want_ifaces; - struct dpif_port_dump dump; - struct dpif_port dpif_port; + struct ofproto_port ofproto_port; + struct ofproto_port_dump dump; /* Get the set of interfaces currently in this datapath. */ shash_init(&cur_ifaces); - DPIF_PORT_FOR_EACH (&dpif_port, &dump, br->dpif) { - struct dpif_port *port_info = xmalloc(sizeof *port_info); - dpif_port_clone(port_info, &dpif_port); - shash_add(&cur_ifaces, dpif_port.name, port_info); + OFPROTO_PORT_FOR_EACH (&ofproto_port, &dump, br->ofproto) { + struct ofproto_port *port_info = xmalloc(sizeof *port_info); + ofproto_port_clone(port_info, &ofproto_port); + shash_add(&cur_ifaces, ofproto_port.name, port_info); } /* Get the set of interfaces we want on this datapath. */ @@ -579,25 +577,26 @@ bridge_reconfigure(const struct ovsrec_open_vswitch *ovs_cfg) SHASH_FOR_EACH (node, &want_ifaces) { const char *if_name = node->name; struct iface *iface = node->data; - struct dpif_port *dpif_port; + struct ofproto_port *ofproto_port; const char *type; int error; type = iface ? iface->type : "internal"; - dpif_port = shash_find_data(&cur_ifaces, if_name); + ofproto_port = shash_find_data(&cur_ifaces, if_name); /* If we have a port or a netdev already, and it's not the type we * want, then delete the port (if any) and close the netdev (if * any). */ - if ((dpif_port && strcmp(dpif_port->type, type)) + if ((ofproto_port && strcmp(ofproto_port->type, type)) || (iface && iface->netdev && strcmp(type, netdev_get_type(iface->netdev)))) { - if (dpif_port) { - error = ofproto_port_del(br->ofproto, dpif_port->port_no); + if (ofproto_port) { + error = ofproto_port_del(br->ofproto, + ofproto_port->ofp_port); if (error) { continue; } - dpif_port = NULL; + ofproto_port = NULL; } if (iface) { if (iface->port->bond) { @@ -614,7 +613,7 @@ bridge_reconfigure(const struct ovsrec_open_vswitch *ovs_cfg) /* If the port doesn't exist or we don't have the netdev open, * we need to do more work. */ - if (!dpif_port || (iface && !iface->netdev)) { + if (!ofproto_port || (iface && !iface->netdev)) { struct netdev_options options; struct netdev *netdev; struct shash args; @@ -641,8 +640,8 @@ bridge_reconfigure(const struct ovsrec_open_vswitch *ovs_cfg) } /* Then add the port if we haven't already. */ - if (!dpif_port) { - error = dpif_port_add(br->dpif, netdev, NULL); + if (!ofproto_port) { + error = ofproto_port_add(br->ofproto, netdev, NULL); if (error) { netdev_close(netdev); if (error == EFBIG) { @@ -676,8 +675,8 @@ bridge_reconfigure(const struct ovsrec_open_vswitch *ovs_cfg) shash_destroy(&want_ifaces); SHASH_FOR_EACH (node, &cur_ifaces) { - struct dpif_port *port_info = node->data; - dpif_port_destroy(port_info); + struct ofproto_port *port_info = node->data; + ofproto_port_destroy(port_info); free(port_info); } shash_destroy(&cur_ifaces); @@ -752,7 +751,8 @@ bridge_reconfigure(const struct ovsrec_open_vswitch *ovs_cfg) memset(&opts, 0, sizeof opts); - dpif_get_netflow_ids(br->dpif, &opts.engine_type, &opts.engine_id); + ofproto_get_netflow_ids(br->ofproto, + &opts.engine_type, &opts.engine_id); if (nf_cfg->engine_type) { opts.engine_type = *nf_cfg->engine_type; } @@ -1638,20 +1638,11 @@ bridge_create(const struct ovsrec_bridge *br_cfg) assert(!bridge_lookup(br_cfg->name)); br = xzalloc(sizeof *br); - error = dpif_create_and_open(br_cfg->name, br_cfg->datapath_type, - &br->dpif); - if (error) { - free(br); - return NULL; - } - error = ofproto_create(br_cfg->name, br_cfg->datapath_type, &bridge_ofhooks, br, &br->ofproto); if (error) { VLOG_ERR("failed to create switch %s: %s", br_cfg->name, strerror(error)); - dpif_delete(br->dpif); - dpif_close(br->dpif); free(br); return NULL; } @@ -1679,7 +1670,6 @@ bridge_destroy(struct bridge *br) { if (br) { struct port *port, *next; - int error; int i; HMAP_FOR_EACH_SAFE (port, next, hmap_node, &br->ports) { @@ -1689,13 +1679,7 @@ bridge_destroy(struct bridge *br) mirror_destroy(br->mirrors[i]); } list_remove(&br->node); - ofproto_destroy(br->ofproto); - error = dpif_delete(br->dpif); - if (error && error != ENOENT) { - VLOG_ERR("bridge %s: failed to destroy (%s)", - br->name, strerror(error)); - } - dpif_close(br->dpif); + ofproto_destroy_and_delete(br->ofproto); mac_learning_destroy(br->ml); hmap_destroy(&br->ifaces); hmap_destroy(&br->ports); @@ -1826,16 +1810,18 @@ bridge_reconfigure_one(struct bridge *br) br->name, name); } } - if (!shash_find(&new_ports, br->name)) { - struct dpif_port dpif_port; + if (bridge_get_controllers(br, NULL) + && !shash_find(&new_ports, br->name)) { + struct ofproto_port local_port; char *type; + int error; VLOG_WARN("bridge %s: no port named %s, synthesizing one", br->name, br->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); + error = ofproto_port_query_by_name(br->ofproto, br->name, &local_port); + type = xstrdup(error ? "internal" : local_port.type); + ofproto_port_destroy(&local_port); br->synth_local_port.interfaces = &br->synth_local_ifacep; br->synth_local_port.n_interfaces = 1; @@ -2073,8 +2059,8 @@ bridge_get_all_ifaces(const struct bridge *br, struct shash *ifaces) static void bridge_fetch_dp_ifaces(struct bridge *br) { - struct dpif_port_dump dump; - struct dpif_port dpif_port; + struct ofproto_port ofproto_port; + struct ofproto_port_dump dump; struct port *port; /* Reset all interface numbers. */ @@ -2087,17 +2073,19 @@ bridge_fetch_dp_ifaces(struct bridge *br) } hmap_clear(&br->ifaces); - DPIF_PORT_FOR_EACH (&dpif_port, &dump, br->dpif) { - struct iface *iface = iface_lookup(br, dpif_port.name); + OFPROTO_PORT_FOR_EACH (&ofproto_port, &dump, br->ofproto) { + struct iface *iface = iface_lookup(br, ofproto_port.name); if (iface) { + uint32_t odp_port = ofp_port_to_odp_port(ofproto_port.ofp_port); + if (iface->dp_ifidx >= 0) { VLOG_WARN("bridge %s: interface %s reported twice", - br->name, dpif_port.name); - } else if (iface_from_dp_ifidx(br, dpif_port.port_no)) { + br->name, ofproto_port.name); + } else if (iface_from_dp_ifidx(br, odp_port)) { VLOG_WARN("bridge %s: interface %"PRIu16" reported twice", - br->name, dpif_port.port_no); + br->name, odp_port); } else { - iface->dp_ifidx = dpif_port.port_no; + iface->dp_ifidx = odp_port; hmap_insert(&br->ifaces, &iface->dp_ifidx_node, hash_int(iface->dp_ifidx, 0)); } -- 2.30.2