On Wed, Oct 17, 2012 at 1:29 PM, Ben Pfaff <[email protected]> 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 <[email protected]> > --- > 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 > [email protected] > http://openvswitch.org/mailman/listinfo/dev >
_______________________________________________ dev mailing list [email protected] http://openvswitch.org/mailman/listinfo/dev
