[ovs-dev] [PATCH V3] feature: Create specific types for ofp and odp port

Ben Pfaff blp at nicira.com
Thu Jun 20 17:53:09 UTC 2013


On Wed, Jun 19, 2013 at 04:58:44PM -0700, Alex Wang wrote:
> Currently datapath ports and openflow ports are both represented by unsigned
> integers of various sizes. With implicit casts, etc. it is easy to mix them
> up and use one where the other is expected. This commit creates two typedefs
> ofp_port_t and odp_port_t. Both of these two types are marked by
> "__attribute__((bitwise))" so that Sparse can be used to detect any misuse.
> 
> Signed-off-by: Alex Wang <alexw at nicira.com>

I folded in some incrementals, below, and pushed this to master.

The incrementals:

        * Better protect the *_PORT_C macros.  I'd meant them only for
          integer literals (like UINT32_C, etc.) so I didn't
          originally put enough parentheses in, but I can see that it
          is tempting to use them in other places.

        * Replace one straggling UINT32_MAX by DOPP_NONE in a comment.

        * Fix dpif_netdev_port_add(), which checked choose_port()'s
          return value for >= 0, but an odp_port_t is always >= 0.

        * Fix some places where UINT16_MAX was used for a mask but it
          had been replaced by OFPP_MAX.  It's important for a mask
          that it be all-1-bits, so UINT16_MAX is appropriate there.

        * Move code around and adjust style in a couple of places to
          suit me.

Thank you very much!

diff --git a/include/openvswitch/types.h b/include/openvswitch/types.h
index 71cf468..d07c1e8 100644
--- a/include/openvswitch/types.h
+++ b/include/openvswitch/types.h
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2010, 2011 Nicira, Inc.
+ * Copyright (c) 2010, 2011, 2013 Nicira, Inc.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -68,8 +68,8 @@ typedef uint32_t OVS_BITWISE odp_port_t;
 typedef uint32_t OVS_BITWISE ofp11_port_t;
 
 /* Macro functions that cast int types to ofp/odp/ofp11 types. */
-#define OFP_PORT_C(X) ((OVS_FORCE ofp_port_t) X)
-#define ODP_PORT_C(X) ((OVS_FORCE odp_port_t) X)
-#define OFP11_PORT_C(X) ((OVS_FORCE ofp11_port_t) X)
+#define OFP_PORT_C(X) ((OVS_FORCE ofp_port_t) (X))
+#define ODP_PORT_C(X) ((OVS_FORCE odp_port_t) (X))
+#define OFP11_PORT_C(X) ((OVS_FORCE ofp11_port_t) (X))
 
 #endif /* openvswitch/types.h */
diff --git a/lib/dpif-linux.c b/lib/dpif-linux.c
index bc5697d..fb87f81 100644
--- a/lib/dpif-linux.c
+++ b/lib/dpif-linux.c
@@ -653,7 +653,7 @@ dpif_linux_port_get_pid(const struct dpif *dpif_, odp_port_t port_no)
     if (dpif->epoll_fd < 0) {
         return 0;
     } else {
-        /* The UINT32_MAX "reserved" port number uses the "ovs-system"'s
+        /* The ODPP_NONE "reserved" port number uses the "ovs-system"'s
          * channel, since it is not heavily loaded. */
         uint32_t idx = port_idx >= dpif->uc_array_size ? 0 : port_idx;
         return nl_sock_pid(dpif->channels[idx].sock);
diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
index 1ad454b..8e5e6df 100644
--- a/lib/dpif-netdev.c
+++ b/lib/dpif-netdev.c
@@ -219,8 +219,8 @@ create_dpif_netdev(struct dp_netdev *dp)
 }
 
 /* Choose an unused, non-zero port number and return it on success.
- * Return 0 on failure. */
-static uint32_t
+ * Return ODPP_NONE on failure. */
+static odp_port_t
 choose_port(struct dp_netdev *dp, const char *name)
 {
     uint32_t port_no;
@@ -243,7 +243,7 @@ choose_port(struct dp_netdev *dp, const char *name)
                 port_no = start_no + strtol(p, NULL, 10);
                 if (port_no > 0 && port_no < MAX_PORTS
                     && !dp->ports[port_no]) {
-                    return port_no;
+                    return u32_to_odp(port_no);
                 }
                 break;
             }
