From df773b8b81a59ebf43a5a9502c05a73dfc83ddee Mon Sep 17 00:00:00 2001 From: Ben Pfaff Date: Thu, 8 Apr 2010 12:31:14 -0700 Subject: [PATCH] wdp: Change functions that use wdp_port to avoid awkward memory semantics. wdp_port_query_by_number(), wdp_port_query_by_name(), and wdp_port_list() all had semantics for the data that they returned that were difficult to describe and easy to get wrong. This commit changes them to be easier to understand and to implement. Suggested by partner. --- ofproto/ofproto.c | 55 ++++++++++++++------------- ofproto/wdp-provider.h | 32 +++++++--------- ofproto/wdp-xflow.c | 32 ++++++++++------ ofproto/wdp.c | 86 +++++++++++++++++++++++++++++++----------- ofproto/wdp.h | 13 ++++--- 5 files changed, 137 insertions(+), 81 deletions(-) diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c index 7629dce44..344246102 100644 --- a/ofproto/ofproto.c +++ b/ofproto/ofproto.c @@ -1133,7 +1133,7 @@ static int handle_port_mod(struct ofproto *p, struct ofp_header *oh) { const struct ofp_port_mod *opm; - struct wdp_port *port; + struct wdp_port port; int error; error = check_ofp_message(oh, OFPT_PORT_MOD, sizeof *opm); @@ -1142,11 +1142,10 @@ handle_port_mod(struct ofproto *p, struct ofp_header *oh) } opm = (struct ofp_port_mod *) oh; - wdp_port_query_by_number(p->wdp, ntohs(opm->port_no), &port); - if (!port) { - return ofp_mkerr(OFPET_PORT_MOD_FAILED, OFPPMFC_BAD_PORT); - } else if (memcmp(port->opp.hw_addr, opm->hw_addr, OFP_ETH_ALEN)) { - return ofp_mkerr(OFPET_PORT_MOD_FAILED, OFPPMFC_BAD_HW_ADDR); + if (wdp_port_query_by_number(p->wdp, ntohs(opm->port_no), &port)) { + error = ofp_mkerr(OFPET_PORT_MOD_FAILED, OFPPMFC_BAD_PORT); + } else if (memcmp(port.opp.hw_addr, opm->hw_addr, OFP_ETH_ALEN)) { + error = ofp_mkerr(OFPET_PORT_MOD_FAILED, OFPPMFC_BAD_HW_ADDR); } else { uint32_t mask, new_config; @@ -1154,16 +1153,18 @@ handle_port_mod(struct ofproto *p, struct ofp_header *oh) | OFPPC_NO_RECV | OFPPC_NO_RECV_STP | OFPPC_NO_FLOOD | OFPPC_NO_FWD | OFPPC_NO_PACKET_IN); - new_config = (port->opp.config & ~mask) | (ntohl(opm->config) & mask); - if (new_config != port->opp.config) { + new_config = (port.opp.config & ~mask) | (ntohl(opm->config) & mask); + if (new_config != port.opp.config) { wdp_port_set_config(p->wdp, ntohs(opm->port_no), new_config); } if (opm->advertise) { - netdev_set_advertisements(port->netdev, ntohl(opm->advertise)); + netdev_set_advertisements(port.netdev, ntohl(opm->advertise)); } + error = 0; } + wdp_port_free(&port); - return 0; + return error; } static struct ofpbuf * @@ -1302,22 +1303,22 @@ handle_port_stats_request(struct ofproto *p, struct ofconn *ofconn, msg = start_stats_reply(osr, sizeof *ops * 16); if (psr->port_no != htons(OFPP_NONE)) { - struct wdp_port *port; + struct wdp_port port; - wdp_port_query_by_number(p->wdp, ntohs(psr->port_no), &port); - if (port) { - append_port_stat(port, ofconn, msg); + if (!wdp_port_query_by_number(p->wdp, ntohs(psr->port_no), &port)) { + append_port_stat(&port, ofconn, msg); + wdp_port_free(&port); } } else { - struct wdp_port **ports; + struct wdp_port *ports; size_t n_ports; size_t i; wdp_port_list(p->wdp, &ports, &n_ports); for (i = 0; i < n_ports; i++) { - append_port_stat(ports[i], ofconn, msg); + append_port_stat(&ports[i], ofconn, msg); } - free(ports); + wdp_port_array_free(ports, n_ports); } queue_tx(msg, ofconn, ofconn->reply_counter); @@ -1873,11 +1874,12 @@ handle_flow_miss(struct ofproto *p, struct wdp_packet *packet) rule = wdp_flow_match(p->wdp, &flow); if (!rule) { /* Don't send a packet-in if OFPPC_NO_PACKET_IN asserted. */ - struct wdp_port *port; + struct wdp_port port; - wdp_port_query_by_number(p->wdp, packet->in_port, &port); - if (port) { - if (port->opp.config & OFPPC_NO_PACKET_IN) { + if (!wdp_port_query_by_number(p->wdp, packet->in_port, &port)) { + bool no_packet_in = (port.opp.config & OFPPC_NO_PACKET_IN) != 0; + wdp_port_free(&port); + if (no_packet_in) { COVERAGE_INC(ofproto_no_packet_in); wdp_packet_destroy(packet); return; @@ -2066,19 +2068,20 @@ send_packet_in_miss(struct wdp_packet *packet, void *p_) static uint64_t pick_datapath_id(const struct ofproto *ofproto) { - struct wdp_port *port; + struct wdp_port port; - wdp_port_query_by_number(ofproto->wdp, OFPP_LOCAL, &port); - if (port) { + if (!wdp_port_query_by_number(ofproto->wdp, OFPP_LOCAL, &port)) { uint8_t ea[ETH_ADDR_LEN]; int error; - error = netdev_get_etheraddr(port->netdev, ea); + error = netdev_get_etheraddr(port.netdev, ea); if (!error) { + wdp_port_free(&port); return eth_addr_to_uint64(ea); } VLOG_WARN("could not get MAC address for %s (%s)", - netdev_get_name(port->netdev), strerror(error)); + netdev_get_name(port.netdev), strerror(error)); + wdp_port_free(&port); } return ofproto->fallback_dpid; diff --git a/ofproto/wdp-provider.h b/ofproto/wdp-provider.h index 8c52295cf..b12ace9d2 100644 --- a/ofproto/wdp-provider.h +++ b/ofproto/wdp-provider.h @@ -188,27 +188,23 @@ struct wdp_class { int (*port_del)(struct wdp *wdp, uint16_t port_no); /* Looks up a port in 'wdp' by name or number. On success, returns 0 and - * points '*portp' to a wdp_port representing the specified port. On - * failure, returns a positive errno value and sets '*portp' to NULL. + * initializes '*portp'. On failure, returns a positive errno value. * - * The caller is not allowed to modify or free the returned wdp_port. The - * wdp_port must remain accessible until the next call to the 'run' member - * function for this class or or wdp_port_poll() for 'wdp'. */ + * The caller takes ownership of everything in '*portp' and will eventually + * free it with, e.g., wdp_port_free(). */ int (*port_query_by_number)(const struct wdp *wdp, uint16_t port_no, - struct wdp_port **portp); + struct wdp_port *portp); int (*port_query_by_name)(const struct wdp *wdp, const char *devname, - struct wdp_port **portp); - - /* Obtains a list of all the ports in 'wdp'. Sets '*portsp' to point to - * an array of pointers to port structures and '*n_portsp' to the number of - * pointers in the array. - * - * The caller is responsible for freeing '*portsp' by calling free(). The - * calleris not allowed to modify or free the individual wdp_port - * structures. The wdp_ports must remain accessible until the next call to - * the 'run' member function for this class or or wdp_port_poll() for - * 'wdp'. */ - int (*port_list)(const struct wdp *wdp, struct wdp_port ***portsp, + struct wdp_port *portp); + + /* Obtains a list of all the ports in 'wdp'. Sets '*portsp' to point to an + * array of port structures and '*n_portsp' to the number of ports in the + * array. + * + * The caller takes ownership of '*portsp' and all of the ports in it and + * is responsible for freeing the ports and the array with, e.g., + * wdp_port_array_free(). */ + int (*port_list)(const struct wdp *wdp, struct wdp_port **portsp, size_t *n_portsp); int (*port_set_config)(struct wdp *sdpif, uint16_t port_no, diff --git a/ofproto/wdp-xflow.c b/ofproto/wdp-xflow.c index d0975a7f8..1c5fb929b 100644 --- a/ofproto/wdp-xflow.c +++ b/ofproto/wdp-xflow.c @@ -1313,24 +1313,36 @@ wx_port_del(struct wdp *wdp, uint16_t port_no) return xfif_port_del(wx->xfif, port_no); } +static int +wx_answer_port_query(const struct wdp_port *port, struct wdp_port *portp) +{ + if (port) { + wdp_port_copy(portp, port); + return 0; + } else { + return ENOENT; + } +} + static int wx_port_query_by_number(const struct wdp *wdp, uint16_t port_no, - struct wdp_port **portp) + struct wdp_port *portp) { struct wx *wx = wx_cast(wdp); + const struct wdp_port *port; - *portp = port_array_get(&wx->ports, ofp_port_to_xflow_port(port_no)); - return *portp ? 0 : ENOENT; + port = port_array_get(&wx->ports, ofp_port_to_xflow_port(port_no)); + return wx_answer_port_query(port, portp); } static int wx_port_query_by_name(const struct wdp *wdp, const char *devname, - struct wdp_port **portp) + struct wdp_port *portp) { struct wx *wx = wx_cast(wdp); - *portp = shash_find_data(&wx->port_by_name, devname); - return *portp ? 0 : ENOENT; + return wx_answer_port_query(shash_find_data(&wx->port_by_name, devname), + portp); } static int @@ -1379,12 +1391,10 @@ wx_port_set_config(struct wdp *wdp, uint16_t port_no, uint32_t config) } static int -wx_port_list(const struct wdp *wdp, struct wdp_port ***portsp, - size_t *n_portsp) +wx_port_list(const struct wdp *wdp, struct wdp_port **portsp, size_t *n_portsp) { struct wx *wx = wx_cast(wdp); - struct wdp_port **ports; - struct wdp_port *port; + struct wdp_port *ports, *port; unsigned int port_no; size_t n_ports, i; @@ -1392,7 +1402,7 @@ wx_port_list(const struct wdp *wdp, struct wdp_port ***portsp, *portsp = ports = xmalloc(n_ports * sizeof *ports); i = 0; PORT_ARRAY_FOR_EACH (port, &wx->ports, port_no) { - ports[i++] = port; + wdp_port_copy(&ports[i++], port); } assert(i == n_ports); diff --git a/ofproto/wdp.c b/ofproto/wdp.c index 52f6027cc..31f129238 100644 --- a/ofproto/wdp.c +++ b/ofproto/wdp.c @@ -27,6 +27,7 @@ #include "coverage.h" #include "dynamic-string.h" #include "flow.h" +#include "netdev.h" #include "netlink.h" #include "ofp-print.h" #include "ofpbuf.h" @@ -478,6 +479,47 @@ wdp_set_drop_frags(struct wdp *wdp, bool drop_frags) return error; } +/* Clears the contents of 'port'. */ +void +wdp_port_clear(struct wdp_port *port) +{ + memset(port, 0, sizeof *port); +} + +/* Makes a deep copy of 'old' in 'port'. The caller may free 'port''s data + * with wdp_port_free(). */ +void +wdp_port_copy(struct wdp_port *port, const struct wdp_port *old) +{ + port->netdev = old->netdev ? netdev_reopen(old->netdev) : NULL; + port->opp = old->opp; + port->devname = old->devname ? xstrdup(old->devname) : NULL; + port->internal = old->internal; +} + +/* Frees the data that 'port' points to (but not 'port' itself). */ +void +wdp_port_free(struct wdp_port *port) +{ + if (port) { + netdev_close(port->netdev); + free(port->devname); + } +} + +/* Frees the data that each of the 'n' ports in 'ports' points to, and then + * frees 'ports' itself. */ +void +wdp_port_array_free(struct wdp_port *ports, size_t n) +{ + size_t i; + + for (i = 0; i < n; i++) { + wdp_port_free(&ports[i]); + } + free(ports); +} + /* Attempts to add 'devname' as a port on 'wdp': * * - If 'internal' is true, attempts to create a new internal port (a virtual @@ -561,9 +603,9 @@ wdp_port_del(struct wdp *wdp, uint16_t port_no) return error; } -/* Looks up port number 'port_no' in 'wdp'. On success, returns 0 and points - * '*portp' to a wdp_port representing the specified port. On failure, returns - * a positive errno value and sets '*portp' to NULL. +/* Looks up port number 'port_no' in 'wdp'. On success, returns 0 and + * initializes 'port' with port details. On failure, returns a positive errno + * value and clears the contents of 'port' (with wdp_port_clear()). * * The caller must not modify or free the returned wdp_port. Calling * wdp_run() or wdp_port_poll() may free the returned wdp_port. @@ -576,16 +618,16 @@ wdp_port_del(struct wdp *wdp, uint16_t port_no) */ int wdp_port_query_by_number(const struct wdp *wdp, uint16_t port_no, - struct wdp_port **portp) + struct wdp_port *port) { int error; - error = wdp->wdp_class->port_query_by_number(wdp, port_no, portp); + error = wdp->wdp_class->port_query_by_number(wdp, port_no, port); if (!error) { VLOG_DBG_RL(&wdpmsg_rl, "%s: port %"PRIu16" is device %s", - wdp_name(wdp), port_no, (*portp)->devname); + wdp_name(wdp), port_no, port->devname); } else { - *portp = NULL; + wdp_port_clear(port); VLOG_WARN_RL(&error_rl, "%s: failed to query port %"PRIu16": %s", wdp_name(wdp), port_no, strerror(error)); } @@ -603,14 +645,14 @@ wdp_port_query_by_number(const struct wdp *wdp, uint16_t port_no, */ int wdp_port_query_by_name(const struct wdp *wdp, const char *devname, - struct wdp_port **portp) + struct wdp_port *port) { - int error = wdp->wdp_class->port_query_by_name(wdp, devname, portp); + int error = wdp->wdp_class->port_query_by_name(wdp, devname, port); if (!error) { VLOG_DBG_RL(&wdpmsg_rl, "%s: device %s is on port %"PRIu16, - wdp_name(wdp), devname, (*portp)->opp.port_no); + wdp_name(wdp), devname, port->opp.port_no); } else { - *portp = NULL; + wdp_port_clear(port); /* Log level is DBG here because all the current callers are interested * in whether 'wdp' actually has a port 'devname', so that it's not @@ -631,27 +673,29 @@ wdp_port_query_by_name(const struct wdp *wdp, const char *devname, int wdp_port_get_name(struct wdp *wdp, uint16_t port_no, char **namep) { - struct wdp_port *port; + struct wdp_port port; int error; error = wdp_port_query_by_number(wdp, port_no, &port); - *namep = !error ? xstrdup(port->devname) : NULL; + *namep = port.devname; + port.devname = NULL; + wdp_port_free(&port); + return error; } /* Obtains a list of all the ports in 'wdp', in no particular order. * - * If successful, returns 0 and sets '*portsp' to point to an array of - * pointers to port structures and '*n_portsp' to the number of pointers in the - * array. On failure, returns a positive errno value and sets '*portsp' to - * NULL and '*n_portsp' to 0. + * If successful, returns 0 and sets '*portsp' to point to an array of struct + * wdp_port and '*n_portsp' to the number of pointers in the array. On + * failure, returns a positive errno value and sets '*portsp' to NULL and + * '*n_portsp' to 0. * - * The caller is responsible for freeing '*portsp' by calling free(). The - * caller must not free the individual wdp_port structures. Calling - * wdp_run() or wdp_port_poll() may free the returned wdp_ports. */ + * The caller is responsible for freeing '*portsp' and the individual wdp_port + * structures, e.g. with wdp_port_array_free(). */ int wdp_port_list(const struct wdp *wdp, - struct wdp_port ***portsp, size_t *n_portsp) + struct wdp_port **portsp, size_t *n_portsp) { int error; diff --git a/ofproto/wdp.h b/ofproto/wdp.h index 3b25047cf..ee1efea00 100644 --- a/ofproto/wdp.h +++ b/ofproto/wdp.h @@ -103,21 +103,24 @@ int wdp_set_drop_frags(struct wdp *, bool drop_frags); struct wdp_port { struct netdev *netdev; - struct ofp_phy_port opp; /* In host byte order. */ + struct ofp_phy_port opp; /* In *host* byte order. */ char *devname; /* Network device name. */ bool internal; }; +void wdp_port_clear(struct wdp_port *); +void wdp_port_copy(struct wdp_port *, const struct wdp_port *); +void wdp_port_free(struct wdp_port *); +void wdp_port_array_free(struct wdp_port *, size_t n); int wdp_port_add(struct wdp *, const char *devname, bool internal, uint16_t *port_no); int wdp_port_del(struct wdp *, uint16_t port_no); int wdp_port_query_by_number(const struct wdp *, uint16_t port_no, - struct wdp_port **); + struct wdp_port *); int wdp_port_query_by_name(const struct wdp *, const char *devname, - struct wdp_port **); + struct wdp_port *); int wdp_port_get_name(struct wdp *, uint16_t port_no, char **namep); -int wdp_port_list(const struct wdp *, - struct wdp_port ***, size_t *n_ports); +int wdp_port_list(const struct wdp *, struct wdp_port **, size_t *n_ports); int wdp_port_set_config(struct wdp *, uint16_t port_no, uint32_t config); -- 2.47.0