[ovs-dev] [PATCH] ovs-ofctl: Better ovs-ofctl add-flows error reporting

Andy Zhou azhou at nicira.com
Thu Mar 21 22:00:29 UTC 2013


When error is encountered with add-flows command, this patch
adds file name and line number in addition to the parser
error message to the output.

The file name and line number will not be added to
the output of "ovs-ofctl add-flow", only parser error
message will appear. Same as before.

Signed-off-by: Andy Zhou <azhou at nicira.com>
---
 lib/bundle.c           |   38 ++--
 lib/bundle.h           |    5 +-
 lib/dynamic-string.c   |   49 +++--
 lib/dynamic-string.h   |    1 +
 lib/learn.c            |   31 +--
 lib/meta-flow.c        |   18 +-
 lib/meta-flow.h        |   12 +-
 lib/multipath.c        |   42 ++--
 lib/multipath.h        |    6 +-
 lib/nx-match.c         |   44 +++--
 lib/nx-match.h         |    9 +-
 lib/ofp-parse.c        |  510 ++++++++++++++++++++++++++++++------------------
 lib/ofp-parse.h        |   56 +++++-
 lib/ofp-util.c         |   19 ++
 lib/ofp-util.h         |    2 +
 tests/test-bundle.c    |    4 +-
 tests/test-multipath.c |    3 +-
 utilities/ovs-ofctl.c  |   27 ++-
 18 files changed, 579 insertions(+), 297 deletions(-)

diff --git a/lib/bundle.c b/lib/bundle.c
index 92ac1e1..337aa82 100644
--- a/lib/bundle.c
+++ b/lib/bundle.c
@@ -244,7 +244,7 @@ bundle_to_nxast(const struct ofpact_bundle *bundle, struct ofpbuf *openflow)
 }
 
 /* Helper for bundle_parse and bundle_parse_load. */