@@ -252,11 +252,11 @@ choose_port(struct dp_netdev *dp, const char *name)
 
     for (port_no = 1; port_no < MAX_PORTS; port_no++) {
         if (!dp->ports[port_no]) {
-            return port_no;
+            return u32_to_odp(port_no);
         }
     }
 
-    return 0;
+    return ODPP_NONE;
 }
 
 static int
@@ -444,21 +444,23 @@ dpif_netdev_port_add(struct dpif *dpif, struct netdev *netdev,
     struct dp_netdev *dp = get_dp_netdev(dpif);
     char namebuf[NETDEV_VPORT_NAME_BUFSIZE];
     const char *dpif_port;
-    uint32_t port_no = odp_to_u32(*port_nop);
+    odp_port_t port_no;
 
     dpif_port = netdev_vport_get_dpif_port(netdev, namebuf, sizeof namebuf);
-    if (port_no != UINT32_MAX) {
-        if (port_no >= MAX_PORTS) {
+    if (*port_nop != ODPP_NONE) {
+        uint32_t port_idx = odp_to_u32(*port_nop);
+        if (port_idx >= MAX_PORTS) {
             return EFBIG;
-        } else if (dp->ports[port_no]) {
+        } else if (dp->ports[port_idx]) {
             return EBUSY;
         }
+        port_no = *port_nop;
     } else {
         port_no = choose_port(dp, dpif_port);
     }
-    if (port_no > 0) {
-        *port_nop = u32_to_odp(port_no);
-        return do_add_port(dp, dpif_port, netdev_get_type(netdev), *port_nop);
+    if (port_no != ODPP_NONE) {
+        *port_nop = port_no;
+        return do_add_port(dp, dpif_port, netdev_get_type(netdev), port_no);
     }
     return EFBIG;
 }
@@ -711,7 +713,7 @@ dpif_netdev_flow_from_nlattrs(const struct nlattr *key, uint32_t key_len,
         return EINVAL;
     }
 
-    if (odp_to_u32(flow->in_port.odp_port) >= MAX_PORTS) {
+    if (!is_valid_port_number(flow->in_port.odp_port)) {
         return EINVAL;
     }
 
diff --git a/lib/flow.c b/lib/flow.c
index c12e2ec..32ac99a 100644
--- a/lib/flow.c
+++ b/lib/flow.c
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2008, 2009, 2010, 2011, 2012 Nicira, Inc.
+ * Copyright (c) 2008, 2009, 2010, 2011, 2012, 2013 Nicira, Inc.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -371,7 +371,9 @@ flow_extract(struct ofpbuf *packet, uint32_t skb_priority, uint32_t skb_mark,
         ovs_assert(tnl != &flow->tunnel);
         flow->tunnel = *tnl;
     }
-    flow->in_port = in_port ? *in_port : flow->in_port;
+    if (in_port) {
+        flow->in_port = *in_port;
+    }
     flow->skb_priority = skb_priority;
     flow->skb_mark = skb_mark;
 
diff --git a/lib/flow.h b/lib/flow.h
index a5790a6..a020937 100644
--- a/lib/flow.h
+++ b/lib/flow.h
@@ -68,9 +68,12 @@ struct flow_tnl {
     uint8_t ip_ttl;
 };
 
-union flow_in_port {           /* Input port union. The port can be either */
-    ofp_port_t   ofp_port;     /* OpenFlow port number or the datapath port */
-    odp_port_t   odp_port;     /* number. */
+/* Unfortunately, a "struct flow" sometimes has to handle OpenFlow port
+ * numbers and other times datapath (dpif) port numbers.  This union allows
+ * access to both. */
+union flow_in_port {
+    ofp_port_t ofp_port;
+    odp_port_t odp_port;
 };
 
 /*
@@ -87,12 +90,12 @@ struct flow {
     struct in6_addr ipv6_src;   /* IPv6 source address. */
     struct in6_addr ipv6_dst;   /* IPv6 destination address. */
     struct in6_addr nd_target;  /* IPv6 neighbor discovery (ND) target. */
-    union flow_in_port in_port; /* Input port union.*/
     uint32_t skb_priority;      /* Packet priority for QoS. */
     uint32_t regs[FLOW_N_REGS]; /* Registers. */
     ovs_be32 nw_src;            /* IPv4 source address. */
     ovs_be32 nw_dst;            /* IPv4 destination address. */
     ovs_be32 ipv6_label;        /* IPv6 flow label. */
+    union flow_in_port in_port; /* Input port.*/
     uint32_t skb_mark;          /* Packet mark. */
     ovs_be32 mpls_lse;          /* MPLS label stack entry. */
     uint16_t mpls_depth;        /* Depth of MPLS stack. */
@@ -174,14 +177,6 @@ flow_hash(const struct flow *flow, uint32_t basis)
     return hash_words((const uint32_t *) flow, sizeof *flow / 4, basis);
 }
 
-/* ofp/odp/ofp11 port cast functions. */
-static inline uint16_t ofp_to_u16(const ofp_port_t ofp_port);
-static inline uint32_t odp_to_u32(const odp_port_t odp_port);
-static inline uint32_t ofp11_to_u32(const ofp11_port_t ofp11_port);
-static inline ofp_port_t u16_to_ofp(const uint16_t port);
-static inline odp_port_t u32_to_odp(const uint32_t port);
-static inline ofp11_port_t u32_to_ofp11(const uint32_t port);
-
 static inline uint16_t
 ofp_to_u16(ofp_port_t ofp_port)
 {
diff --git a/lib/learning-switch.c b/lib/learning-switch.c
index c430f31..e786913 100644
--- a/lib/learning-switch.c
+++ b/lib/learning-switch.c
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2008, 2009, 2010, 2011, 2012 Nicira, Inc.
+ * Copyright (c) 2008, 2009, 2010, 2011, 2012, 2013 Nicira, Inc.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -48,7 +48,7 @@ VLOG_DEFINE_THIS_MODULE(learning_switch);
 
 struct lswitch_port {
     struct hmap_node hmap_node; /* Hash node for port number. */
-    ofp_port_t port_no;         /* OpenFlow port number, in host byte order. */
+    ofp_port_t port_no;         /* OpenFlow port number. */
     uint32_t queue_id;          /* OpenFlow queue number. */
 };
 
diff --git a/lib/match.c b/lib/match.c
index 4ee8ed6..6d66eba 100644
--- a/lib/match.c
+++ b/lib/match.c
@@ -273,7 +273,7 @@ match_set_tun_flags_masked(struct match *match, uint16_t flags, uint16_t mask)
 void
 match_set_in_port(struct match *match, ofp_port_t ofp_port)
 {
-    match->wc.masks.in_port.ofp_port = OFPP_NONE;
+    match->wc.masks.in_port.ofp_port = u16_to_ofp(UINT16_MAX);
     match->flow.in_port.ofp_port = ofp_port;
 }
 
diff --git a/lib/netlink.c b/lib/netlink.c
index 8353f83..7e7884e 100644
--- a/lib/netlink.c
+++ b/lib/netlink.c
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2008, 2009, 2010, 2011, 2012 Nicira, Inc.
+ * Copyright (c) 2008, 2009, 2010, 2011, 2012, 2013 Nicira, Inc.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -21,6 +21,7 @@
 #include <sys/types.h>
 #include <unistd.h>
 #include "coverage.h"
+#include "flow.h"
 #include "netlink-protocol.h"
 #include "ofpbuf.h"
 #include "timeval.h"
@@ -300,8 +301,7 @@ nl_msg_put_be64(struct ofpbuf *msg, uint16_t type, ovs_be64 value)
 void
 nl_msg_put_odp_port(struct ofpbuf *msg, uint16_t type, odp_port_t value)
 {
-    uint32_t value_ = (OVS_FORCE uint32_t) value;
-    nl_msg_put_unspec(msg, type, &value_, sizeof value);
+    nl_msg_put_u32(msg, type, odp_to_u32(value));
 }
 
 
@@ -586,7 +586,7 @@ nl_attr_get_be64(const struct nlattr *nla)
 odp_port_t
 nl_attr_get_odp_port(const struct nlattr *nla)
 {
-    return ODP_PORT_C(NL_ATTR_GET_AS(nla, uint32_t));
+    return u32_to_odp(nl_attr_get_u32(nla));
 }
 
 /* Returns the null-terminated string value in 'nla''s payload.
diff --git a/lib/odp-util.c b/lib/odp-util.c
index 400a7e7..d70cf17 100644
--- a/lib/odp-util.c
+++ b/lib/odp-util.c
@@ -1334,7 +1334,7 @@ odp_flow_format(const struct nlattr *key, size_t key_len,
         }
         if (left) {
             int i;
-
+            
             if (left == key_len) {
                 ds_put_cstr(ds, "<empty>");
             }
diff --git a/lib/odp-util.h b/lib/odp-util.h
index 1b602b4..bcde078 100644
--- a/lib/odp-util.h
+++ b/lib/odp-util.h
@@ -35,7 +35,7 @@ struct ofpbuf;
 struct simap;
 
 #define ODPP_LOCAL ODP_PORT_C(OVSP_LOCAL)
-#define ODPP_NONE  ODP_PORT_C(UINT_MAX)
+#define ODPP_NONE  ODP_PORT_C(UINT32_MAX)
 
 void format_odp_actions(struct ds *, const struct nlattr *odp_actions,
                         size_t actions_len);
diff --git a/lib/ofp-util.c b/lib/ofp-util.c
index 8d0a8da..90f4f35 100644
--- a/lib/ofp-util.c
+++ b/lib/ofp-util.c
@@ -90,7 +90,7 @@ ofputil_wildcard_from_ofpfw10(uint32_t ofpfw, struct flow_wildcards *wc)
     flow_wildcards_init_catchall(wc);
 
     if (!(ofpfw & OFPFW10_IN_PORT)) {
-        wc->masks.in_port.ofp_port = OFPP_NONE;
+        wc->masks.in_port.ofp_port = u16_to_ofp(UINT16_MAX);
     }
 
     if (!(ofpfw & OFPFW10_NW_TOS)) {
@@ -4861,9 +4861,8 @@ ofputil_encode_queue_stats_request(enum ofp_version ofp_version,
         request = ofpraw_alloc(OFPRAW_OFPST10_QUEUE_REQUEST, ofp_version, 0);
         req = ofpbuf_put_zeros(request, sizeof *req);
         /* OpenFlow 1.0 needs OFPP_ALL instead of OFPP_ANY */
-        req->port_no = htons(oqsr->port_no == OFPP_ANY
-                             ? ofp_to_u16(OFPP_ALL)
-                               : ofp_to_u16(oqsr->port_no));
+        req->port_no = htons(ofp_to_u16(oqsr->port_no == OFPP_ANY
+                                        ? OFPP_ALL : oqsr->port_no));
         req->queue_id = htonl(oqsr->queue_id);
         break;
     }
diff --git a/lib/util.c b/lib/util.c
index 776caab..a118412 100644
--- a/lib/util.c
+++ b/lib/util.c
@@ -480,17 +480,6 @@ str_to_ullong(const char *s, int base, unsigned long long *ull)
     return str_to_llong(s, base, (long long *) ull);
 }
 
-bool
-str_to_ofp(const char *s, ofp_port_t *ofp_port)
-{
-    bool ret;
-    uint32_t port_;
-
-    ret = str_to_uint(s, 10, &port_);
-    *ofp_port = OFP_PORT_C(port_);
-    return ret;
-}
-
 /* Converts floating-point string 's' into a double.  If successful, stores
  * the double in '*d' and returns true; on failure, stores 0 in '*d' and
  * returns false.
diff --git a/lib/util.h b/lib/util.h
index 4a06a75..c436a43 100644
--- a/lib/util.h
+++ b/lib/util.h
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2008, 2009, 2010, 2011, 2012 Nicira, Inc.
+ * Copyright (c) 2008, 2009, 2010, 2011, 2012, 2013 Nicira, Inc.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -225,8 +225,6 @@ bool str_to_ullong(const char *, int base, unsigned long long *);
 
 bool str_to_double(const char *, double *);
 
-bool str_to_ofp(const char *, ofp_port_t *);
-
 int hexit_value(int c);
 unsigned int hexits_value(const char *s, size_t n, bool *ok);
 
diff --git a/ofproto/netflow.h b/ofproto/netflow.h
index 14ac454..7e6debc 100644
--- a/ofproto/netflow.h
+++ b/ofproto/netflow.h
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2008, 2009, 2010, 2011 Nicira, Inc.
+ * Copyright (c) 2008, 2009, 2010, 2011, 2013 Nicira, Inc.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -38,9 +38,9 @@ struct netflow_options {
     bool add_id_to_iface;
 };
 
-#define NF_OUT_FLOOD OFPP_NONE
-#define NF_OUT_MULTI (u16_to_ofp(ofp_to_u16(OFPP_NONE) - 1))
-#define NF_OUT_DROP  (u16_to_ofp(ofp_to_u16(OFPP_NONE) - 2))
+#define NF_OUT_FLOOD OFP_PORT_C(UINT16_MAX)
+#define NF_OUT_MULTI OFP_PORT_C(UINT16_MAX - 1)
+#define NF_OUT_DROP  OFP_PORT_C(UINT16_MAX - 2)
 
 struct netflow_flow {
     long long int last_expired;   /* Time this flow last timed out. */
diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index 93e5a6d..6fa7894 100644
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -595,8 +595,7 @@ type_run(const char *type)
                     }
                 }
 
-                iter->odp_port = node ? u32_to_odp(node->data)
-                                        : ODPP_NONE;
+                iter->odp_port = node ? u32_to_odp(node->data) : ODPP_NONE;
                 if (tnl_port_reconfigure(&iter->up, iter->odp_port,
                                          &iter->tnl_port)) {
                     backer->need_revalidate = REV_RECONFIGURE;
diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
index df2148e..afd8e17 100644
--- a/ofproto/ofproto.c
+++ b/ofproto/ofproto.c
@@ -1683,26 +1683,29 @@ reinit_ports(struct ofproto *p)
 static ofp_port_t
 alloc_ofp_port(struct ofproto *ofproto, const char *netdev_name)
 {
-    ofp_port_t ofp_port;
-    uint16_t end_port_no = ofp_to_u16(ofproto->alloc_port_no);
+    uint16_t max_ports = ofp_to_u16(ofproto->max_ports);
+    uint16_t port_idx;
 
-    ofp_port = u16_to_ofp(simap_get(&ofproto->ofp_requests, netdev_name));
-    ofp_port = ofp_port ? ofp_port : OFPP_NONE;
+    port_idx = simap_get(&ofproto->ofp_requests, netdev_name);
+    if (!port_idx) {
+        port_idx = UINT16_MAX;
+    }
 
-    if (ofp_to_u16(ofp_port) >= ofp_to_u16(ofproto->max_ports)
-        || bitmap_is_set(ofproto->ofp_port_ids, ofp_to_u16(ofp_port))) {
-        uint16_t alloc_port_no = ofp_to_u16(ofproto->alloc_port_no);
+    if (port_idx >= max_ports
+        || bitmap_is_set(ofproto->ofp_port_ids, port_idx)) {
+        uint16_t end_port_no = ofp_to_u16(ofproto->alloc_port_no);
+        uint16_t alloc_port_no = end_port_no;
 
         /* Search for a free OpenFlow port number.  We try not to
          * immediately reuse them to prevent problems due to old
          * flows. */
         for (;;) {
-            if (++alloc_port_no >= ofp_to_u16(ofproto->max_ports)) {
+            if (++alloc_port_no >= max_ports) {
                 alloc_port_no = 0;
             }
             if (!bitmap_is_set(ofproto->ofp_port_ids, alloc_port_no)) {
-                ofp_port = u16_to_ofp(alloc_port_no);
-                ofproto->alloc_port_no = ofp_port;
+                port_idx = alloc_port_no;
+                ofproto->alloc_port_no = u16_to_ofp(alloc_port_no);
                 break;
             }
             if (alloc_port_no == end_port_no) {
@@ -1710,8 +1713,8 @@ alloc_ofp_port(struct ofproto *ofproto, const char *netdev_name)
             }
         }
     }
-    bitmap_set1(ofproto->ofp_port_ids, ofp_to_u16(ofp_port));
-    return ofp_port;
+    bitmap_set1(ofproto->ofp_port_ids, port_idx);
+    return u16_to_ofp(port_idx);
 }
 
 static void
diff --git a/utilities/ovs-ofctl.c b/utilities/ovs-ofctl.c
index 75bedb1..28e3863 100644
--- a/utilities/ovs-ofctl.c
+++ b/utilities/ovs-ofctl.c
@@ -746,6 +746,16 @@ fetch_port_by_stats(const char *vconn_name,
     return found;
 }
 
+static bool
+str_to_ofp(const char *s, ofp_port_t *ofp_port)
+{
+    bool ret;
+    uint32_t port_;
+
+    ret = str_to_uint(s, 10, &port_);
+    *ofp_port = u16_to_ofp(port_);
+    return ret;
+}
 
 /* Opens a connection to 'vconn_name', fetches the port structure for
  * 'port_name' (which may be a port name or number), and copies it into



More information about the dev mailing list