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 <blp@nicira.com>

diff --git a/lib/autopath.c b/lib/autopath.c
index b204e847307230ae72a19eb747ced40ad741963d..9da646339a19587ca6a7f928bf3b1628cef195ed 100644 (file)
--- 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 b68ebab8313fbea59159c4318ce117b668923974..92ac1e1d4e77ad141faabf629b011e5f861f8e97 100644 (file)
--- 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 591eb34447672c6e8671254f272d3e0553fc147b..4fa05ae74452051530a4185c447063ce9d714550 100644 (file)
--- 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 ea0703a3819303df26ca2b3ccfb581f7b1971c22..04f5ab174b68f9e151bc57d7bd011c2e42cec4a5 100644 (file)
--- 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 9527d2cb8a855f90fe7bdcf44ddc441e469770ab..419a1cd471a1eb5e8a078082c4d22dadc0d6976a 100644 (file)
--- 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 2f73ec2bf6f91a3479fe97858aaaec59a1cd6b8e..ede54cca282a095dba8618cc86fb699d54dd1f06 100644 (file)
--- 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 a0b70793b93413a247491aa3ffde5c1de308d75a..a67a5547dfab6e90cdd7b9d20e480fc687e5cad3 100644 (file)
--- 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;