From: Ben Pfaff <blp@nicira.com>
Date: Wed, 27 Jan 2010 19:25:20 +0000 (-0800)
Subject: ovs-vsctl: Make parsing functions return error instead of aborting.
X-Git-Tag: v1.0.0~259^2~240
X-Git-Url: http://git.onelab.eu/?a=commitdiff_plain;h=1bc6ff291822a37ac95659f347e8edba125cd09c;p=sliver-openvswitch.git

ovs-vsctl: Make parsing functions return error instead of aborting.

The upcoming "remove" command for ovs-vsctl wants to try parsing an
argument two different ways.  This doesn't work if a parse error always
aborts immediately.  This commit fixes the problem, by making a parsing
failure pass up an error for higher layers to deal with instead of aborting
immediately.

This commit should have no user-visible effect.
---

diff --git a/lib/ovsdb-data.c b/lib/ovsdb-data.c
index cb68d09dc..b89aa57a6 100644
--- a/lib/ovsdb-data.c
+++ b/lib/ovsdb-data.c
@@ -371,8 +371,11 @@ ovsdb_atom_to_json(const union ovsdb_atom *atom, enum ovsdb_atomic_type type)
  *        an arbitrary string.
  *
  *      - OVSDB_TYPE_UUID: A UUID in RFC 4122 format.
+ *
+ * Returns a null pointer if successful, otherwise an error message describing
+ * the problem.  The caller is responsible for freeing the error.
  */
