[ovs-dev] [PATCH 1/2] ofp-util: Separate output, error reporting in ofputil_port_from_string().

Gurucharan Shetty gshetty at nicira.com
Thu Oct 18 16:18:18 UTC 2012


On Wed, Oct 17, 2012 at 1:29 PM, Ben Pfaff <blp at nicira.com> wrote:

> When I wrote this function I didn't think that port 0 was important (it's
> not a valid OpenFlow port number) so I used a return value of 0 to indicate
> an error.  However, my assumption turns out to be wrong, so this commit
> changes the interface to use the return value only for error reporting
> and store the parsed port number into a pointer passed in as a parameter.
>
> This commit doesn't change the behavior of ofputil_port_from_string().
>
> Signed-off-by: Ben Pfaff <blp at nicira.com>
> ---
>  lib/autopath.c        |    5 +++--
>  lib/bundle.c          |    3 +--
>  lib/meta-flow.c       |    3 +--
>  lib/ofp-parse.c       |   10 ++++------
>  lib/ofp-util.c        |   41 ++++++++++++++++++++++++-----------------
>  lib/ofp-util.h        |    2 +-
>  utilities/ovs-ofctl.c |    5 +++--
>  7 files changed, 37 insertions(+), 32 deletions(-)
>
> diff --git a/lib/autopath.c b/lib/autopath.c
> index b204e84..9da6463 100644
> --- a/lib/autopath.c
> +++ b/lib/autopath.c
> @@ -37,6 +37,7 @@ autopath_parse(struct ofpact_autopath *ap, const char
> *s_)
>  {
>      char *s;
>      char *id_str, *dst, *save_ptr;
> +    uint16_t port;
>
>      ofpact_init_AUTOPATH(ap);
>
> @@ -49,10 +50,10 @@ autopath_parse(struct ofpact_autopath *ap, const char
> *s_)
>          ovs_fatal(0, "%s: not enough arguments to autopath action", s_);
>      }
>
> -    ap->port = ofputil_port_from_string(id_str);
> -    if (!ap->port) {
> +    if (!ofputil_port_from_string(id_str, &port)) {
>          ovs_fatal(0, "%s: bad port number", s_);
>      }
> +    ap->port = port;
>
>      mf_parse_subfield(&ap->dst, dst);
>      if (ap->dst.n_bits < 16) {
> diff --git a/lib/bundle.c b/lib/bundle.c
> index b68ebab..92ac1e1 100644
> --- a/lib/bundle.c
> +++ b/lib/bundle.c
> @@ -272,8 +272,7 @@ bundle_parse__(const char *s, char **save_ptr,
>              break;
>          }
>
> -        slave_port = ofputil_port_from_string(slave);
> -        if (!slave_port) {
> +        if (!ofputil_port_from_string(slave, &slave_port)) {
>              ovs_fatal(0, "%s: bad port number", slave);
>          }
>          ofpbuf_put(ofpacts, &slave_port, sizeof slave_port);
> diff --git a/lib/meta-flow.c b/lib/meta-flow.c
> index 591eb34..4fa05ae 100644
> --- a/lib/meta-flow.c
> +++ b/lib/meta-flow.c
> @@ -1941,8 +1941,7 @@ mf_from_ofp_port_string(const struct mf_field *mf,
> const char *s,
>      uint16_t port;
>
>      assert(mf->n_bytes == sizeof(ovs_be16));
> -    port = ofputil_port_from_string(s);
> -    if (port) {
> +    if (ofputil_port_from_string(s, &port)) {
>          *valuep = htons(port);
>          *maskp = htons(UINT16_MAX);
>          return NULL;
> diff --git a/lib/ofp-parse.c b/lib/ofp-parse.c
> index 8941e17..c048a46 100644
> --- a/lib/ofp-parse.c
> +++ b/lib/ofp-parse.c
> @@ -168,8 +168,7 @@ parse_resubmit(char *arg, struct ofpbuf *ofpacts)
>
>      in_port_s = strsep(&arg, ",");
>      if (in_port_s && in_port_s[0]) {
> -        resubmit->in_port = ofputil_port_from_string(in_port_s);
> -        if (!resubmit->in_port) {
> +        if (!ofputil_port_from_string(in_port_s, &resubmit->in_port)) {
>              ovs_fatal(0, "%s: resubmit to unknown port", in_port_s);
>          }
>      } else {
> @@ -537,8 +536,8 @@ str_to_ofpact__(const struct flow *flow, char *pos,
> char *act, char *arg,
>          }
>          return false;
>      } else {
> -        uint16_t port = ofputil_port_from_string(act);
> -        if (port) {
> +        uint16_t port;
> +        if (ofputil_port_from_string(act, &port)) {
>              ofpact_put_OUTPUT(ofpacts)->port = port;
>          } else {
>              ovs_fatal(0, "Unknown action: %s", act);
> @@ -813,8 +812,7 @@ parse_ofp_str(struct ofputil_flow_mod *fm, int
> command, const char *str_,
>              if (!strcmp(name, "table")) {
>                  fm->table_id = str_to_table_id(value);
>              } else if (!strcmp(name, "out_port")) {
> -                fm->out_port = ofputil_port_from_string(name);
> -                if (!fm->out_port) {
> +                if (!ofputil_port_from_string(name, &fm->out_port)) {
>                      ofp_fatal(str_, verbose, "%s is not a valid OpenFlow
> port",
>                                name);
>                  }
> diff --git a/lib/ofp-util.c b/lib/ofp-util.c
> index 9527d2c..419a1cd 100644
> --- a/lib/ofp-util.c
> +++ b/lib/ofp-util.c
> @@ -3542,34 +3542,38 @@ ofputil_check_output_port(uint16_t port, int
> max_ports)
>          OFPUTIL_NAMED_PORT(LOCAL)               \
>          OFPUTIL_NAMED_PORT(NONE)
>
> -/* Returns the port number represented by 's', which may be an integer
> or, for
> - * reserved ports, the standard OpenFlow name for the port (e.g. "LOCAL").
> +/* Stores the port number represented by 's' into '*portp'.  's' may be an
> + * integer or, for reserved ports, the standard OpenFlow name for the port
> + * (e.g. "LOCAL").
>   *
> - * Returns 0 if 's' is not a valid OpenFlow port number or name.  The
> caller
> - * should issue an error message in this case, because this function
> usually
> - * does not.  (This gives the caller an opportunity to look up the port
> name
> - * another way, e.g. by contacting the switch and listing the names of
> all its
> - * ports).
> + * Returns true if successful, false if 's' is not a valid OpenFlow port
> number
> + * or name.  The caller should issue an error message in this case,
> because
> + * this function usually does not.  (This gives the caller an opportunity
> to
> + * look up the port name another way, e.g. by contacting the switch and
> listing
> + * the names of all its ports).
>   *
>   * This function accepts OpenFlow 1.0 port numbers.  It also accepts a
> subset
>   * of OpenFlow 1.1+ port numbers, mapping those port numbers into the
> 16-bit
>   * range as described in include/openflow/openflow-1.1.h. */
> -uint16_t
> -ofputil_port_from_string(const char *s)
> +bool
> +ofputil_port_from_string(const char *s, uint16_t *portp)
>  {
>      unsigned int port32;
>
> +    *portp = 0;
>      if (str_to_uint(s, 10, &port32)) {
>          if (port32 == 0) {
>              VLOG_WARN("port 0 is not a valid OpenFlow port number");
> -            return 0;
> +            return false;
>          } else if (port32 < OFPP_MAX) {
> -            return port32;
> +            *portp = port32;
> +            return true;
>          } else if (port32 < OFPP_FIRST_RESV) {
>              VLOG_WARN("port %u is a reserved OF1.0 port number that will "
>                        "be translated to %u when talking to an OF1.1 or "
>                        "later controller", port32, port32 + OFPP11_OFFSET);
> -            return port32;
> +            *portp = port32;
> +            return true;
>          } else if (port32 <= OFPP_LAST_RESV) {
>              struct ds s;
>
> @@ -3580,14 +3584,16 @@ ofputil_port_from_string(const char *s)
>                             ds_cstr(&s), port32);
>              ds_destroy(&s);
>
> -            return port32;
> +            *portp = port32;
> +            return true;
>          } else if (port32 < OFPP11_MAX) {
>              VLOG_WARN("port %u is outside the supported range 0 through "
>                        "%"PRIx16"or 0x%x through 0x%"PRIx32, port32,
>                        UINT16_MAX, (unsigned int) OFPP11_MAX, UINT32_MAX);
> -            return 0;
> +            return false;
>          } else {
> -            return port32 - OFPP11_OFFSET;
> +            *portp = port32 - OFPP11_OFFSET;
> +            return true;
>          }
>      } else {
>          struct pair {
> @@ -3603,10 +3609,11 @@ ofputil_port_from_string(const char *s)
>
>          for (p = pairs; p < &pairs[ARRAY_SIZE(pairs)]; p++) {
>              if (!strcasecmp(s, p->name)) {
> -                return p->value;
> +                *portp = p->value;
> +                return true;
>              }
>          }
> -        return 0;
> +        return false;
>      }
>  }
>
> diff --git a/lib/ofp-util.h b/lib/ofp-util.h
> index 2f73ec2..ede54cc 100644
> --- a/lib/ofp-util.h
> +++ b/lib/ofp-util.h
> @@ -36,7 +36,7 @@ enum ofperr ofputil_port_from_ofp11(ovs_be32 ofp11_port,
> uint16_t *ofp10_port);
>  ovs_be32 ofputil_port_to_ofp11(uint16_t ofp10_port);
>
>  enum ofperr ofputil_check_output_port(uint16_t ofp_port, int max_ports);
> -uint16_t ofputil_port_from_string(const char *);
> +bool ofputil_port_from_string(const char *, uint16_t *portp);
>  void ofputil_format_port(uint16_t port, struct ds *);
>
>  /* Converting OFPFW10_NW_SRC_MASK and OFPFW10_NW_DST_MASK wildcard bit
> counts
> diff --git a/utilities/ovs-ofctl.c b/utilities/ovs-ofctl.c
> index a0b7079..a67a554 100644
> --- a/utilities/ovs-ofctl.c
> +++ b/utilities/ovs-ofctl.c
> @@ -741,8 +741,9 @@ fetch_ofputil_phy_port(const char *vconn_name, const
> char *port_name,
>  static uint16_t
>  str_to_port_no(const char *vconn_name, const char *port_name)
>  {
> -    uint16_t port_no = ofputil_port_from_string(port_name);
> -    if (port_no) {
> +    uint16_t port_no;
> +
> +    if (ofputil_port_from_string(port_name, &port_no)) {
>          return port_no;
>      } else {
>          struct ofputil_phy_port pp;
>

Looks good to me.




> --
> 1.7.2.5
>
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openvswitch.org/pipermail/ovs-dev/attachments/20121018/49026c1a/attachment-0003.html>


More information about the dev mailing list