Thanks very much, Ben,
And sorry, I should have put you as co-author in the last commit.
On Thu, Jun 20, 2013 at 10:53 AM, Ben Pfaff <b...@nicira.com> wrote:
> 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.
>
I really want to learn the difference here. Could you please explain more
why the additional parentheses matter?
> * Replace one straggling UINT32_MAX by DOPP_NONE in a comment.
>
Thanks, for this fix.
> * Fix dpif_netdev_port_add(), which checked choose_port()'s
> return value for >= 0, but an odp_port_t is always >= 0.
>
Thanks for this fix, the code looks more consistent.
> * 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.
>
Actually, I think you mean OFPP_NONE here, right? Is it to be more clear
that we use "u16_to_ofp(UINT16_MAX)" for "masks.in_port.ofp_port" assignment
?
> * 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);
> -
>
Thanks for removing these. Surely is not useful.
> 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));
> }
>
Thanks for this fix.
> @@ -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));
>
This is cleaner!
> 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);
> }
>
>
With fewer ofp_to_u16() calls, this looks cleaner. Thanks,
> 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
