[ovs-dev] [PATCH V1] bridge.c: Change variable "ofport" type in "struct if_cfg" and "struct iface"

Ben Pfaff blp at nicira.com
Tue Jun 4 17:18:15 UTC 2013


Sorry, I was out until today.

On Fri, May 31, 2013 at 03:48:51PM -0700, Alex Wang wrote:
> This patch will help my project ""create specific types for ofp and odp
> ports"", in which all OpenFlow-1.0 port is defined as type uint16_t.
> 
> Hope to hear more comments.
> 
> Thanks,
> 
> 
> 
> On Fri, May 31, 2013 at 4:04 PM, Alex Wang <alexw at nicira.com> wrote:
> 
> > This patch changes the variable type of "ofport" in "struct if_cfg" and
> > "struct iface" from int64_t to uint16_t. This is more consistent with
> > the OpenFlow-1.0 port definition.
> >
> > Also, before this patch, -1 is used to indicate an unknown port. This
> > patch uses OFPP_NONE, since "ofport" becomes uint16_t.
> >
> > Signed-off-by: Alex Wang <alexw at nicira.com>
> > ---
> >  vswitchd/bridge.c |   53
> > ++++++++++++++++++++++++++++-------------------------
> >  1 file changed, 28 insertions(+), 25 deletions(-)
> >
> > diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c
> > index 3bdb5db..8990949 100644
> > --- a/vswitchd/bridge.c
> > +++ b/vswitchd/bridge.c
> > @@ -67,7 +67,7 @@ struct if_cfg {
> >      struct hmap_node hmap_node;         /* Node in bridge's if_cfg_todo.
> > */
> >      const struct ovsrec_interface *cfg; /* Interface record. */
> >      const struct ovsrec_port *parent;   /* Parent port record. */
> > -    int64_t ofport;                     /* Requested OpenFlow port
> > number. */
> > +    uint16_t ofport;                    /* Requested OpenFlow port
> > number. */
> >  };
> >
> >  /* OpenFlow port slated for removal from ofproto. */
> > @@ -86,7 +86,8 @@ struct iface {
> >      /* These members are valid only after bridge_reconfigure() causes
> > them to
> >       * be initialized. */
> >      struct hmap_node ofp_port_node; /* In struct bridge's "ifaces" hmap.
> > */
> > -    int ofp_port;               /* OpenFlow port number, -1 if unknown. */
> > +    uint16_t ofp_port;          /* OpenFlow port number, */
> > +                                /* OFPP_NONE if unknown. */
> >      struct netdev *netdev;      /* Network device. */
> >      const char *type;           /* Usually same as cfg->type. */
> >      const struct ovsrec_interface *cfg;
> > @@ -245,7 +246,7 @@ static bool mirror_configure(struct mirror *);
> >  static void mirror_refresh_stats(struct mirror *);
> >
> >  static void iface_configure_lacp(struct iface *, struct
> > lacp_slave_settings *);
> > -static bool iface_create(struct bridge *, struct if_cfg *, int ofp_port);
> > +static bool iface_create(struct bridge *, struct if_cfg *, uint16_t
> > ofp_port);
> >  static bool iface_is_internal(const struct ovsrec_interface *iface,
> >                                const struct ovsrec_bridge *br);
> >  static const char *iface_get_type(const struct ovsrec_interface *,
> > @@ -257,7 +258,7 @@ static struct if_cfg *if_cfg_lookup(const struct
> > bridge *, const char *name);
> >  static struct iface *iface_from_ofp_port(const struct bridge *,
> >                                           uint16_t ofp_port);
> >  static void iface_set_mac(struct iface *);
> > -static void iface_set_ofport(const struct ovsrec_interface *, int64_t
> > ofport);
> > +static void iface_set_ofport(const struct ovsrec_interface *, uint16_t
> > ofport);
> >  static void iface_clear_db_record(const struct ovsrec_interface *if_cfg);
> >  static void iface_configure_qos(struct iface *, const struct ovsrec_qos
> > *);
> >  static void iface_configure_cfm(struct iface *);
> > @@ -265,7 +266,7 @@ static void iface_refresh_cfm_stats(struct iface *);
> >  static void iface_refresh_stats(struct iface *);
> >  static void iface_refresh_status(struct iface *);
> >  static bool iface_is_synthetic(const struct iface *);
> > -static int64_t iface_pick_ofport(const struct ovsrec_interface *);
> > +static uint16_t iface_pick_ofport(const struct ovsrec_interface *);
> >
> >  /* Linux VLAN device support (e.g. "eth0.10" for VLAN 10.)
> >   *
> > @@ -565,7 +566,7 @@ bridge_reconfigure_ofp(void)
> >          struct if_cfg *if_cfg, *next;
> >
> >          HMAP_FOR_EACH_SAFE (if_cfg, next, hmap_node, &br->if_cfg_todo) {
> > -            iface_create(br, if_cfg, -1);
> > +            iface_create(br, if_cfg, OFPP_NONE);
> >              time_refresh();
> >              if (time_msec() >= deadline) {
> >                  return false;
> > @@ -1267,11 +1268,11 @@ add_del_bridges(const struct ovsrec_open_vswitch
> > *cfg)
> >  }
> >
> >  static void
> > -iface_set_ofp_port(struct iface *iface, int ofp_port)
> > +iface_set_ofp_port(struct iface *iface, uint16_t ofp_port)
> >  {
> >      struct bridge *br = iface->port->bridge;
> >
> > -    ovs_assert(iface->ofp_port < 0 && ofp_port >= 0);
> > +    ovs_assert(iface->ofp_port == OFPP_NONE && ofp_port != OFPP_NONE);
> >      iface->ofp_port = ofp_port;
> >      hmap_insert(&br->ifaces, &iface->ofp_port_node, hash_int(ofp_port,
> > 0));
> >      iface_set_ofport(iface->cfg, ofp_port);
> > @@ -1313,7 +1314,7 @@ bridge_refresh_one_ofp_port(struct bridge *br,
> >      struct iface *iface = iface_lookup(br, name);
> >      if (iface) {
> >          /* Check that the name-to-number mapping is one-to-one. */
> > -        if (iface->ofp_port >= 0) {
> > +        if (iface->ofp_port != OFPP_NONE) {
> >              VLOG_WARN("bridge %s: interface %s reported twice",
> >                        br->name, name);
> >              return false;
> > @@ -1367,7 +1368,7 @@ bridge_refresh_ofp_port(struct bridge *br)
> >          struct iface *iface;
> >
> >          LIST_FOR_EACH (iface, port_elem, &port->ifaces) {
> > -            iface->ofp_port = -1;
> > +            iface->ofp_port = OFPP_NONE;
> >          }
> >      }
> >
> > @@ -1389,7 +1390,7 @@ bridge_refresh_ofp_port(struct bridge *br)
> >          struct iface *iface, *iface_next;
> >
> >          LIST_FOR_EACH_SAFE (iface, iface_next, port_elem, &port->ifaces) {
> > -            if (iface->ofp_port < 0) {
> > +            if (iface->ofp_port == OFPP_NONE) {
> >                  bridge_queue_if_cfg(br, iface->cfg, port->cfg);
> >                  iface_destroy(iface);
> >              }
> > @@ -1402,7 +1403,7 @@ bridge_refresh_ofp_port(struct bridge *br)
> >  }
> >
> >  /* Opens a network device for 'if_cfg' and configures it.  If '*ofp_portp'
> > - * is negative, adds the network device to br->ofproto and stores the
> > OpenFlow
> > + * is OFPP_NONE, adds the network device to br->ofproto and stores the
> > OpenFlow
> >   * port number in '*ofp_portp'; otherwise leaves br->ofproto and
> > '*ofp_portp'
> >   * untouched.
> >   *
> > @@ -1411,7 +1412,7 @@ bridge_refresh_ofp_port(struct bridge *br)
> >  static int
> >  iface_do_create(const struct bridge *br,
> >                  const struct if_cfg *if_cfg,
> > -                int *ofp_portp, struct netdev **netdevp)
> > +                uint16_t *ofp_portp, struct netdev **netdevp)
> >  {
> >      const struct ovsrec_interface *iface_cfg = if_cfg->cfg;
> >      const struct ovsrec_port *port_cfg = if_cfg->parent;
> > @@ -1438,7 +1439,7 @@ iface_do_create(const struct bridge *br,
> >          goto error;
> >      }
> >
> > -    if (*ofp_portp < 0) {
> > +    if (*ofp_portp == OFPP_NONE) {
> >          uint16_t ofp_port = if_cfg->ofport;
> >
> >          error = ofproto_port_add(br->ofproto, netdev, &ofp_port);
> > @@ -1469,13 +1470,13 @@ error:
> >  }
> >
> >  /* Creates a new iface on 'br' based on 'if_cfg'.  The new iface has
> > OpenFlow
> > - * port number 'ofp_port'.  If ofp_port is negative, an OpenFlow port is
> > + * port number 'ofp_port'.  If ofp_port is OFPP_NONE, an OpenFlow port is
> >   * automatically allocated for the iface.  Takes ownership of and
> >   * deallocates 'if_cfg'.
> >   *
> >   * Return true if an iface is successfully created, false otherwise. */
> >  static bool
> > -iface_create(struct bridge *br, struct if_cfg *if_cfg, int ofp_port)
> > +iface_create(struct bridge *br, struct if_cfg *if_cfg, uint16_t ofp_port)
> >  {
> >      const struct ovsrec_interface *iface_cfg = if_cfg->cfg;
> >      const struct ovsrec_port *port_cfg = if_cfg->parent;
> > @@ -1496,7 +1497,7 @@ iface_create(struct bridge *br, struct if_cfg
> > *if_cfg, int ofp_port)
> >      error = iface_do_create(br, if_cfg, &ofp_port, &netdev);
> >      bridge_run_fast();
> >      if (error) {
> > -        iface_set_ofport(iface_cfg, -1);
> > +        iface_set_ofport(iface_cfg, OFPP_NONE);
> >          iface_clear_db_record(iface_cfg);
> >          ok = false;
> >          goto done;
> > @@ -1515,7 +1516,7 @@ iface_create(struct bridge *br, struct if_cfg
> > *if_cfg, int ofp_port)
> >                  hash_string(iface_cfg->name, 0));
> >      iface->port = port;
> >      iface->name = xstrdup(iface_cfg->name);
> > -    iface->ofp_port = -1;
> > +    iface->ofp_port = OFPP_NONE;
> >      iface->netdev = netdev;
> >      iface->type = iface_get_type(iface_cfg, br->cfg);
> >      iface->cfg = iface_cfg;
> > @@ -3422,11 +3423,11 @@ iface_destroy(struct iface *iface)
> >          struct port *port = iface->port;
> >          struct bridge *br = port->bridge;
> >
> > -        if (br->ofproto && iface->ofp_port >= 0) {
> > +        if (br->ofproto && iface->ofp_port != OFPP_NONE) {
> >              ofproto_port_unregister(br->ofproto, iface->ofp_port);
> >          }
> >
> > -        if (iface->ofp_port >= 0) {
> > +        if (iface->ofp_port != OFPP_NONE) {
> >              hmap_remove(&br->ifaces, &iface->ofp_port_node);
> >          }
> >
> > @@ -3527,10 +3528,12 @@ iface_set_mac(struct iface *iface)
> >
> >  /* Sets the ofport column of 'if_cfg' to 'ofport'. */
> >  static void
> > -iface_set_ofport(const struct ovsrec_interface *if_cfg, int64_t ofport)
> > +iface_set_ofport(const struct ovsrec_interface *if_cfg, uint16_t ofport)
> >  {
> > +    int64_t port_;
> > +    port_ = (ofport == OFPP_NONE) ? -1 : ofport;
> >      if (if_cfg && !ovsdb_idl_row_is_synthetic(&if_cfg->header_)) {
> > -        ovsrec_interface_set_ofport(if_cfg, &ofport, 1);
> > +        ovsrec_interface_set_ofport(if_cfg, &port_, 1);
> >      }
> >  }
> >
> > @@ -3714,11 +3717,11 @@ iface_is_synthetic(const struct iface *iface)
> >      return ovsdb_idl_row_is_synthetic(&iface->cfg->header_);
> >  }
> >
> > -static int64_t
> > +static uint16_t
> >  iface_pick_ofport(const struct ovsrec_interface *cfg)
> >  {
> > -    int64_t ofport = cfg->n_ofport ? *cfg->ofport : OFPP_NONE;
> > -    return cfg->n_ofport_request ? *cfg->ofport_request : ofport;
> > +    uint16_t ofport = cfg->n_ofport ? (uint16_t) *cfg->ofport : OFPP_NONE;
> > +    return cfg->n_ofport_request ? (uint16_t) *cfg->ofport_request :
> > ofport;
> >  }
> >
> >
> > --
> > 1.7.9.5
> >
> >

> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev




More information about the dev mailing list