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 <al...@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
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