-static void
+static char *
 bundle_parse__(const char *s, char **save_ptr,
                const char *fields, const char *basis, const char *algorithm,
                const char *slave_type, const char *dst,
@@ -253,12 +253,13 @@ bundle_parse__(const char *s, char **save_ptr,
     struct ofpact_bundle *bundle;
 
     if (!slave_delim) {
-        ovs_fatal(0, "%s: not enough arguments to bundle action", s);
+        return ofputil_perr_str("%s: not enough arguments to bundle action",
+                                s);
     }
 
     if (strcasecmp(slave_delim, "slaves")) {
-        ovs_fatal(0, "%s: missing slave delimiter, expected `slaves' got `%s'",
-                   s, slave_delim);
+        return ofputil_perr_str("%s: missing slave delimiter,"
+                                " expected `slaves' got `%s'", s, slave_delim);
     }
 
     bundle = ofpact_put_BUNDLE(ofpacts);
@@ -273,7 +274,7 @@ bundle_parse__(const char *s, char **save_ptr,
         }
 
         if (!ofputil_port_from_string(slave, &slave_port)) {
-            ovs_fatal(0, "%s: bad port number", slave);
+            return ofputil_perr_str("%s: bad port number", slave);
         }
         ofpbuf_put(ofpacts, &slave_port, sizeof slave_port);
 
@@ -289,7 +290,7 @@ bundle_parse__(const char *s, char **save_ptr,
     } else if (!strcasecmp(fields, "symmetric_l4")) {
         bundle->fields = NX_HASH_FIELDS_SYMMETRIC_L4;
     } else {
-        ovs_fatal(0, "%s: unknown fields `%s'", s, fields);
+        return ofputil_perr_str("%s: unknown fields `%s'", s, fields);
     }
 
     if (!strcasecmp(algorithm, "active_backup")) {
@@ -297,21 +298,23 @@ bundle_parse__(const char *s, char **save_ptr,
     } else if (!strcasecmp(algorithm, "hrw")) {
         bundle->algorithm = NX_BD_ALG_HRW;
     } else {
-        ovs_fatal(0, "%s: unknown algorithm `%s'", s, algorithm);
+        return ofputil_perr_str("%s: unknown algorithm `%s'", s, algorithm);
     }
 
     if (strcasecmp(slave_type, "ofport")) {
-        ovs_fatal(0, "%s: unknown slave_type `%s'", s, slave_type);
+        return ofputil_perr_str("%s: unknown slave_type `%s'", s, slave_type);
     }
 
     if (dst) {
-        mf_parse_subfield(&bundle->dst, dst);
+        return mf_parse_subfield(&bundle->dst, &dst);
     }
+
+    return NULL;
 }
 
 /* Converts a bundle action string contained in 's' to an nx_action_bundle and
  * stores it in 'b'.  Sets 'b''s l2 pointer to NULL. */
-void
+char *
 bundle_parse(const char *s, struct ofpbuf *ofpacts)
 {
     char *fields, *basis, *algorithm, *slave_type, *slave_delim;
@@ -325,14 +328,17 @@ bundle_parse(const char *s, struct ofpbuf *ofpacts)
     slave_type = strtok_r(NULL, ", ", &save_ptr);
     slave_delim = strtok_r(NULL, ": ", &save_ptr);
 
-    bundle_parse__(s, &save_ptr, fields, basis, algorithm, slave_type, NULL,
-                   slave_delim, ofpacts);
+    IF_PERR_CRET(bundle_parse__(s, &save_ptr, fields, basis, algorithm,
+                                slave_type, NULL, slave_delim, ofpacts),
+                 free(tokstr));
+
     free(tokstr);
+    return NULL;
 }
 
 /* Converts a bundle_load action string contained in 's' to an nx_action_bundle
  * and stores it in 'b'.  Sets 'b''s l2 pointer to NULL. */
-void
+char *
 bundle_parse_load(const char *s, struct ofpbuf *ofpacts)
 {
     char *fields, *basis, *algorithm, *slave_type, *dst, *slave_delim;
@@ -347,10 +353,12 @@ bundle_parse_load(const char *s, struct ofpbuf *ofpacts)
     dst = strtok_r(NULL, ", ", &save_ptr);
     slave_delim = strtok_r(NULL, ": ", &save_ptr);
 
-    bundle_parse__(s, &save_ptr, fields, basis, algorithm, slave_type, dst,
-                   slave_delim, ofpacts);
+    IF_PERR_CRET(bundle_parse__(s, &save_ptr, fields, basis, algorithm,
+                                slave_type, dst, slave_delim, ofpacts),
+                 free(tokstr));
 
     free(tokstr);
+    return NULL;
 }
 
 /* Appends a human-readable representation of 'nab' to 's'. */
diff --git a/lib/bundle.h b/lib/bundle.h
index 5b6bb67..6ef8d15 100644
--- a/lib/bundle.h
+++ b/lib/bundle.h
@@ -24,6 +24,7 @@
 #include "ofp-errors.h"
 #include "openflow/nicira-ext.h"
 #include "openvswitch/types.h"
+#include "compiler.h"
 
 struct ds;
 struct flow;
@@ -42,8 +43,8 @@ enum ofperr bundle_from_openflow(const struct nx_action_bundle *,
 enum ofperr bundle_check(const struct ofpact_bundle *, int max_ports,
                          const struct flow *);
 void bundle_to_nxast(const struct ofpact_bundle *, struct ofpbuf *of10);
-void bundle_parse(const char *, struct ofpbuf *ofpacts);
-void bundle_parse_load(const char *, struct ofpbuf *ofpacts);
+char *bundle_parse(const char *, struct ofpbuf *) WARN_UNUSED_RESULT;
+char *bundle_parse_load(const char *, struct ofpbuf *) WARN_UNUSED_RESULT;
 void bundle_format(const struct ofpact_bundle *, struct ds *);
 
 #endif /* bundle.h */
diff --git a/lib/dynamic-string.c b/lib/dynamic-string.c
index bd1cf45..4c302a5 100644
--- a/lib/dynamic-string.c
+++ b/lib/dynamic-string.c
@@ -223,30 +223,45 @@ ds_get_line(struct ds *ds, FILE *file)
     }
 }
 
-/* Reads a line from 'file' into 'ds', clearing anything initially in 'ds'.
- * Deletes comments introduced by "#" and skips lines that contains only white
- * space (after deleting comments).
+/* Preprocess a line in 'ds'. Deletes comments introduced by "#".
+ *
+ * Returns 'true' if non-blank line was found, 'false' otherwise.
+ */
+bool
+ds_preprocess_line(struct ds *ds)
+{
+    char *line = ds_cstr(ds);
+    char *comment;
+
+    /* Delete comments. */
+    comment = strchr(line, '#');
+    if (comment) {
+        *comment = '\0';
+    }
+
+    /* Return false if line is empty. */
+    if (line[strspn(line, " \t\n")] == '\0') {
+        return false;
+    }
+
+    return true;
+}
+
+/* Reads a line from 'file' into 'ds'.  Deletes comments introduced
+ * by "#" and skips lines that contains only white space
+ * (after deleting comments).
  *
  * Returns 0 if successful, EOF if no non-blank line was found. */
 int
 ds_get_preprocessed_line(struct ds *ds, FILE *file)
 {
-    while (!ds_get_line(ds, file)) {
-        char *line = ds_cstr(ds);
-        char *comment;
-
-        /* Delete comments. */
-        comment = strchr(line, '#');
-        if (comment) {
-            *comment = '\0';
+    do {
+        if (ds_get_line(ds, file)) {
+            return EOF;
         }
+    } while (ds_preprocess_line(ds) == false);
 
-        /* Return successfully unless the line is all spaces. */
-        if (line[strspn(line, " \t\n")] != '\0') {
-            return 0;
-        }
-    }
-    return EOF;
+    return 0;
 }
 
 /* Reads a line from 'file' into 'ds' and does some preprocessing on it:
diff --git a/lib/dynamic-string.h b/lib/dynamic-string.h
index 098caaf..e30b436 100644
--- a/lib/dynamic-string.h
+++ b/lib/dynamic-string.h
@@ -61,6 +61,7 @@ void ds_put_strftime(struct ds *, const char *, bool utc)
 void ds_put_hex_dump(struct ds *ds, const void *buf_, size_t size,
                      uintptr_t ofs, bool ascii);
 int ds_get_line(struct ds *, FILE *);
+bool ds_preprocess_line(struct ds *);
 int ds_get_preprocessed_line(struct ds *, FILE *);
 int ds_get_test_line(struct ds *, FILE *);
 
diff --git a/lib/learn.c b/lib/learn.c
index b9bbc97..6439005 100644
--- a/lib/learn.c
+++ b/lib/learn.c
@@ -25,6 +25,7 @@
 #include "nx-match.h"
 #include "ofp-actions.h"
 #include "ofp-errors.h"
+#include "ofp-parse.h"
 #include "ofp-util.h"
 #include "ofpbuf.h"
 #include "openflow/openflow.h"
@@ -406,7 +407,7 @@ learn_parse_load_immediate(const char *s, struct ofpact_learn_spec *spec)
     }
     s += 2;
 
-    s = mf_parse_subfield(&dst, s);
+    IF_PERR_EXIT(mf_parse_subfield(&dst, &s));
     if (*s != '\0') {
         ovs_fatal(0, "%s: trailing garbage following destination", full_s);
     }
@@ -428,6 +429,8 @@ static void
 learn_parse_spec(const char *orig, char *name, char *value,
                  struct ofpact_learn_spec *spec)
 {
+    char const *s;
+
     if (mf_from_name(name)) {
         const struct mf_field *dst = mf_from_name(name);
         union mf_value imm;
@@ -449,17 +452,17 @@ learn_parse_spec(const char *orig, char *name, char *value,
         spec->dst.n_bits = dst->n_bits;
     } else if (strchr(name, '[')) {
         /* Parse destination and check prerequisites. */
-        if (mf_parse_subfield(&spec->dst, name)[0] != '\0') {
-            ovs_fatal(0, "%s: syntax error after NXM field name `%s'",
-                      orig, name);
-        }
+        s = name;
+        IF_PERR_EXIT_MSG(mf_parse_subfield(&spec->dst, &s),
+                         "%s: syntax error after NXM field name `%s', %s",
+                         orig, name);
 
         /* Parse source and check prerequisites. */
         if (value[0] != '\0') {
-            if (mf_parse_subfield(&spec->src, value)[0] != '\0') {
-                ovs_fatal(0, "%s: syntax error after NXM field name `%s'",
-                          orig, value);
-            }
+            s = value;
+            IF_PERR_EXIT_MSG(mf_parse_subfield(&spec->src, &s),
+                             "%s: syntax error after NXM field name `%s', %s",
+                             orig, value);
             if (spec->src.n_bits != spec->dst.n_bits) {
                 ovs_fatal(0, "%s: bit widths of %s (%u) and %s (%u) differ",
                           orig, name, spec->src.n_bits, value,
@@ -478,7 +481,7 @@ learn_parse_spec(const char *orig, char *name, char *value,
         } else {
             struct ofpact_reg_move move;
 
-            nxm_parse_reg_move(&move, value);
+            IF_PERR_EXIT(nxm_parse_reg_move(&move, value));
 
             spec->n_bits = move.src.n_bits;
             spec->src_type = NX_LEARN_SRC_FIELD;
@@ -487,10 +490,10 @@ learn_parse_spec(const char *orig, char *name, char *value,
             spec->dst = move.dst;
         }
     } else if (!strcmp(name, "output")) {
-        if (mf_parse_subfield(&spec->src, value)[0] != '\0') {
-            ovs_fatal(0, "%s: syntax error after NXM field name `%s'",
-                      orig, name);
-        }
+        s = value;
+        IF_PERR_EXIT_MSG(mf_parse_subfield(&spec->src, &s),
+                         "%s: syntax error after NXM field name `%s', %s",
+                         orig, name);
 
         spec->n_bits = spec->src.n_bits;
         spec->src_type = NX_LEARN_SRC_FIELD;
diff --git a/lib/meta-flow.c b/lib/meta-flow.c
index 6f7a3aa..81d9103 100644
--- a/lib/meta-flow.c
+++ b/lib/meta-flow.c
@@ -2604,24 +2604,20 @@ mf_parse_subfield__(struct mf_subfield *sf, const char **sp)
     return NULL;
 }
 
-/* Parses a subfield from the beginning of 's' into 'sf'.  Returns the first
- * byte in 's' following the parsed string.
+/* Parses a subfield from the beginning of '**s' into 'sf'.  Returns the first
+ * byte in '*s' following the parsed string.
  *
- * Exits with an error message if 's' has incorrect syntax.
+ * Return an error message if '*s' has incorrect syntax.
  *
- * The syntax parsed from 's' takes the form "header[start..end]" where
+ * The syntax parsed from '*s' takes the form "header[start..end]" where
  * 'header' is the name of an NXM field and 'start' and 'end' are (inclusive)
  * bit indexes.  "..end" may be omitted to indicate a single bit.  "start..end"
  * may both be omitted (the [] are still required) to indicate an entire
  * field.  */
-const char *
-mf_parse_subfield(struct mf_subfield *sf, const char *s)
+char *
+mf_parse_subfield(struct mf_subfield *sf, const char **s)
 {
-    char *msg = mf_parse_subfield__(sf, &s);
-    if (msg) {
-        ovs_fatal(0, "%s", msg);
-    }
-    return s;
+    return  mf_parse_subfield__(sf, s);
 }
 
 void
diff --git a/lib/meta-flow.h b/lib/meta-flow.h
index 57f6df5..d11b5c9 100644
--- a/lib/meta-flow.h
+++ b/lib/meta-flow.h
@@ -348,16 +348,20 @@ uint64_t mf_get_subfield(const struct mf_subfield *, const struct flow *);
 
 
 void mf_format_subfield(const struct mf_subfield *, struct ds *);
-char *mf_parse_subfield__(struct mf_subfield *sf, const char **s);
-const char *mf_parse_subfield(struct mf_subfield *, const char *);
+char *mf_parse_subfield__(struct mf_subfield *sf, const char **s)
+    WARN_UNUSED_RESULT;
+char *mf_parse_subfield(struct mf_subfield *, const char **)
+    WARN_UNUSED_RESULT;
 
 enum ofperr mf_check_src(const struct mf_subfield *, const struct flow *);
 enum ofperr mf_check_dst(const struct mf_subfield *, const struct flow *);
 
 /* Parsing and formatting. */
 char *mf_parse(const struct mf_field *, const char *,
-               union mf_value *value, union mf_value *mask);
-char *mf_parse_value(const struct mf_field *, const char *, union mf_value *);
+               union mf_value *value, union mf_value *mask)
+    WARN_UNUSED_RESULT;
+char *mf_parse_value(const struct mf_field *, const char *, union mf_value *)
+    WARN_UNUSED_RESULT;
 void mf_format(const struct mf_field *,
                const union mf_value *value, const union mf_value *mask,
                struct ds *);
diff --git a/lib/multipath.c b/lib/multipath.c
index f6a1a0a..aacb0c0 100644
--- a/lib/multipath.c
+++ b/lib/multipath.c
@@ -193,14 +193,20 @@ multipath_algorithm(uint32_t hash, enum nx_mp_algorithm algorithm,
  * 'mp' accordingly.  ovs-ofctl(8) describes the format parsed.
  *
  * Prints an error on stderr and aborts the program if 's_' syntax is
- * invalid. */
-void
+ * invalid.
+ *
+ * Return:  NULL success, error string otherwise.
+ */
+char *
 multipath_parse(struct ofpact_multipath *mp, const char *s_)
 {
     char *s = xstrdup(s_);
     char *save_ptr = NULL;
-    char *fields, *basis, *algorithm, *n_links_str, *arg, *dst;
+    char *fields, *basis, *algorithm, *n_links_str, *arg;
+    char const *dst;
+    char *err_str = NULL;
     int n_links;
+    #define CF free(s)
 
     fields = strtok_r(s, ", ", &save_ptr);
     basis = strtok_r(NULL, ", ", &save_ptr);
@@ -209,7 +215,9 @@ multipath_parse(struct ofpact_multipath *mp, const char *s_)
     arg = strtok_r(NULL, ", ", &save_ptr);
     dst = strtok_r(NULL, ", ", &save_ptr);
     if (!dst) {
-        ovs_fatal(0, "%s: not enough arguments to multipath action", s_);
+        err_str = ofputil_perr_str("%s: not enough arguments to"
+                                " multipath action", s_);
+        goto ret;
     }
 
     ofpact_init_MULTIPATH(mp);
@@ -218,7 +226,8 @@ multipath_parse(struct ofpact_multipath *mp, const char *s_)
     } else if (!strcasecmp(fields, "symmetric_l4")) {
         mp->fields = NX_HASH_FIELDS_SYMMETRIC_L4;
     } else {
-        ovs_fatal(0, "%s: unknown fields `%s'", s_, fields);
+        err_str =ofputil_perr_str("%s: unknown fields `%s'", s_, fields);
+        goto ret;
     }
     mp->basis = atoi(basis);
     if (!strcasecmp(algorithm, "modulo_n")) {
@@ -230,24 +239,31 @@ multipath_parse(struct ofpact_multipath *mp, const char *s_)
     } else if (!strcasecmp(algorithm, "iter_hash")) {
         mp->algorithm = NX_MP_ALG_ITER_HASH;
     } else {
-        ovs_fatal(0, "%s: unknown algorithm `%s'", s_, algorithm);
+        err_str = ofputil_perr_str("%s: unknown algorithm `%s'", s_, algorithm);
+        goto ret;
     }
     n_links = atoi(n_links_str);
     if (n_links < 1 || n_links > 65536) {
-        ovs_fatal(0, "%s: n_links %d is not in valid range 1 to 65536",
-                  s_, n_links);
+        err_str = ofputil_perr_str("%s: n_links %d is not in valid"
+                                " range 1 to 65536", s_, n_links);
+        goto ret;
     }
     mp->max_link = n_links - 1;
     mp->arg = atoi(arg);
 
-    mf_parse_subfield(&mp->dst, dst);
+    IF_PERR_CRET(mf_parse_subfield(&mp->dst, &dst), CF);
     if (mp->dst.n_bits < 16 && n_links > (1u << mp->dst.n_bits)) {
-        ovs_fatal(0, "%s: %d-bit destination field has %u possible values, "
-                  "less than specified n_links %d",
-                  s_, mp->dst.n_bits, 1u << mp->dst.n_bits, n_links);
+        err_str =ofputil_perr_str("%s: %d-bit destination field has"
+                                " %u possible values, "
+                                "less than specified n_links %d",
+                                 s_, mp->dst.n_bits,
+                                 1u << mp->dst.n_bits, n_links);
+        goto ret;
     }
 
-    free(s);
+ret:
+    CF;
+    return err_str;
 }
 
 /* Appends a description of 'mp' to 's', in the format that ovs-ofctl(8)
diff --git a/lib/multipath.h b/lib/multipath.h
index 1b5160d..172ea76 100644
--- a/lib/multipath.h
+++ b/lib/multipath.h
@@ -18,7 +18,9 @@
 #define MULTIPATH_H 1
 
 #include <stdint.h>
+#include "compiler.h"
 #include "ofp-errors.h"
+#include "ofp-parse.h"
 
 struct ds;
 struct flow;
@@ -40,7 +42,9 @@ void multipath_to_nxast(const struct ofpact_multipath *,
 
 void multipath_execute(const struct ofpact_multipath *, struct flow *);
 
-void multipath_parse(struct ofpact_multipath *, const char *);
+char *multipath_parse(struct ofpact_multipath *, const char *)
+    WARN_UNUSED_RESULT;
+
 void multipath_format(const struct ofpact_multipath *, struct ds *);
 
 #endif /* multipath.h */
diff --git a/lib/nx-match.c b/lib/nx-match.c
index bfead68..617f9d2 100644
--- a/lib/nx-match.c
+++ b/lib/nx-match.c
@@ -24,6 +24,7 @@
 #include "dynamic-string.h"
 #include "meta-flow.h"
 #include "ofp-actions.h"
+#include "ofp-parse.h"
 #include "ofp-errors.h"
 #include "ofp-util.h"
 #include "ofpbuf.h"
@@ -1006,50 +1007,57 @@ oxm_match_from_string(const char *s, struct ofpbuf *b)
     return match_len;
 }
 
-void
+char *
 nxm_parse_reg_move(struct ofpact_reg_move *move, const char *s)
 {
     const char *full_s = s;
 
-    s = mf_parse_subfield(&move->src, s);
+    IF_PERR_RET(mf_parse_subfield(&move->src, &s));
     if (strncmp(s, "->", 2)) {
-        ovs_fatal(0, "%s: missing `->' following source", full_s);
+        return ofputil_perr_str("%s: missing `->' following source", full_s);
     }
     s += 2;
-    s = mf_parse_subfield(&move->dst, s);
+    IF_PERR_RET(mf_parse_subfield(&move->dst, &s));
     if (*s != '\0') {
-        ovs_fatal(0, "%s: trailing garbage following destination", full_s);
+        return ofputil_perr_str("%s: trailing garbage following"
+                                " destination", full_s);
     }
 
     if (move->src.n_bits != move->dst.n_bits) {
-        ovs_fatal(0, "%s: source field is %d bits wide but destination is "
-                  "%d bits wide", full_s,
-                  move->src.n_bits, move->dst.n_bits);
+        return ofputil_perr_str("%s: source field is %d bits wide"
+                               " but destination is %d bits wide",
+                               full_s, move->src.n_bits, move->dst.n_bits);
     }
+
+    return NULL;
 }
 
-void
+char *
 nxm_parse_reg_load(struct ofpact_reg_load *load, const char *s)
 {
     const char *full_s = s;
     uint64_t value = strtoull(s, (char **) &s, 0);
 
     if (strncmp(s, "->", 2)) {
-        ovs_fatal(0, "%s: missing `->' following value", full_s);
+        return ofputil_perr_str("%s: missing `->' following value", full_s);
     }
     s += 2;
-    s = mf_parse_subfield(&load->dst, s);
+    IF_PERR_RET(mf_parse_subfield(&load->dst, &s));
     if (*s != '\0') {
-        ovs_fatal(0, "%s: trailing garbage following destination", full_s);
+        return ofputil_perr_str("%s: trailing garbage following"
+                                " destination", full_s);
     }
 
     if (load->dst.n_bits < 64 && (value >> load->dst.n_bits) != 0) {
-        ovs_fatal(0, "%s: value %"PRIu64" does not fit into %d bits",
-                  full_s, value, load->dst.n_bits);
+        return ofputil_perr_str("%s: value %"PRIu64" does not fit"
+                                " into %d bits",
+                                full_s, value, load->dst.n_bits);
     }
 
     load->subvalue.be64[0] = htonll(0);
     load->subvalue.be64[1] = htonll(value);
+
+    return NULL;
 }
 
 /* nxm_format_reg_move(), nxm_format_reg_load(). */
@@ -1314,13 +1322,15 @@ nxm_reg_load(const struct mf_subfield *dst, uint64_t src_data,
 }
 
 /* nxm_parse_stack_action, works for both push() and pop(). */
-void
+char *
 nxm_parse_stack_action(struct ofpact_stack *stack_action, const char *s)
 {
-    s = mf_parse_subfield(&stack_action->subfield, s);
+    IF_PERR_RET(mf_parse_subfield(&stack_action->subfield, &s));
     if (*s != '\0') {
-        ovs_fatal(0, "%s: trailing garbage following push or pop", s);
+        return ofputil_perr_str("%s: trailing garbage following"
+                                " push or pop", s);
     }
+    return NULL;
 }
 
 void
diff --git a/lib/nx-match.h b/lib/nx-match.h
index 7d316d8..3dbc926 100644
--- a/lib/nx-match.h
+++ b/lib/nx-match.h
@@ -57,8 +57,10 @@ char *oxm_match_to_string(const uint8_t *, unsigned int match_len);
 int nx_match_from_string(const char *, struct ofpbuf *);
 int oxm_match_from_string(const char *, struct ofpbuf *);
 
-void nxm_parse_reg_move(struct ofpact_reg_move *, const char *);
-void nxm_parse_reg_load(struct ofpact_reg_load *, const char *);
+char *nxm_parse_reg_move(struct ofpact_reg_move *, const char *)
+    WARN_UNUSED_RESULT;
+char *nxm_parse_reg_load(struct ofpact_reg_load *, const char *)
+    WARN_UNUSED_RESULT;
 
 void nxm_format_reg_move(const struct ofpact_reg_move *, struct ds *);
 void nxm_format_reg_load(const struct ofpact_reg_load *, struct ds *);
@@ -85,7 +87,8 @@ void nxm_execute_reg_load(const struct ofpact_reg_load *, struct flow *);
 void nxm_reg_load(const struct mf_subfield *, uint64_t src_data,
                   struct flow *);
 
-void nxm_parse_stack_action(struct ofpact_stack *, const char *);
+char *nxm_parse_stack_action(struct ofpact_stack *, const char *)
+    WARN_UNUSED_RESULT;
 
 void nxm_format_stack_push(const struct ofpact_stack *, struct ds *);
 void nxm_format_stack_pop(const struct ofpact_stack *, struct ds *);
diff --git a/lib/ofp-parse.c b/lib/ofp-parse.c
index e8abc9f..3219d9a 100644
--- a/lib/ofp-parse.c
+++ b/lib/ofp-parse.c
@@ -41,88 +41,113 @@
 
 VLOG_DEFINE_THIS_MODULE(ofp_parse);
 
-static void ofp_fatal(const char *flow, bool verbose, const char *format, ...)
-    NO_RETURN;
+static char *ofp_err_str(const char *flow, bool verbose,
+                         const char *format, ...) WARN_UNUSED_RESULT;
 
-static uint8_t
-str_to_table_id(const char *str)
+WARN_UNUSED_RESULT static char *
+str_to_u8(const char *str, uint8_t *vp)
+{
+    int u;
+
+    if (!str_to_int(str, 10, &u) || u < 0 || u > 255) {
+        return (ofputil_perr_str("invalid value \"%s\"", str));
+    }
+
+    *vp = (uint8_t)u;
+    return NULL;
+}
+
+WARN_UNUSED_RESULT static char *
+str_to_table_id(const char *str, uint8_t *table_id_p)
 {
     int table_id;
 
     if (!str_to_int(str, 10, &table_id) || table_id < 0 || table_id > 255) {
-        ovs_fatal(0, "invalid table \"%s\"", str);
+        return (ofputil_perr_str("invalid table \"%s\"", str));
     }
-    return table_id;
+
+    *table_id_p = table_id;
+    return NULL;
 }
 
-static uint16_t
-str_to_u16(const char *str, const char *name)
+WARN_UNUSED_RESULT static char *
+str_to_u16(const char *str, const char *name, uint16_t *vp )
 {
     int value;
 
     if (!str_to_int(str, 0, &value) || value < 0 || value > 65535) {
-        ovs_fatal(0, "invalid %s \"%s\"", name, str);
+        return (ofputil_perr_str("invalid %s \"%s\"", name, str));
     }
-    return value;
+
+    *vp = value;
+    return NULL;
 }
 
-static uint32_t
-str_to_u32(const char *str)
+WARN_UNUSED_RESULT static char *
+str_to_u32(const char *str, uint32_t *vp)
 {
     char *tail;
     uint32_t value;
 
     if (!str[0]) {
-        ovs_fatal(0, "missing required numeric argument");
+        return (ofputil_perr_str("missing required numeric argument"));
     }
 
     errno = 0;
     value = strtoul(str, &tail, 0);
     if (errno == EINVAL || errno == ERANGE || *tail) {
-        ovs_fatal(0, "invalid numeric format %s", str);
+        return (ofputil_perr_str("invalid numeric format %s", str));
     }
-    return value;
+
+    *vp = value;
+    return NULL;
 }
 
-static uint64_t
-str_to_u64(const char *str)
+WARN_UNUSED_RESULT static char *
+str_to_u64(const char *str, uint64_t *vp)
 {
     char *tail;
     uint64_t value;
 
     if (!str[0]) {
-        ovs_fatal(0, "missing required numeric argument");
+        return (ofputil_perr_str("missing required numeric argument"));
     }
 
     errno = 0;
     value = strtoull(str, &tail, 0);
     if (errno == EINVAL || errno == ERANGE || *tail) {
-        ovs_fatal(0, "invalid numeric format %s", str);
+        return (ofputil_perr_str("invalid numeric format %s", str));
     }
-    return value;
+
+    *vp = value;
+    return NULL;
 }
 
-static void
+WARN_UNUSED_RESULT static char *
 str_to_mac(const char *str, uint8_t mac[6])
 {
     if (sscanf(str, ETH_ADDR_SCAN_FMT, ETH_ADDR_SCAN_ARGS(mac))
         != ETH_ADDR_SCAN_COUNT) {
-        ovs_fatal(0, "invalid mac address %s", str);
+        return (ofputil_perr_str("invalid mac address %s", str));
     }
+
+    return NULL;
 }
 
-static void
+WARN_UNUSED_RESULT static char *
 str_to_ip(const char *str, ovs_be32 *ip)
 {
     struct in_addr in_addr;
 
     if (lookup_ip(str, &in_addr)) {
-        ovs_fatal(0, "%s: could not convert to IP address", str);
+        return (ofputil_perr_str("%s: could not convert to IP address", str));
     }
     *ip = in_addr.s_addr;
+
+    return NULL;
 }
 
-static void
+WARN_UNUSED_RESULT static char *
 parse_enqueue(char *arg, struct ofpbuf *ofpacts)
 {
     char *sp = NULL;
@@ -131,33 +156,38 @@ parse_enqueue(char *arg, struct ofpbuf *ofpacts)
     struct ofpact_enqueue *enqueue;
 
     if (port == NULL || queue == NULL) {
-        ovs_fatal(0, "\"enqueue\" syntax is \"enqueue:PORT:QUEUE\"");
+        return ofputil_perr_str("\"enqueue\" syntax is \"enqueue:"
+                                      "PORT:QUEUE\"");
     }
 
     enqueue = ofpact_put_ENQUEUE(ofpacts);
-    enqueue->port = str_to_u32(port);
-    enqueue->queue = str_to_u32(queue);
+    IF_PERR_RET(str_to_u16(port, "port", &enqueue->port));
+    IF_PERR_RET(str_to_u32(queue, &enqueue->queue));
+
+    return NULL;
 }
 
-static void
+WARN_UNUSED_RESULT static char *
 parse_output(char *arg, struct ofpbuf *ofpacts)
 {
     if (strchr(arg, '[')) {
         struct ofpact_output_reg *output_reg;
 
         output_reg = ofpact_put_OUTPUT_REG(ofpacts);
-        mf_parse_subfield(&output_reg->src, arg);
         output_reg->max_len = UINT16_MAX;
+        IF_PERR_RET(mf_parse_subfield(&output_reg->src, (char const **)&arg));
     } else {
         struct ofpact_output *output;
 
         output = ofpact_put_OUTPUT(ofpacts);
-        output->port = str_to_u32(arg);
         output->max_len = output->port == OFPP_CONTROLLER ? UINT16_MAX : 0;
+        IF_PERR_RET(str_to_u16(arg, "port",  &output->port));
     }
+
+    return NULL;
 }
 
-static void
+WARN_UNUSED_RESULT static char *
 parse_resubmit(char *arg, struct ofpbuf *ofpacts)
 {
     struct ofpact_resubmit *resubmit;
@@ -168,22 +198,29 @@ parse_resubmit(char *arg, struct ofpbuf *ofpacts)
     in_port_s = strsep(&arg, ",");
     if (in_port_s && in_port_s[0]) {
         if (!ofputil_port_from_string(in_port_s, &resubmit->in_port)) {
-            ovs_fatal(0, "%s: resubmit to unknown port", in_port_s);
+            return ofputil_perr_str("%s: resubmit to unknown port", in_port_s);
         }
     } else {
         resubmit->in_port = OFPP_IN_PORT;
     }
 
     table_s = strsep(&arg, ",");
-    resubmit->table_id = table_s && table_s[0] ? str_to_u32(table_s) : 255;
+
+    if (table_s && table_s[0]){
+        IF_PERR_RET(str_to_table_id(table_s, &resubmit->table_id));
+    }else {
+        resubmit -> table_id = 255;
+    }
 
     if (resubmit->in_port == OFPP_IN_PORT && resubmit->table_id == 255) {
-        ovs_fatal(0, "at least one \"in_port\" or \"table\" must be specified "
-                  " on resubmit");
+        return ofputil_perr_str("at least one \"in_port\" or \"table\""
+                                " must be specified on resubmit");
     }
+
+    return NULL;
 }
 
-static void
+WARN_UNUSED_RESULT static char *
 parse_note(const char *arg, struct ofpbuf *ofpacts)
 {
     struct ofpact_note *note;
@@ -202,7 +239,7 @@ parse_note(const char *arg, struct ofpbuf *ofpacts)
 
         byte = hexits_value(arg, 2, &ok);
         if (!ok) {
-            ovs_fatal(0, "bad hex digit in `note' argument");
+            return ofputil_perr_str("bad hex digit in `note' argument");
         }
         ofpbuf_put(ofpacts, &byte, 1);
 
@@ -212,9 +249,11 @@ parse_note(const char *arg, struct ofpbuf *ofpacts)
         arg += 2;
     }
     ofpact_update_len(ofpacts, &note->ofpact);
+
+    return NULL;
 }
 
-static void
+WARN_UNUSED_RESULT static char *
 parse_fin_timeout(struct ofpbuf *b, char *arg)
 {
     struct ofpact_fin_timeout *oft = ofpact_put_FIN_TIMEOUT(b);
@@ -222,16 +261,18 @@ parse_fin_timeout(struct ofpbuf *b, char *arg)
 
     while (ofputil_parse_key_value(&arg, &key, &value)) {
         if (!strcmp(key, "idle_timeout")) {
-            oft->fin_idle_timeout = str_to_u16(value, key);
+            IF_PERR_RET(str_to_u16(value, key, &oft->fin_idle_timeout));
         } else if (!strcmp(key, "hard_timeout")) {
-            oft->fin_hard_timeout = str_to_u16(value, key);
+            IF_PERR_RET(str_to_u16(value, key, &oft->fin_hard_timeout));
         } else {
-            ovs_fatal(0, "invalid key '%s' in 'fin_timeout' argument", key);
+            return ofputil_perr_str("invalid key '%s' in 'fin_timeout'"
+                                    " argument", key);
         }
     }
+    return NULL;
 }
 
-static void
+WARN_UNUSED_RESULT static char *
 parse_controller(struct ofpbuf *b, char *arg)
 {
     enum ofp_packet_in_reason reason = OFPR_ACTION;
@@ -241,22 +282,22 @@ parse_controller(struct ofpbuf *b, char *arg)
     if (!arg[0]) {
         /* Use defaults. */
     } else if (strspn(arg, "0123456789") == strlen(arg)) {
-        max_len = str_to_u16(arg, "max_len");
+        IF_PERR_RET(str_to_u16(arg, "max_len", &max_len));
     } else {
         char *name, *value;
 
         while (ofputil_parse_key_value(&arg, &name, &value)) {
             if (!strcmp(name, "reason")) {
                 if (!ofputil_packet_in_reason_from_string(value, &reason)) {
-                    ovs_fatal(0, "unknown reason \"%s\"", value);
+                    return ofputil_perr_str("unknown reason \"%s\"", value);
                 }
             } else if (!strcmp(name, "max_len")) {
-                max_len = str_to_u16(value, "max_len");
+                IF_PERR_RET(str_to_u16(value, "max_len", &max_len));
             } else if (!strcmp(name, "id")) {
-                controller_id = str_to_u16(value, "id");
+                IF_PERR_RET(str_to_u16(value, "id", &controller_id));
             } else {
-                ovs_fatal(0, "unknown key \"%s\" parsing controller action",
-                          name);
+                return ofputil_perr_str("unknown key \"%s\""
+                                        " parsing controller action", name);
             }
         }
     }
@@ -275,9 +316,11 @@ parse_controller(struct ofpbuf *b, char *arg)
         controller->reason = reason;
         controller->controller_id = controller_id;
     }
+
+    return NULL;
 }
 
-static void
+WARN_UNUSED_RESULT static char *
 parse_noargs_dec_ttl(struct ofpbuf *b)
 {
     struct ofpact_cnt_ids *ids;
@@ -288,13 +331,15 @@ parse_noargs_dec_ttl(struct ofpbuf *b)
     ids = b->l2;
     ids->n_controllers++;
     ofpact_update_len(b, &ids->ofpact);
+
+    return NULL;
 }
 
-static void
+WARN_UNUSED_RESULT static char *
 parse_dec_ttl(struct ofpbuf *b, char *arg)
 {
     if (*arg == '\0') {
-        parse_noargs_dec_ttl(b);
+        return parse_noargs_dec_ttl(b);
     } else {
         struct ofpact_cnt_ids *ids;
         char *cntr;
@@ -310,26 +355,29 @@ parse_dec_ttl(struct ofpbuf *b, char *arg)
             ids->n_controllers++;
         }
         if (!ids->n_controllers) {
-            ovs_fatal(0, "dec_ttl_cnt_ids: expected at least one controller "
-                      "id.");
+            return ofputil_perr_str("dec_ttl_cnt_ids: expected"
+                                    " at least one controller id.");
         }
         ofpact_update_len(b, &ids->ofpact);
     }
+
+    return NULL;
 }
 
