wdp: Change functions that use wdp_port to avoid awkward memory semantics.
authorBen Pfaff <blp@nicira.com>
Thu, 8 Apr 2010 19:31:14 +0000 (12:31 -0700)
committerBen Pfaff <blp@nicira.com>
Thu, 8 Apr 2010 19:31:47 +0000 (12:31 -0700)
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
ofproto/wdp-provider.h
ofproto/wdp-xflow.c
ofproto/wdp.c
ofproto/wdp.h

index 7629dce..3442461 100644 (file)
@@ -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;
index 8c52295..b12ace9 100644 (file)
@@ -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,
index d0975a7..1c5fb92 100644 (file)
@@ -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);
 
index 52f6027..31f1292 100644 (file)
@@ -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;
 
index 3b25047..ee1efea 100644 (file)
@@ -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);