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

Ben Pfaff blp at nicira.com
Wed Oct 17 20:29:43 UTC 2012


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;
-- 
1.7.2.5




More information about the dev mailing list