-void
+char *
 ovsdb_atom_from_string(union ovsdb_atom *atom, enum ovsdb_atomic_type type,
                        const char *s)
 {
@@ -383,7 +386,7 @@ ovsdb_atom_from_string(union ovsdb_atom *atom, enum ovsdb_atomic_type type,
     case OVSDB_TYPE_INTEGER: {
         long long int integer;
         if (!str_to_llong(s, 10, &integer)) {
-            ovs_fatal(0, "%s is not a valid integer", s);
+            return xasprintf("\"%s\" is not a valid integer", s);
         }
         atom->integer = integer;
     }
@@ -391,7 +394,7 @@ ovsdb_atom_from_string(union ovsdb_atom *atom, enum ovsdb_atomic_type type,
 
     case OVSDB_TYPE_REAL:
         if (!str_to_double(s, &atom->real)) {
-            ovs_fatal(0, "%s is not a valid real number", s);
+            return xasprintf("\"%s\" is not a valid real number", s);
         }
         break;
 
@@ -403,22 +406,26 @@ ovsdb_atom_from_string(union ovsdb_atom *atom, enum ovsdb_atomic_type type,
                    || !strcmp(s, "0")) {
             atom->boolean = false;
         } else {
-            ovs_fatal(0, "%s is not a valid boolean "
-                      "(use \"true\" or \"false\")", s);
+            return xasprintf("\"%s\" is not a valid boolean "
+                             "(use \"true\" or \"false\")", s);
         }
         break;
 
     case OVSDB_TYPE_STRING:
         if (*s == '\0') {
-            ovs_fatal(0, "use \"\" to represent the empty string");
+            return xstrdup("An empty string is not valid as input; "
+                           "use \"\" to represent the empty string");
         } else if (*s == '"') {
             size_t s_len = strlen(s);
 
             if (s_len < 2 || s[s_len - 1] != '"') {
-                ovs_fatal(0, "%s: missing quote at end of quoted string", s);
+                return xasprintf("%s: missing quote at end of "
+                                 "quoted string", s);
             } else if (!json_string_unescape(s + 1, s_len - 2,
                                              &atom->string)) {
-                ovs_fatal(0, "%s: %s", s, atom->string);
+                char *error = xasprintf("%s: %s", s, atom->string);
+                free(atom->string);
+                return error;
             }
         } else {
             atom->string = xstrdup(s);
@@ -427,7 +434,7 @@ ovsdb_atom_from_string(union ovsdb_atom *atom, enum ovsdb_atomic_type type,
 
     case OVSDB_TYPE_UUID:
         if (!uuid_from_string(&atom->uuid, s)) {
-            ovs_fatal(0, "%s is not a valid UUID", s);
+            return xasprintf("\"%s\" is not a valid UUID", s);
         }
         break;
 
@@ -435,6 +442,8 @@ ovsdb_atom_from_string(union ovsdb_atom *atom, enum ovsdb_atomic_type type,
     default:
         NOT_REACHED();
     }
+
+    return NULL;
 }
 
 static bool
@@ -802,31 +811,42 @@ skip_spaces(const char *p)
     return p + strspn(p, " ");
 }
 
-static const char *
-parse_key_value(const char *s, const struct ovsdb_type *type,
-                union ovsdb_atom *key, union ovsdb_atom *value)
+static char *
+parse_atom_token(const char **s, enum ovsdb_atomic_type type,
+                 union ovsdb_atom *atom)
 {
-    char *key_string;
-    const char *p;
+    char *token, *error;
 
-    /* Parse key. */
-    p = ovsdb_token_parse(s, &key_string);
-    ovsdb_atom_from_string(key, type->key_type, key_string);
-    free(key_string);
+    error = ovsdb_token_parse(s, &token);
+    if (!error) {
+        error = ovsdb_atom_from_string(atom, type, token);
+        free(token);
+    }
+    return error;
+}
 
-    /* Parse value. */
-    if (type->value_type != OVSDB_TYPE_VOID) {
-        char *value_string;
 
-        if (*p != '=') {
-            ovs_fatal(0, "%s: syntax error at \"%c\" expecting \"=\"",
-                      s, *p);
+static char *
+parse_key_value(const char **s, const struct ovsdb_type *type,
+                union ovsdb_atom *key, union ovsdb_atom *value)
+{
+    const char *start = *s;
+    char *error;
+
+    error = parse_atom_token(s, type->key_type, key);
+    if (!error && type->value_type != OVSDB_TYPE_VOID) {
+        if (**s == '=') {
+            (*s)++;
+            error = parse_atom_token(s, type->value_type, value);
+        } else {
+            error = xasprintf("%s: syntax error at \"%c\" expecting \"=\"",
+                              start, **s);
+        }
+        if (error) {
+            ovsdb_atom_destroy(key, type->key_type);
         }
-        p = ovsdb_token_parse(p + 1, &value_string);
-        ovsdb_atom_from_string(value, type->value_type, value_string);
-        free(value_string);
     }
-    return p;
+    return error;
 }
 
 static void
@@ -845,13 +865,15 @@ free_key_value(const struct ovsdb_type *type,
  * acceptable to ovsdb_atom_from_string().  Optionally, a set may be enclosed
  * in "[]" or a map in "{}"; for an empty set or map these punctuators are
  * required. */
-void
+char *
 ovsdb_datum_from_string(struct ovsdb_datum *datum,
                         const struct ovsdb_type *type, const char *s)
 {
     bool is_map = ovsdb_type_is_map(type);
+    struct ovsdb_error *dberror;
     const char *p;
     int end_delim;
+    char *error;
 
     ovsdb_datum_init_empty(datum);
 
@@ -862,9 +884,9 @@ ovsdb_datum_from_string(struct ovsdb_datum *datum,
         p = skip_spaces(p + 1);
     } else if (!*p) {
         if (is_map) {
-            ovs_fatal(0, "use \"{}\" to specify the empty map");
+            return xstrdup("use \"{}\" to specify the empty map");
         } else {
-            ovs_fatal(0, "use \"[]\" to specify the empty set");
+            return xstrdup("use \"[]\" to specify the empty set");
         }
     } else {
         end_delim = 0;
@@ -874,12 +896,16 @@ ovsdb_datum_from_string(struct ovsdb_datum *datum,
         union ovsdb_atom key, value;
 
         if (ovsdb_token_is_delim(*p)) {
-            ovs_fatal(0, "%s: unexpected \"%c\" parsing %s",
-                      s, *p, ovsdb_type_to_english(type));
+            error = xasprintf("%s: unexpected \"%c\" parsing %s",
+                              s, *p, ovsdb_type_to_english(type));
+            goto error;
         }
 
         /* Add to datum. */
-        p = parse_key_value(p, type, &key, &value);
+        error = parse_key_value(&p, type, &key, &value);
+        if (error) {
+            goto error;
+        }
         ovsdb_datum_add_unsafe(datum, &key, &value, type);
         free_key_value(type, &key, &value);
 
@@ -891,34 +917,47 @@ ovsdb_datum_from_string(struct ovsdb_datum *datum,
     }
 
     if (*p != end_delim) {
-        ovs_fatal(0, "%s: missing \"%c\" at end of data", s, end_delim);
+        error = xasprintf("%s: missing \"%c\" at end of data", s, end_delim);
+        goto error;
     }
     if (end_delim) {
         p = skip_spaces(p + 1);
         if (*p) {
-            ovs_fatal(0, "%s: trailing garbage after \"%c\"", s, end_delim);
+            error = xasprintf("%s: trailing garbage after \"%c\"",
+                              s, end_delim);
+            goto error;
         }
     }
 
     if (datum->n < type->n_min) {
-        ovs_fatal(0, "%s: %u %s were specified but at least %u are required",
-                  s, datum->n,
-                  type->value_type == OVSDB_TYPE_VOID ? "values" : "pairs",
-                  type->n_min);
+        error = xasprintf("%s: %u %s specified but the minimum number is %u",
+                          s, datum->n, is_map ? "pair(s)" : "value(s)",
+                          type->n_min);
+        goto error;
     } else if (datum->n > type->n_max) {
-        ovs_fatal(0, "%s: %u %s were specified but at most %u are allowed",
-                  s, datum->n,
-                  type->value_type == OVSDB_TYPE_VOID ? "values" : "pairs",
-                  type->n_max);
+        error = xasprintf("%s: %u %s specified but the maximum number is %u",
+                          s, datum->n, is_map ? "pair(s)" : "value(s)",
+            type->n_max);
+        goto error;
     }
 
-    if (ovsdb_datum_sort(datum, type)) {
+    dberror = ovsdb_datum_sort(datum, type);
+    if (dberror) {
+        ovsdb_error_destroy(dberror);
         if (ovsdb_type_is_map(type)) {
-            ovs_fatal(0, "%s: map contains duplicate key", s);
+            error = xasprintf("%s: map contains duplicate key", s);
         } else {
-            ovs_fatal(0, "%s: set contains duplicate value", s);
+            error = xasprintf("%s: set contains duplicate value", s);
         }
+        goto error;
     }
+
+    return NULL;
+
+error:
+    ovsdb_datum_destroy(datum, type);
+    ovsdb_datum_init_empty(datum);
+    return error;
 }
 
 /* Appends to 'out' the 'datum' (with the given 'type') in a format acceptable
@@ -1292,23 +1331,25 @@ ovsdb_symbol_table_put(struct ovsdb_symbol_table *symtab, const char *name,
  * quotes are retained in the output.  (Backslashes inside double quotes are
  * not removed, either.)
  */
-const char *
-ovsdb_token_parse(const char *s, char **outp)
+char *
+ovsdb_token_parse(const char **s, char **outp)
 {
     const char *p;
     struct ds out;
     bool in_quotes;
+    char *error;
 
     ds_init(&out);
     in_quotes = false;
-    for (p = s; *p != '\0'; ) {
+    for (p = *s; *p != '\0'; ) {
         int c = *p++;
         if (c == '\\') {
             if (in_quotes) {
                 ds_put_char(&out, '\\');
             }
             if (!*p) {
-                ovs_fatal(0, "%s: backslash at end of argument", s);
+                error = xasprintf("%s: backslash at end of argument", *s);
+                goto error;
             }
             ds_put_char(&out, *p++);
         } else if (!in_quotes && ovsdb_token_is_delim(c)) {
@@ -1322,10 +1363,18 @@ ovsdb_token_parse(const char *s, char **outp)
         }
     }
     if (in_quotes) {
-        ovs_fatal(0, "%s: quoted string extends past end of argument", s);
+        error = xasprintf("%s: quoted string extends past end of argument",
+                          *s);
+        goto error;
     }
     *outp = ds_cstr(&out);
-    return p;
+    *s = p;
+    return NULL;
+
+error:
+    ds_destroy(&out);
+    *outp = NULL;
+    return error;
 }
 
 /* Returns true if 'c' delimits tokens, or if 'c' is 0, and false otherwise. */
diff --git a/lib/ovsdb-data.h b/lib/ovsdb-data.h
index 70dfd0f65..0638fa1bc 100644
--- a/lib/ovsdb-data.h
+++ b/lib/ovsdb-data.h
@@ -74,8 +74,9 @@ struct ovsdb_error *ovsdb_atom_from_json(union ovsdb_atom *,
 struct json *ovsdb_atom_to_json(const union ovsdb_atom *,
                                 enum ovsdb_atomic_type);
 
-void ovsdb_atom_from_string(union ovsdb_atom *, enum ovsdb_atomic_type,
-                            const char *);
+char *ovsdb_atom_from_string(union ovsdb_atom *, enum ovsdb_atomic_type,
+                             const char *)
+    WARN_UNUSED_RESULT;
 void ovsdb_atom_to_string(const union ovsdb_atom *, enum ovsdb_atomic_type,
                           struct ds *);
 
@@ -128,8 +129,9 @@ struct ovsdb_error *ovsdb_datum_from_json(struct ovsdb_datum *,
 struct json *ovsdb_datum_to_json(const struct ovsdb_datum *,
                                  const struct ovsdb_type *);
 
-void ovsdb_datum_from_string(struct ovsdb_datum *,
-                             const struct ovsdb_type *, const char *);
+char *ovsdb_datum_from_string(struct ovsdb_datum *,
+                             const struct ovsdb_type *, const char *)
+    WARN_UNUSED_RESULT;
 void ovsdb_datum_to_string(const struct ovsdb_datum *,
                            const struct ovsdb_type *, struct ds *);
 
@@ -204,7 +206,7 @@ void ovsdb_symbol_table_put(struct ovsdb_symbol_table *, const char *name,
  *
  * Used by ovsdb_atom_from_string() and ovsdb_datum_from_string(). */
 
-const char *ovsdb_token_parse(const char *, char **outp);
+char *ovsdb_token_parse(const char **, char **outp) WARN_UNUSED_RESULT;
 bool ovsdb_token_is_delim(unsigned char);
 
 #endif /* ovsdb-data.h */
diff --git a/utilities/ovs-vsctl.c b/utilities/ovs-vsctl.c
index e33d2d1e0..c5864b7ee 100644
--- a/utilities/ovs-vsctl.c
+++ b/utilities/ovs-vsctl.c
@@ -1648,6 +1648,14 @@ static const struct vsctl_table_class tables[] = {
     {NULL, NULL, {{NULL, NULL, NULL}, {NULL, NULL, NULL}}}
 };
 
+static void
+die_if_error(char *error)
+{
+    if (error) {
+        ovs_fatal(0, "%s", error);
+    }
+}
+
 static int
 to_lower_and_underscores(unsigned c)
 {
@@ -1794,8 +1802,9 @@ get_row(struct vsctl_context *ctx,
     return row;
 }
 
-static const struct vsctl_column *
-get_column(const struct vsctl_table_class *table, const char *column_name)
+static char *
+get_column(const struct vsctl_table_class *table, const char *column_name,
+           const struct vsctl_column **columnp)
 {
     const struct vsctl_column *column;
     const struct vsctl_column *best_match = NULL;
@@ -1813,44 +1822,52 @@ get_column(const struct vsctl_table_class *table, const char *column_name)
             }
         }
     }
+
+    *columnp = best_match;
     if (best_match) {
-        return best_match;
+        return NULL;
     } else if (best_score) {
-        ovs_fatal(0, "%s has more than one column whose name matches \"%s\"",
-                  table->class->name, column_name);
+        return xasprintf("%s contains more than one column whose name "
+                         "matches \"%s\"", table->class->name, column_name);
     } else {
-        ovs_fatal(0, "%s does not have a column \"%s\"",
-                  table->class->name, column_name);
+        return xasprintf("%s does not contain a column whose name matches "
+                         "\"%s\"", table->class->name, column_name);
     }
 }
 
-static void
-check_trailer(const char *s, const char *p)
-{
-    if (*p != '\0') {
-        ovs_fatal(0, "%s: trailing garbage in argument at offset %td",
-                  s, p - s);
-    }
-}
-
-static void
+static char * WARN_UNUSED_RESULT
 parse_column_key_value(const char *arg, const struct vsctl_table_class *table,
                        const struct vsctl_column **columnp,
                        char **keyp, char **valuep)
 {
     const char *p = arg;
+    char *error;
 
     assert(columnp || keyp);
+    if (keyp) {
+        *keyp = NULL;
+    }
+    if (valuep) {
+        *valuep = NULL;
+    }
 
     /* Parse column name. */
     if (columnp) {
         char *column_name;
 
-        p = ovsdb_token_parse(arg, &column_name);
+        error = ovsdb_token_parse(&arg, &column_name);
+        if (error) {
+            goto error;
+        }
         if (column_name[0] == '\0') {
-            ovs_fatal(0, "%s: missing column name", arg);
+            free(column_name);
+            error = xasprintf("%s: missing column name", arg);
+            goto error;
+        }
+        error = get_column(table, column_name, columnp);
+        if (error) {
+            goto error;
         }
-        *columnp = get_column(table, column_name);
         free(column_name);
     }
 
@@ -1859,9 +1876,13 @@ parse_column_key_value(const char *arg, const struct vsctl_table_class *table,
         if (columnp) {
             p++;
         } else if (!keyp) {
-            ovs_fatal(0, "%s: key not accepted here", arg);
+            error = xasprintf("%s: key not accepted here", arg);
+            goto error;
+        }
+        error = ovsdb_token_parse(&p, keyp);
+        if (error) {
+            goto error;
         }
-        p = ovsdb_token_parse(p, keyp);
     } else if (keyp) {
         *keyp = NULL;
     }
@@ -1869,16 +1890,35 @@ parse_column_key_value(const char *arg, const struct vsctl_table_class *table,
     /* Parse value string. */
     if (*p == '=') {
         if (!valuep) {
-            ovs_fatal(0, "%s: value not accepted here", arg);
+            error = xasprintf("%s: value not accepted here", arg);
+            goto error;
         }
         *valuep = xstrdup(p + 1);
-        return;
     } else {
         if (valuep) {
             *valuep = NULL;
         }
-        check_trailer(arg, p);
+        if (*p != '\0') {
+            error = xasprintf("%s: trailing garbage in argument at offset %td",
+                              arg, p - arg);
+            goto error;
+        }
+    }
+    return NULL;
+
+error:
+    if (columnp) {
+        *columnp = NULL;
+    }
+    if (keyp) {
+        free(*keyp);
+        *keyp = NULL;
+    }
+    if (valuep) {
+        free(*valuep);
+        *valuep = NULL;
     }
+    return error;
 }
 
 static void
@@ -1898,8 +1938,8 @@ cmd_get(struct vsctl_context *ctx)
         struct ovsdb_datum datum;
         char *key_string;
 
-        parse_column_key_value(ctx->argv[i], table,
-                               &column, &key_string, NULL);
+        die_if_error(parse_column_key_value(ctx->argv[i], table,
+                                            &column, &key_string, NULL));
 
         ovsdb_idl_txn_read(row, column->idl, &datum);
         if (key_string) {
@@ -1911,8 +1951,9 @@ cmd_get(struct vsctl_context *ctx)
                           column->idl->name);
             }
 
-            ovsdb_atom_from_string(&key, column->idl->type.key_type,
-                                   key_string);
+            die_if_error(ovsdb_atom_from_string(&key,
+                                                column->idl->type.key_type,
+                                                key_string));
 
             idx = ovsdb_datum_find_key(&datum, &key,
                                        column->idl->type.key_type);
@@ -1937,8 +1978,8 @@ cmd_get(struct vsctl_context *ctx)
 }
 
 static void
-list_record(const struct vsctl_table_class *table, const struct ovsdb_idl_row *row,
-            struct ds *out)
+list_record(const struct vsctl_table_class *table,
+            const struct ovsdb_idl_row *row, struct ds *out)
 {
     const struct vsctl_column *column;
 
@@ -2092,9 +2133,11 @@ cmd_set(struct vsctl_context *ctx)
     for (i = 3; i < ctx->argc; i++) {
         const struct vsctl_column *column;
         char *key_string, *value_string;
+        char *error;
 
-        parse_column_key_value(ctx->argv[i], table,
-                               &column, &key_string, &value_string);
+        error = parse_column_key_value(ctx->argv[i], table,
+                                       &column, &key_string, &value_string);
+        die_if_error(error);
         if (column->flags & VSCF_READONLY) {
             ovs_fatal(0, "%s: cannot modify read-only column %s in table %s",
                       ctx->argv[i], column->idl->name, table_name);
@@ -2112,10 +2155,12 @@ cmd_set(struct vsctl_context *ctx)
                           column->idl->name);
             }
 
-            ovsdb_atom_from_string(&key, column->idl->type.key_type,
-                                   key_string);
-            ovsdb_atom_from_string(&value, column->idl->type.value_type,
-                                   value_string);
+            die_if_error(ovsdb_atom_from_string(&key,
+                                                column->idl->type.key_type,
+                                                key_string));
+            die_if_error(ovsdb_atom_from_string(&value,
+                                                column->idl->type.value_type,
+                                                value_string));
 
             ovsdb_datum_init_empty(&new);
             ovsdb_datum_add_unsafe(&new, &key, &value, &column->idl->type);
@@ -2128,7 +2173,8 @@ cmd_set(struct vsctl_context *ctx)
         } else {
             struct ovsdb_datum datum;
 
-            ovsdb_datum_from_string(&datum, &column->idl->type, value_string);
+            die_if_error(ovsdb_datum_from_string(&datum, &column->idl->type,
+                                                 value_string));
             check_constraint(&datum, &column->idl->type, column->constraint);
             ovsdb_idl_txn_write(row, column->idl, &datum);
         }
@@ -2153,7 +2199,7 @@ cmd_add(struct vsctl_context *ctx)
 
     table = get_table(table_name);
     row = get_row(ctx, table, record_id);
-    column = get_column(table, column_name);
+    die_if_error(get_column(table, column_name, &column));
     type = &column->idl->type;
     ovsdb_idl_txn_read(row, column->idl, &old);
     for (i = 4; i < ctx->argc; i++) {
@@ -2168,7 +2214,7 @@ cmd_add(struct vsctl_context *ctx)
         add_type = *type;
         add_type.n_min = 1;
         add_type.n_max = UINT_MAX;
-        ovsdb_datum_from_string(&add, &add_type, ctx->argv[i]);
+        die_if_error(ovsdb_datum_from_string(&add, &add_type, ctx->argv[i]));
         ovsdb_datum_union(&old, &add, type, false);
         ovsdb_datum_destroy(&add, type);
     }