-static void
+WARN_UNUSED_RESULT static char *
 parse_set_mpls_ttl(struct ofpbuf *b, const char *arg)
 {
     struct ofpact_mpls_ttl *mpls_ttl = ofpact_put_SET_MPLS_TTL(b);
 
     if (*arg == '\0') {
-        ovs_fatal(0, "parse_set_mpls_ttl: expected ttl.");
+        return ofputil_perr_str("parse_set_mpls_ttl: expected ttl.");
     }
 
     mpls_ttl->ttl = atoi(arg);
+    return NULL;
 }
 
-static void
+WARN_UNUSED_RESULT static char *
 set_field_parse(const char *arg, struct ofpbuf *ofpacts)
 {
     char *orig = xstrdup(arg);
@@ -338,58 +386,67 @@ set_field_parse(const char *arg, struct ofpbuf *ofpacts)
     char *delim;
     char *key;
     const struct mf_field *mf;
-    const char *error;
     union mf_value mf_value;
 
     value = orig;
     delim = strstr(orig, "->");
     if (!delim) {
-        ovs_fatal(0, "%s: missing `->'", orig);
+        free(orig);
+        return ofputil_perr_str("%s: missing `->'", orig);
     }
     if (strlen(delim) <= strlen("->")) {
-        ovs_fatal(0, "%s: missing field name following `->'", orig);
+        free(orig);
+        return ofputil_perr_str("%s: missing field name following `->'", orig);
     }
 
     key = delim + strlen("->");
     mf = mf_from_name(key);
     if (!mf) {
-        ovs_fatal(0, "%s is not valid oxm field name", key);
+        free(orig);
+        return ofputil_perr_str("%s is not valid oxm field name", key);
     }
     if (!mf->writable) {
-        ovs_fatal(0, "%s is not allowed to set", key);
+        free(orig);
+        return ofputil_perr_str("%s is not allowed to set", key);
     }
 
     delim[0] = '\0';
-    error = mf_parse_value(mf, value, &mf_value);
-    if (error) {
-        ovs_fatal(0, "%s", error);
-    }
+    IF_PERR_CRET(mf_parse_value(mf, value, &mf_value), free(orig));
     if (!mf_is_value_valid(mf, &mf_value)) {
-        ovs_fatal(0, "%s is not valid valid for field %s", value, key);
+        free(orig);
+        return ofputil_perr_str("%s is not valid valid for field %s",
+                                value, key);
     }
     ofpact_set_field_init(load, mf, &mf_value);
     free(orig);
+
+    return NULL;
 }
 
