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

Alex Wang alexw at nicira.com
Thu Jun 20 19:35:57 UTC 2013


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 <blp at 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 <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.
>


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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openvswitch.org/pipermail/ovs-dev/attachments/20130620/02a0ca26/attachment-0003.html>


More information about the dev mailing list