From 8010100bb17598479d1b3bb06452546075038f6f Mon Sep 17 00:00:00 2001 From: Ben Pfaff Date: Wed, 17 Oct 2012 13:10:08 -0700 Subject: [PATCH] ofp-util: Separate output, error reporting in ofputil_port_from_string(). 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 --- 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 b204e847..9da64633 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 b68ebab8..92ac1e1d 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 591eb344..4fa05ae7 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 ea0703a3..04f5ab17 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 { @@ -547,8 +546,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); @@ -823,8 +822,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 9527d2cb..419a1cd4 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 2f73ec2b..ede54cca 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 a0b70793..a67a5547 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; -- 2.30.2