-static void
+WARN_UNUSED_RESULT static char *
 parse_metadata(struct ofpbuf *b, char *arg)
 {
     struct ofpact_metadata *om;
     char *mask = strchr(arg, '/');
+    uint64_t u;
 
     om = ofpact_put_WRITE_METADATA(b);
 
     if (mask) {
         *mask = '\0';
-        om->mask = htonll(str_to_u64(mask + 1));
+        IF_PERR_RET(str_to_u64(mask + 1, &u));
+        om->mask = htonll(u);
     } else {
         om->mask = htonll(UINT64_MAX);
     }
 
-    om->metadata = htonll(str_to_u64(arg));
+    IF_PERR_RET(str_to_u64(arg, &u));
+    om->metadata = htonll(u);
+
+    return NULL;
 }
 
-static void
+WARN_UNUSED_RESULT static char *
 parse_named_action(enum ofputil_action_code code, const struct flow *flow,
                    char *arg, struct ofpbuf *ofpacts)
 {
@@ -406,29 +463,29 @@ parse_named_action(enum ofputil_action_code code, const struct flow *flow,
 
     case OFPUTIL_OFPAT10_OUTPUT:
     case OFPUTIL_OFPAT11_OUTPUT:
-        parse_output(arg, ofpacts);
+        IF_PERR_RET(parse_output(arg, ofpacts));
         break;
 
     case OFPUTIL_OFPAT10_SET_VLAN_VID:
     case OFPUTIL_OFPAT11_SET_VLAN_VID:
-        vid = str_to_u32(arg);
+        IF_PERR_RET(str_to_u16(arg, "vid", &vid));
         if (vid & ~VLAN_VID_MASK) {
-            ovs_fatal(0, "%s: not a valid VLAN VID", arg);
+            return ofputil_perr_str("%s: not a valid VLAN VID", arg);
         }
         ofpact_put_SET_VLAN_VID(ofpacts)->vlan_vid = vid;
         break;
 
     case OFPUTIL_OFPAT10_SET_VLAN_PCP:
     case OFPUTIL_OFPAT11_SET_VLAN_PCP:
-        pcp = str_to_u32(arg);
+        IF_PERR_RET(str_to_u8(arg,&pcp));
         if (pcp & ~7) {
-            ovs_fatal(0, "%s: not a valid VLAN PCP", arg);
+            return ofputil_perr_str("%s: not a valid VLAN PCP", arg);
         }
         ofpact_put_SET_VLAN_PCP(ofpacts)->vlan_pcp = pcp;
         break;
 
     case OFPUTIL_OFPAT12_SET_FIELD:
-        set_field_parse(arg, ofpacts);
+        IF_PERR_RET(set_field_parse(arg, ofpacts));
         break;
 
     case OFPUTIL_OFPAT10_STRIP_VLAN:
@@ -437,46 +494,46 @@ parse_named_action(enum ofputil_action_code code, const struct flow *flow,
         break;
 
     case OFPUTIL_OFPAT11_PUSH_VLAN:
-        ethertype = str_to_u16(arg, "ethertype");
+        IF_PERR_RET(str_to_u16(arg, "ethertype", &ethertype));
         if (ethertype != ETH_TYPE_VLAN_8021Q) {
             /* XXX ETH_TYPE_VLAN_8021AD case isn't supported */
-            ovs_fatal(0, "%s: not a valid VLAN ethertype", arg);
+            return ofputil_perr_str("%s: not a valid VLAN ethertype", arg);
         }
         ofpact_put_PUSH_VLAN(ofpacts);
         break;
 
     case OFPUTIL_OFPAT11_SET_QUEUE:
-        ofpact_put_SET_QUEUE(ofpacts)->queue_id = str_to_u32(arg);
+        IF_PERR_RET(str_to_u32(arg, &ofpact_put_SET_QUEUE(ofpacts)->queue_id));
         break;
 
 
     case OFPUTIL_OFPAT10_SET_DL_SRC:
     case OFPUTIL_OFPAT11_SET_DL_SRC:
-        str_to_mac(arg, ofpact_put_SET_ETH_SRC(ofpacts)->mac);
+        IF_PERR_RET(str_to_mac(arg, ofpact_put_SET_ETH_SRC(ofpacts)->mac));
         break;
 
     case OFPUTIL_OFPAT10_SET_DL_DST:
     case OFPUTIL_OFPAT11_SET_DL_DST:
-        str_to_mac(arg, ofpact_put_SET_ETH_DST(ofpacts)->mac);
+        IF_PERR_RET(str_to_mac(arg, ofpact_put_SET_ETH_DST(ofpacts)->mac));
         break;
 
     case OFPUTIL_OFPAT10_SET_NW_SRC:
     case OFPUTIL_OFPAT11_SET_NW_SRC:
-        str_to_ip(arg, &ip);
+        IF_PERR_RET(str_to_ip(arg, &ip));
         ofpact_put_SET_IPV4_SRC(ofpacts)->ipv4 = ip;
         break;
 
     case OFPUTIL_OFPAT10_SET_NW_DST:
     case OFPUTIL_OFPAT11_SET_NW_DST:
-        str_to_ip(arg, &ip);
+        IF_PERR_RET(str_to_ip(arg, &ip));
         ofpact_put_SET_IPV4_DST(ofpacts)->ipv4 = ip;
         break;
 
     case OFPUTIL_OFPAT10_SET_NW_TOS:
     case OFPUTIL_OFPAT11_SET_NW_TOS:
-        tos = str_to_u32(arg);
+        IF_PERR_RET(str_to_u8(arg, &tos));
         if (tos & ~IP_DSCP_MASK) {
-            ovs_fatal(0, "%s: not a valid TOS", arg);
+            return ofputil_perr_str("%s: not a valid TOS", arg);
         }
         ofpact_put_SET_IPV4_DSCP(ofpacts)->dscp = tos;
         break;
@@ -486,35 +543,49 @@ parse_named_action(enum ofputil_action_code code, const struct flow *flow,
 
     case OFPUTIL_OFPAT10_SET_TP_SRC:
     case OFPUTIL_OFPAT11_SET_TP_SRC:
-        ofpact_put_SET_L4_SRC_PORT(ofpacts)->port = str_to_u32(arg);
+        {
+            uint32_t u;
+
+            IF_PERR_RET(str_to_u32(arg, &u));
+            ofpact_put_SET_L4_SRC_PORT(ofpacts)->port = u;
+        };
         break;
 
     case OFPUTIL_OFPAT10_SET_TP_DST:
     case OFPUTIL_OFPAT11_SET_TP_DST:
-        ofpact_put_SET_L4_DST_PORT(ofpacts)->port = str_to_u32(arg);
+        {
+            uint32_t u;
+
+            IF_PERR_RET(str_to_u32(arg, &u));
+            ofpact_put_SET_L4_DST_PORT(ofpacts)->port = u;
+        };
         break;
 
     case OFPUTIL_OFPAT10_ENQUEUE:
-        parse_enqueue(arg, ofpacts);
+        IF_PERR_RET(parse_enqueue(arg, ofpacts));
         break;
 
     case OFPUTIL_NXAST_RESUBMIT:
-        parse_resubmit(arg, ofpacts);
+        IF_PERR_RET(parse_resubmit(arg, ofpacts));
         break;
 
     case OFPUTIL_NXAST_SET_TUNNEL:
     case OFPUTIL_NXAST_SET_TUNNEL64:
-        tunnel = ofpact_put_SET_TUNNEL(ofpacts);
-        tunnel->ofpact.compat = code;
-        tunnel->tun_id = str_to_u64(arg);
+        {
+            uint64_t u;
+            IF_PERR_RET(str_to_u64(arg, &u));
+            tunnel = ofpact_put_SET_TUNNEL(ofpacts);
+            tunnel->ofpact.compat = code;
+            tunnel->tun_id = u;
+        }
         break;
 
     case OFPUTIL_NXAST_WRITE_METADATA:
-        parse_metadata(ofpacts, arg);
+        IF_PERR_RET(parse_metadata(ofpacts, arg));
         break;
 
     case OFPUTIL_NXAST_SET_QUEUE:
-        ofpact_put_SET_QUEUE(ofpacts)->queue_id = str_to_u32(arg);
+        IF_PERR_RET(str_to_u32(arg, &ofpact_put_SET_QUEUE(ofpacts)->queue_id));
         break;
 
     case OFPUTIL_NXAST_POP_QUEUE:
@@ -522,27 +593,27 @@ parse_named_action(enum ofputil_action_code code, const struct flow *flow,
         break;
 
     case OFPUTIL_NXAST_REG_MOVE:
-        nxm_parse_reg_move(ofpact_put_REG_MOVE(ofpacts), arg);
+        IF_PERR_RET(nxm_parse_reg_move(ofpact_put_REG_MOVE(ofpacts), arg));
         break;
 
     case OFPUTIL_NXAST_REG_LOAD:
-        nxm_parse_reg_load(ofpact_put_REG_LOAD(ofpacts), arg);
+        IF_PERR_RET(nxm_parse_reg_load(ofpact_put_REG_LOAD(ofpacts), arg));
         break;
 
     case OFPUTIL_NXAST_NOTE:
-        parse_note(arg, ofpacts);
+        IF_PERR_RET(parse_note(arg, ofpacts));
         break;
 
     case OFPUTIL_NXAST_MULTIPATH:
-        multipath_parse(ofpact_put_MULTIPATH(ofpacts), arg);
+        IF_PERR_RET(multipath_parse(ofpact_put_MULTIPATH(ofpacts), arg));
         break;
 
     case OFPUTIL_NXAST_BUNDLE:
-        bundle_parse(arg, ofpacts);
+        IF_PERR_RET(bundle_parse(arg, ofpacts));
         break;
 
     case OFPUTIL_NXAST_BUNDLE_LOAD:
-        bundle_parse_load(arg, ofpacts);
+        IF_PERR_RET(bundle_parse_load(arg, ofpacts));
         break;
 
     case OFPUTIL_NXAST_RESUBMIT_TABLE:
@@ -559,12 +630,12 @@ parse_named_action(enum ofputil_action_code code, const struct flow *flow,
         break;
 
     case OFPUTIL_NXAST_DEC_TTL:
-        parse_dec_ttl(ofpacts, arg);
+        IF_PERR_RET(parse_dec_ttl(ofpacts, arg));
         break;
 
     case OFPUTIL_NXAST_SET_MPLS_TTL:
     case OFPUTIL_OFPAT11_SET_MPLS_TTL:
-        parse_set_mpls_ttl(ofpacts, arg);
+        IF_PERR_RET(parse_set_mpls_ttl(ofpacts, arg));
         break;
 
     case OFPUTIL_OFPAT11_DEC_MPLS_TTL:
@@ -573,62 +644,71 @@ parse_named_action(enum ofputil_action_code code, const struct flow *flow,
         break;
 
     case OFPUTIL_NXAST_FIN_TIMEOUT:
-        parse_fin_timeout(ofpacts, arg);
+        IF_PERR_RET(parse_fin_timeout(ofpacts, arg));
         break;
 
     case OFPUTIL_NXAST_CONTROLLER:
-        parse_controller(ofpacts, arg);
+        IF_PERR_RET(parse_controller(ofpacts, arg));
         break;
 
     case OFPUTIL_OFPAT11_PUSH_MPLS:
     case OFPUTIL_NXAST_PUSH_MPLS:
-        ofpact_put_PUSH_MPLS(ofpacts)->ethertype =
-            htons(str_to_u16(arg, "push_mpls"));
+        {
+            uint16_t u;
+            IF_PERR_RET(str_to_u16(arg, "push_mpls", &u));
+            ofpact_put_PUSH_MPLS(ofpacts)->ethertype = htons(u);
+        }
         break;
 
     case OFPUTIL_OFPAT11_POP_MPLS:
     case OFPUTIL_NXAST_POP_MPLS:
-        ofpact_put_POP_MPLS(ofpacts)->ethertype =
-            htons(str_to_u16(arg, "pop_mpls"));
+        {
+            uint16_t u;
+            IF_PERR_RET(str_to_u16(arg, "pop_mpls", &u));
+            ofpact_put_POP_MPLS(ofpacts)->ethertype = htons(u);
+        }
         break;
     case OFPUTIL_NXAST_STACK_PUSH:
-        nxm_parse_stack_action(ofpact_put_STACK_PUSH(ofpacts), arg);
+        IF_PERR_RET(nxm_parse_stack_action(ofpact_put_STACK_PUSH(ofpacts),
+                                           arg));
         break;
     case OFPUTIL_NXAST_STACK_POP:
-        nxm_parse_stack_action(ofpact_put_STACK_POP(ofpacts), arg);
+        IF_PERR_RET(nxm_parse_stack_action(ofpact_put_STACK_POP(ofpacts),
+                                           arg));
         break;
     }
+
+    return NULL;
 }
 
-static bool
+WARN_UNUSED_RESULT static char *
 str_to_ofpact__(const struct flow *flow, char *pos, char *act, char *arg,
                 struct ofpbuf *ofpacts, int n_actions)
 {
     int code = ofputil_action_code_from_name(act);
     if (code >= 0) {
-        parse_named_action(code, flow, arg, ofpacts);
+        IF_PERR_RET(parse_named_action(code, flow, arg, ofpacts));
     } else if (!strcasecmp(act, "drop")) {
         if (n_actions) {
-            ovs_fatal(0, "Drop actions must not be preceded by other "
-                      "actions");
+            return ofputil_perr_str("Drop actions must not"
+                                    " be preceded by other actions");
         } else if (ofputil_parse_key_value(&pos, &act, &arg)) {
-            ovs_fatal(0, "Drop actions must not be followed by other "
-                      "actions");
+            return ofputil_perr_str("Drop actions must not"
+                                    " be followed by other actions");
         }
-        return false;
     } else {
         uint16_t port;
         if (ofputil_port_from_string(act, &port)) {
             ofpact_put_OUTPUT(ofpacts)->port = port;
         } else {
-            ovs_fatal(0, "Unknown action: %s", act);
+            return ofputil_perr_str("Unknown action: %s", act);
         }
     }
 
-    return true;
+    return NULL;
 }
 
-static void
+WARN_UNUSED_RESULT static char *
 str_to_ofpacts(const struct flow *flow, char *str, struct ofpbuf *ofpacts)
 {
     char *pos, *act, *arg;
@@ -638,21 +718,21 @@ str_to_ofpacts(const struct flow *flow, char *str, struct ofpbuf *ofpacts)
     pos = str;
     n_actions = 0;
     while (ofputil_parse_key_value(&pos, &act, &arg)) {
-        if (!str_to_ofpact__(flow, pos, act, arg, ofpacts, n_actions)) {
-            break;
-        }
+        IF_PERR_RET(str_to_ofpact__(flow, pos, act, arg, ofpacts, n_actions));
         n_actions++;
     }
 
     error = ofpacts_verify(ofpacts->data, ofpacts->size);
-    if (error) {
-        ovs_fatal(0, "Incorrect action ordering");
+    if (!error) {
+         ofpact_pad(ofpacts);
+    } else {
+         return ofputil_perr_str("Incorrect action ordering");
     }
 
-    ofpact_pad(ofpacts);
+    return NULL;
 }
 
-static void
+WARN_UNUSED_RESULT static char *
 parse_named_instruction(enum ovs_instruction_type type,
                         char *arg, struct ofpbuf *ofpacts)
 {
@@ -665,7 +745,8 @@ parse_named_instruction(enum ovs_instruction_type type,
 
     case OVSINST_OFPIT11_WRITE_ACTIONS:
         /* XXX */
-        ovs_fatal(0, "instruction write-actions is not supported yet");
+        return ofputil_perr_str("instruction write-actions"
+                                " is not supported yet");
         break;
 
     case OVSINST_OFPIT11_CLEAR_ACTIONS:
@@ -673,16 +754,16 @@ parse_named_instruction(enum ovs_instruction_type type,
         break;
 
     case OVSINST_OFPIT11_WRITE_METADATA:
-        parse_metadata(ofpacts, arg);
+        IF_PERR_RET(parse_metadata(ofpacts, arg));
         break;
 
     case OVSINST_OFPIT11_GOTO_TABLE: {
         struct ofpact_goto_table *ogt = ofpact_put_GOTO_TABLE(ofpacts);
         char *table_s = strsep(&arg, ",");
         if (!table_s || !table_s[0]) {
-            ovs_fatal(0, "instruction goto-table needs table id");
+            return ofputil_perr_str("instruction goto-table needs table id");
         }
-        ogt->table_id = str_to_table_id(table_s);
+        IF_PERR_RET(str_to_table_id(table_s, &ogt->table_id));
         break;
     }
     }
@@ -691,11 +772,13 @@ parse_named_instruction(enum ovs_instruction_type type,
        could be invalid. */
     error = ofpacts_verify(ofpacts->data, ofpacts->size);
     if (error) {
-        ovs_fatal(0, "Incorrect instruction ordering");
+        return ofputil_perr_str("Incorrect instruction ordering");
     }
+
+    return NULL;
 }
 
-static void
+static char *
 str_to_inst_ofpacts(const struct flow *flow, char *str, struct ofpbuf *ofpacts)
 {
     char *pos, *inst, *arg;
@@ -708,9 +791,8 @@ str_to_inst_ofpacts(const struct flow *flow, char *str, struct ofpbuf *ofpacts)
     while (ofputil_parse_key_value(&pos, &inst, &arg)) {
         type = ofpact_instruction_type_from_name(inst);
         if (type < 0) {
-            if (!str_to_ofpact__(flow, pos, inst, arg, ofpacts, n_actions)) {
-                break;
-            }
+            IF_PERR_RET(str_to_ofpact__(flow, pos, inst, arg, ofpacts,
+                                        n_actions));
 
             type = OVSINST_OFPIT11_APPLY_ACTIONS;
             if (prev_type == type) {
@@ -718,19 +800,20 @@ str_to_inst_ofpacts(const struct flow *flow, char *str, struct ofpbuf *ofpacts)
                 continue;
             }
         } else if (type == OVSINST_OFPIT11_APPLY_ACTIONS) {
-            ovs_fatal(0, "%s isn't supported. Just write actions then "
-                      "it is interpreted as apply_actions", inst);
+            return ofputil_perr_str("%s isn't supported. Just write"
+                               " actions then it is interpreted as"
+                               " apply_actions", inst);
         } else {
-            parse_named_instruction(type, arg, ofpacts);
+            IF_PERR_RET(parse_named_instruction(type, arg, ofpacts));
         }
 
         if (type == prev_type) {
-            ovs_fatal(0, "instruction can be specified at most once: %s",
-                      inst);
+            return ofputil_perr_str("Instruction can be specified"
+                                    " at most once: %s", inst);
         }
         if (type <= prev_type) {
-            ovs_fatal(0, "Instruction %s must be specified before %s",
-                      inst, prev_inst);
+            return ofputil_perr_str("Instruction %s must be specified"
+                                    " before %s", inst, prev_inst);
         }
 
         prev_inst = inst;
@@ -738,6 +821,7 @@ str_to_inst_ofpacts(const struct flow *flow, char *str, struct ofpbuf *ofpacts)
         n_actions++;
     }
     ofpact_pad(ofpacts);
+    return NULL;
 }
 
 struct protocol {
@@ -776,31 +860,31 @@ parse_protocol(const char *name, const struct protocol **p_out)
     return false;
 }
 
-static void
-ofp_fatal(const char *flow, bool verbose, const char *format, ...)
+WARN_UNUSED_RESULT static char *
+ofp_err_str(const char *flow, bool verbose, const char *format, ...)
 {
     va_list args;
+    struct ds err_str;
+
+    ds_init(&err_str);
 
     if (verbose) {
-        fprintf(stderr, "%s:\n", flow);
+        ds_put_format(&err_str, "%s\n", flow);
     }
 
     va_start(args, format);
-    ovs_fatal_valist(0, format, args);
+    ds_put_format_valist(&err_str, format, args);
+
+    return err_str.string;
 }
 
-static void
+WARN_UNUSED_RESULT static char *
 parse_field(const struct mf_field *mf, const char *s, struct match *match)
 {
     union mf_value value, mask;
-    char *error;
-
-    error = mf_parse(mf, s, &value, &mask);
-    if (error) {
-        ovs_fatal(0, "%s", error);
-    }
-
+    IF_PERR_RET(mf_parse(mf, s, &value, &mask));
     mf_set(mf, &value, &mask, match);
+    return NULL;
 }
 
 /* Convert 'str_' (as described in the Flow Syntax section of the ovs-ofctl man
@@ -810,11 +894,15 @@ parse_field(const struct mf_field *mf, const char *s, struct match *match)
  *
  * To parse syntax for an OFPT_FLOW_MOD (or NXT_FLOW_MOD), use an OFPFC_*
  * constant for 'command'.  To parse syntax for an OFPST_FLOW or
- * OFPST_AGGREGATE (or NXST_FLOW or NXST_AGGREGATE), use -1 for 'command'. */
-void
+ * OFPST_AGGREGATE (or NXST_FLOW or NXST_AGGREGATE), use -1 for 'command'.
+ *
+ * Return 0 if successful, otherwise a string indicating the parser error.
+ */
+char *
 parse_ofp_str(struct ofputil_flow_mod *fm, int command, const char *str_,
               bool verbose)
 {
+    uint64_t v;
     enum {
         F_OUT_PORT = 1 << 0,
         F_ACTIONS = 1 << 1,
@@ -826,7 +914,10 @@ parse_ofp_str(struct ofputil_flow_mod *fm, int command, const char *str_,
     char *save_ptr = NULL;
     char *act_str = NULL;
     char *name;
-
+    #define CF (free(string))    /* The clean up function, in case we
+                                  * need to exit from the middle of the
+                                  * function
+                                  */
     switch (command) {
     case -1:
         fields = F_OUT_PORT;
@@ -876,13 +967,13 @@ parse_ofp_str(struct ofputil_flow_mod *fm, int command, const char *str_,
     if (fields & F_ACTIONS) {
         act_str = strstr(string, "action");
         if (!act_str) {
-            ofp_fatal(str_, verbose, "must specify an action");
+            return ofp_err_str(str_, verbose, "must specify an action");
         }
         *act_str = '\0';
 
         act_str = strchr(act_str + 1, '=');
         if (!act_str) {
-            ofp_fatal(str_, verbose, "must specify an action");
+            return ofp_err_str(str_, verbose, "must specify an action");
         }
 
         act_str++;
@@ -911,44 +1002,55 @@ parse_ofp_str(struct ofputil_flow_mod *fm, int command, const char *str_,
 
             value = strtok_r(NULL, ", \t\r\n", &save_ptr);
             if (!value) {
-                ofp_fatal(str_, verbose, "field %s missing value", name);
+                return ofp_err_str(str_, verbose,
+                                   "field %s missing value", name);
             }
 
             if (!strcmp(name, "table")) {
-                fm->table_id = str_to_table_id(value);
+                IF_PERR_CRET(str_to_table_id(value, &fm->table_id), CF);
             } else if (!strcmp(name, "out_port")) {
                 if (!ofputil_port_from_string(name, &fm->out_port)) {
-                    ofp_fatal(str_, verbose, "%s is not a valid OpenFlow port",
-                              name);
+                    return ofp_err_str(str_, verbose,
+                                       "%s is not a valid OpenFlow port",
+                                        name);
                 }
             } else if (fields & F_PRIORITY && !strcmp(name, "priority")) {
-                fm->priority = str_to_u16(value, name);
+                uint16_t v16;
+                IF_PERR_CRET(str_to_u16(value, name, &v16), CF);
+                fm->priority = v16;
             } else if (fields & F_TIMEOUT && !strcmp(name, "idle_timeout")) {
-                fm->idle_timeout = str_to_u16(value, name);
+                IF_PERR_CRET(str_to_u16(value, name, &fm->idle_timeout), CF);
             } else if (fields & F_TIMEOUT && !strcmp(name, "hard_timeout")) {
-                fm->hard_timeout = str_to_u16(value, name);
+                IF_PERR_CRET(str_to_u16(value, name, &fm->hard_timeout), CF);
             } else if (!strcmp(name, "cookie")) {
                 char *mask = strchr(value, '/');
 
                 if (mask) {
                     /* A mask means we're searching for a cookie. */
                     if (command == OFPFC_ADD) {
-                        ofp_fatal(str_, verbose, "flow additions cannot use "
-                                  "a cookie mask");
+                        return ofp_err_str(str_, verbose,
+                                           "flow additions cannot use"
+                                           " a cookie mask");
                     }
                     *mask = '\0';
-                    fm->cookie = htonll(str_to_u64(value));
-                    fm->cookie_mask = htonll(str_to_u64(mask+1));
+
+                    IF_PERR_CRET(str_to_u64(value, &v), CF);
+                    fm->cookie = htonll(v);
+                    IF_PERR_CRET(str_to_u64(mask+1, &v), CF);
+                    fm->cookie_mask = htonll(v);
                 } else {
                     /* No mask means that the cookie is being set. */
                     if (command != OFPFC_ADD && command != OFPFC_MODIFY
                             && command != OFPFC_MODIFY_STRICT) {
-                        ofp_fatal(str_, verbose, "cannot set cookie");
+                        return ofp_err_str(str_, verbose,
+                                           "cannot set cookie");
                     }
-                    fm->new_cookie = htonll(str_to_u64(value));
+                    IF_PERR_CRET(str_to_u64(value, &v), CF);
+                    fm->new_cookie = htonll(v);
                 }
             } else if (mf_from_name(name)) {
-                parse_field(mf_from_name(name), value, &fm->match);
+                IF_PERR_CRET(parse_field(mf_from_name(name),
+                                        value, &fm->match), CF);
             } else if (!strcmp(name, "duration")
                        || !strcmp(name, "n_packets")
                        || !strcmp(name, "n_bytes")
@@ -958,7 +1060,8 @@ parse_ofp_str(struct ofputil_flow_mod *fm, int command, const char *str_,
                  * "ovs-ofctl dump-flows" back into commands that parse
                  * flows. */
             } else {
-                ofp_fatal(str_, verbose, "unknown keyword %s", name);
+                free(string);
+                return ofp_err_str(str_, verbose, "unknown keyword %s", name);
             }
         }
     }
@@ -973,7 +1076,9 @@ parse_ofp_str(struct ofputil_flow_mod *fm, int command, const char *str_,
         struct ofpbuf ofpacts;
 
         ofpbuf_init(&ofpacts, 32);
-        str_to_inst_ofpacts(&fm->match.flow, act_str, &ofpacts);
+        IF_PERR_CRET(str_to_inst_ofpacts(&fm->match.flow, act_str, &ofpacts),
+                     {CF; ofpbuf_uninit(&ofpacts);});
+
         fm->ofpacts_len = ofpacts.size;
         fm->ofpacts = ofpbuf_steal_data(&ofpacts);
     } else {
@@ -982,6 +1087,7 @@ parse_ofp_str(struct ofputil_flow_mod *fm, int command, const char *str_,
     }
 
     free(string);
+    return NULL;
 }
 
 /* Convert 'str_' (as described in the documentation for the "monitor" command
@@ -1033,11 +1139,12 @@ parse_flow_monitor_request(struct ofputil_flow_monitor_request *fmr,
             }
 
             if (!strcmp(name, "table")) {
-                fmr->table_id = str_to_table_id(value);
+                IF_PERR_EXIT(str_to_table_id(value, &fmr->table_id));
             } else if (!strcmp(name, "out_port")) {
                 fmr->out_port = atoi(value);
             } else if (mf_from_name(name)) {
-                parse_field(mf_from_name(name), value, &fmr->match);
+                IF_PERR_EXIT(parse_field(mf_from_name(name),
+                                         value, &fmr->match));
             } else {
                 ovs_fatal(0, "%s: unknown keyword %s", str_, name);
             }
@@ -1051,29 +1158,33 @@ parse_flow_monitor_request(struct ofputil_flow_monitor_request *fmr,
  *
  * Prints an error on stderr and aborts the program if 's' syntax is
  * invalid. */
-void
+char *
 parse_ofpacts(const char *s_, struct ofpbuf *ofpacts)
 {
     char *s = xstrdup(s_);
-    str_to_ofpacts(NULL, s, ofpacts);
+    IF_PERR_CRET(str_to_ofpacts(NULL, s, ofpacts), free(s));
+
     free(s);
+    return NULL;
 }
 
 /* Parses 'string' as an OFPT_FLOW_MOD or NXT_FLOW_MOD with command 'command'
  * (one of OFPFC_*) into 'fm'. */
-void
+char *
 parse_ofp_flow_mod_str(struct ofputil_flow_mod *fm, const char *string,
                        uint16_t command, bool verbose)
 {
     struct match match_copy;
 
-    parse_ofp_str(fm, command, string, verbose);
+    IF_PERR_RET(parse_ofp_str(fm, command, string, verbose));
 
     /* Normalize a copy of the match.  This ensures that non-normalized flows
      * get logged but doesn't affect what gets sent to the switch, so that the
      * switch can do whatever it likes with the flow. */
     match_copy = fm->match;
     ofputil_normalize_match(&match_copy);
+
+    return NULL;
 }
 
 void
@@ -1083,6 +1194,7 @@ parse_ofp_flow_mod_file(const char *file_name, uint16_t command,
     size_t allocated_fms;
     FILE *stream;
     struct ds s;
+    int lineno = 0;
 
     stream = !strcmp(file_name, "-") ? stdin : fopen(file_name, "r");
     if (stream == NULL) {
@@ -1091,11 +1203,24 @@ parse_ofp_flow_mod_file(const char *file_name, uint16_t command,
 
     allocated_fms = *n_fms;
     ds_init(&s);
-    while (!ds_get_preprocessed_line(&s, stream)) {
+    while (!ds_get_line(&s, stream)) {
+        char *err_str;
+        lineno++;
+
+        /* If the line is empty, skip it. */
+        if (ds_preprocess_line(&s) == false) {
+            continue;
+        }
+
         if (*n_fms >= allocated_fms) {
             *fms = x2nrealloc(*fms, &allocated_fms, sizeof **fms);
         }
-        parse_ofp_flow_mod_str(&(*fms)[*n_fms], ds_cstr(&s), command, false);
+        err_str = parse_ofp_flow_mod_str(&(*fms)[*n_fms], ds_cstr(&s),
+                                         command, false);
+        if (err_str) {
+            fprintf(stderr, "%s:%d ", file_name, lineno);
+            parser_fatal_exit_with_reason(err_str);
+        }
         *n_fms += 1;
     }
     ds_destroy(&s);
@@ -1111,7 +1236,8 @@ parse_ofp_flow_stats_request_str(struct ofputil_flow_stats_request *fsr,
 {
     struct ofputil_flow_mod fm;
 
-    parse_ofp_str(&fm, -1, string, false);
+    IF_PERR_EXIT(parse_ofp_str(&fm, -1, string, false));
+
     fsr->aggregate = aggregate;
     fsr->cookie = fm.cookie;
     fsr->cookie_mask = fm.cookie_mask;
@@ -1200,3 +1326,15 @@ exit:
     }
     return error;
 }
+
+/* Display the err_str to stderr, then exit.
+ *
+ * This function will free the memory allocated for err_str.
+ */
+void
+parser_fatal_exit_with_reason(char *err_str)
+{
+    fprintf(stderr, "%s\n", err_str);
+    free(err_str);
+    exit(EXIT_FAILURE);
+}
diff --git a/lib/ofp-parse.h b/lib/ofp-parse.h
index d2d3c3c..a3aaaae 100644
--- a/lib/ofp-parse.h
+++ b/lib/ofp-parse.h
@@ -22,6 +22,9 @@
 #include <stdbool.h>
 #include <stdint.h>
 #include <stdio.h>
+#include <stdarg.h>
+
+#include "compiler.h"
 
 struct flow;
 struct ofpbuf;
@@ -29,11 +32,12 @@ struct ofputil_flow_mod;
 struct ofputil_flow_monitor_request;
 struct ofputil_flow_stats_request;
 
-void parse_ofp_str(struct ofputil_flow_mod *, int command, const char *str_,
-                   bool verbose);
+char *parse_ofp_str(struct ofputil_flow_mod *, int command,
+                    const char *str_, bool verbose) WARN_UNUSED_RESULT;
+
+char *parse_ofp_flow_mod_str(struct ofputil_flow_mod *, const char *string,
+                            uint16_t command, bool verbose) WARN_UNUSED_RESULT;
 
-void parse_ofp_flow_mod_str(struct ofputil_flow_mod *, const char *string,
-                            uint16_t command, bool verbose);
 void parse_ofp_flow_mod_file(const char *file_name, uint16_t command,
                              struct ofputil_flow_mod **fms, size_t *n_fms);
 
@@ -41,11 +45,51 @@ void parse_ofp_flow_stats_request_str(struct ofputil_flow_stats_request *,
                                       bool aggregate, const char *string);
 
 
-void parse_ofpacts(const char *, struct ofpbuf *ofpacts);
+char *parse_ofpacts(const char *, struct ofpbuf *ofpacts) WARN_UNUSED_RESULT;
 
-char *parse_ofp_exact_flow(struct flow *, const char *);
+char *parse_ofp_exact_flow(struct flow *, const char *) WARN_UNUSED_RESULT;
 
 void parse_flow_monitor_request(struct ofputil_flow_monitor_request *,
                                 const char *);
 
+void parser_fatal_exit_with_reason(char* err_str);
+
+/* Both 'IF_PERR_RET' and 'IF_PERR_CRET' will check the return
+ * value of the parser function 'pf'. In case 'pf' retuns
+ * an error string, return this error string immediately to the caller.
+ * if the error string is NULL, the program will contine to execute, from
+ * the next statement.
+ *
+ * In addition, 'IF_PERR_CRET(pf, cf)' executes a cleanup function 'cf'
+ * before returning.
+ *
+ * Both 'IF_PERR_EXIT' and 'IF_PERR_EXIT_MSG' will also check the
+ * return value of a parser function 'pf', When 'pf' returns an error
+ * string, both macros will exit current running process with exit code
+ * 1.   'IF_PERR_EXIT_MSG' accepts a printf style error string. which
+ * will be sent to stderr. The error string 'pf' returns is assumed to
+ * be the last paramter.
+ */
+#define IF_PERR_CRET(pf, cf) \
+    {\
+        char *err_str = pf;\
+        if (err_str) {\
+            cf; /* Clean up first */\
+            return err_str; \
+         }\
+    }
+
+#define IF_PERR_RET(pf)  IF_PERR_CRET(pf, ;)
+
+#define IF_PERR_EXIT_MSG(pf, ...) \
+    {\
+        char *err_str = pf;\
+        if (err_str) {\
+            ovs_error(0, __VA_ARGS__, err_str); \
+            exit(1); \
+        }\
+    }
+
+#define IF_PERR_EXIT(pf) IF_PERR_EXIT_MSG(pf, "%s")
+
 #endif /* ofp-parse.h */
diff --git a/lib/ofp-util.c b/lib/ofp-util.c
index 6b78f84..123db87 100644
--- a/lib/ofp-util.c
+++ b/lib/ofp-util.c
@@ -5054,3 +5054,22 @@ ofputil_append_queue_stat(struct list *replies,
         NOT_REACHED();
     }
 }
+
+/* Generate a formated parser error string. $
+ *
+ * Note: This function will allocate memory to store the error string
+ * The caller is responsible for freeing the memory.  $
+ */
+char *
+ofputil_perr_str(const char* format, ...)
+{
+    va_list args;
+    struct ds err_str;
+
+    ds_init(&err_str);
+
+    va_start(args, format);
+    ds_put_format_valist(&err_str, format, args);
+
+    return err_str.string;
+}
diff --git a/lib/ofp-util.h b/lib/ofp-util.h
index db28c58..124bd5e 100644
--- a/lib/ofp-util.h
+++ b/lib/ofp-util.h
@@ -723,4 +723,6 @@ int ofputil_decode_queue_stats(struct ofputil_queue_stats *qs, struct ofpbuf *ms
 void ofputil_append_queue_stat(struct list *replies,
                                const struct ofputil_queue_stats *oqs);
 
+char *ofputil_perr_str(const char* format, ...) WARN_UNUSED_RESULT;
+
 #endif /* ofp-util.h */
diff --git a/tests/test-bundle.c b/tests/test-bundle.c
index f5b24b4..8cd993b 100644
--- a/tests/test-bundle.c
+++ b/tests/test-bundle.c
@@ -22,6 +22,7 @@
 
 #include "flow.h"
 #include "ofp-actions.h"
+#include "ofp-parse.h"
 #include "ofpbuf.h"
 #include "random.h"
 
@@ -73,7 +74,8 @@ parse_bundle_actions(char *actions)
     struct ofpact *action;
 
     ofpbuf_init(&ofpacts, 0);
-    bundle_parse_load(actions, &ofpacts);
+    IF_PERR_EXIT(bundle_parse_load(actions, &ofpacts));
+
     action = ofpacts.data;
     bundle = ofpact_get_BUNDLE(xmemdup(action, action->len));
     ofpbuf_uninit(&ofpacts);
diff --git a/tests/test-multipath.c b/tests/test-multipath.c
index 8442bc2..5173ee0 100644
--- a/tests/test-multipath.c
+++ b/tests/test-multipath.c
@@ -26,6 +26,7 @@
 
 #include "flow.h"
 #include "ofp-actions.h"
+#include "ofp-parse.h"
 #include "random.h"
 #include "util.h"
 
@@ -44,7 +45,7 @@ main(int argc, char *argv[])
         ovs_fatal(0, "usage: %s multipath_action", program_name);
     }
 
-    multipath_parse(&mp, argv[1]);
+    IF_PERR_EXIT(multipath_parse(&mp, argv[1]));
     for (n = 1; n <= MP_MAX_LINKS; n++) {
         enum { N_FLOWS = 65536 };
         double disruption, perfect, distribution;
diff --git a/utilities/ovs-ofctl.c b/utilities/ovs-ofctl.c
index ec775c7..8ee37f2 100644
--- a/utilities/ovs-ofctl.c
+++ b/utilities/ovs-ofctl.c
@@ -1090,7 +1090,8 @@ ofctl_flow_mod(int argc, char *argv[], uint16_t command)
         ofctl_flow_mod_file(argc, argv, command);
     } else {
         struct ofputil_flow_mod fm;
-        parse_ofp_flow_mod_str(&fm, argc > 2 ? argv[2] : "", command, false);
+        IF_PERR_EXIT(parse_ofp_flow_mod_str(&fm, argc > 2 ? argv[2] : "",
+                                            command, false));
         ofctl_flow_mod__(argv[1], &fm, 1);
     }
 }
@@ -1513,7 +1514,7 @@ ofctl_packet_out(int argc, char *argv[])
     int i;
 
     ofpbuf_init(&ofpacts, 64);
-    parse_ofpacts(argv[3], &ofpacts);
+    IF_PERR_EXIT(parse_ofpacts(argv[3], &ofpacts));
 
     po.buffer_id = UINT32_MAX;
     po.in_port = str_to_port_no(argv[1], argv[2]);
@@ -1879,6 +1880,7 @@ read_flows_from_file(const char *filename, struct classifier *cls, int index)
     enum ofputil_protocol usable_protocols;
     struct ds s;
     FILE *file;
+    int lineno = 0;
 
     file = !strcmp(filename, "-") ? stdin : fopen(filename, "r");
     if (file == NULL) {
@@ -1887,11 +1889,23 @@ read_flows_from_file(const char *filename, struct classifier *cls, int index)
 
     ds_init(&s);
     usable_protocols = OFPUTIL_P_ANY;
-    while (!ds_get_preprocessed_line(&s, file)) {
+    while (!ds_get_line(&s, file)) {
         struct fte_version *version;
         struct ofputil_flow_mod fm;
+        char *err_str;
 
-        parse_ofp_str(&fm, OFPFC_ADD, ds_cstr(&s), true);
+        lineno++;
+
+        /* Skip parsing an empty lines. */
+        if (ds_preprocess_line(&s) == false) {
+            continue;
+        }
+
+        err_str = parse_ofp_str(&fm, OFPFC_ADD, ds_cstr(&s), true);
+        if (err_str) {
+            fprintf(stderr, "%s:%d ", filename, lineno);
+            parser_fatal_exit_with_reason(err_str);
+        }
 
         version = xmalloc(sizeof *version);
         version->cookie = fm.new_cookie;
@@ -2205,7 +2219,8 @@ ofctl_parse_flow(int argc OVS_UNUSED, char *argv[])
 {
     struct ofputil_flow_mod fm;
 
-    parse_ofp_flow_mod_str(&fm, argv[1], OFPFC_ADD, false);
+    IF_PERR_EXIT(parse_ofp_flow_mod_str(&fm, argv[1], OFPFC_ADD, false));
+
     ofctl_parse_flows__(&fm, 1);
 }
 
@@ -2663,7 +2678,7 @@ ofctl_check_vlan(int argc OVS_UNUSED, char *argv[])
     string_s = match_to_string(&match, OFP_DEFAULT_PRIORITY);
     printf("%s -> ", string_s);
     fflush(stdout);
-    parse_ofp_str(&fm, -1, string_s, false);
+    IF_PERR_EXIT(parse_ofp_str(&fm, -1, string_s, false));
     printf("%04"PRIx16"/%04"PRIx16"\n",
            ntohs(fm.match.flow.vlan_tci),
            ntohs(fm.match.wc.masks.vlan_tci));
-- 
1.7.9.5




More information about the dev mailing list