ovs-vsctl: Fix memory leaks.
[openvswitch] / utilities / ovs-vsctl.c
index 46f8c4f1d8bf6a7596a01ef79d070b44c6073363..f8863bfc1238990838b39cc58a5622e6829e79a6 100644 (file)
@@ -45,6 +45,9 @@
 #include "vlog.h"
 #define THIS_MODULE VLM_vsctl
 
+/* vsctl_fatal() also logs the error, so it is preferred in this file. */
+#define ovs_fatal please_use_vsctl_fatal_instead_of_ovs_fatal
+
 struct vsctl_context;
 
 typedef void vsctl_handler_func(struct vsctl_context *);
@@ -87,6 +90,13 @@ static int timeout = 5;
 /* All supported commands. */
 static const struct vsctl_command_syntax all_commands[];
 
+/* The IDL we're using and the current transaction, if any.
+ * This is for use by vsctl_exit() only, to allow it to clean up.
+ * Other code should use its context arguments. */
+static struct ovsdb_idl *the_idl;
+static struct ovsdb_idl_txn *the_idl_txn;
+
+static void vsctl_exit(int status) NO_RETURN;
 static void vsctl_fatal(const char *, ...) PRINTF_FORMAT(1, 2) NO_RETURN;
 static char *default_db(void);
 static void usage(void) NO_RETURN;
@@ -131,7 +141,7 @@ main(int argc, char *argv[])
     /* Do basic command syntax checking. */
 
     /* Now execute the commands. */
-    idl = ovsdb_idl_create(db, &ovsrec_idl_class);
+    idl = the_idl = ovsdb_idl_create(db, &ovsrec_idl_class);
     seqno = ovsdb_idl_get_seqno(idl);
     trials = 0;
     for (;;) {
@@ -216,8 +226,8 @@ parse_options(int argc, char *argv[])
         case 't':
             timeout = strtoul(optarg, NULL, 10);
             if (timeout < 0) {
-                ovs_fatal(0, "value %s on -t or --timeout is invalid",
-                          optarg);
+                vsctl_fatal("value %s on -t or --timeout is invalid",
+                            optarg);
             }
             break;
 
@@ -335,7 +345,25 @@ vsctl_fatal(const char *format, ...)
 
     vlog_set_levels(VLM_vsctl, VLF_CONSOLE, VLL_EMER);
     VLOG_ERR("%s", message);
-    ovs_fatal(0, "%s", message);
+    ovs_error(0, "%s", message);
+    vsctl_exit(EXIT_FAILURE);
+}
+
+/* Frees the current transaction and the underlying IDL and then calls
+ * exit(status).
+ *
+ * Freeing the transaction and the IDL is not strictly necessary, but it makes
+ * for a clean memory leak report from valgrind in the normal case.  That makes
+ * it easier to notice real memory leaks. */
+static void
+vsctl_exit(int status)
+{
+    if (the_idl_txn) {
+        ovsdb_idl_txn_abort(the_idl_txn);
+        ovsdb_idl_txn_destroy(the_idl_txn);
+    }
+    ovsdb_idl_destroy(the_idl);
+    exit(status);
 }
 
 static void
@@ -921,7 +949,7 @@ cmd_br_exists(struct vsctl_context *ctx)
 
     get_info(ctx->ovs, &info);
     if (!find_bridge(&info, ctx->argv[1], false)) {
-        exit(2);
+        vsctl_exit(2);
     }
     free_info(&info);
 }
@@ -1560,7 +1588,6 @@ static const struct vsctl_column open_vswitch_columns[] = {
     {&ovsrec_open_vswitch_col_bridges, VSCF_READONLY, NULL},
     {&ovsrec_open_vswitch_col_controller, VSCF_READONLY, NULL},
     {&ovsrec_open_vswitch_col_cur_cfg, VSCF_HIDDEN, NULL},
-    {&ovsrec_open_vswitch_col_management_id, 0, "[0-9a-fA-F]{12}"},
     {&ovsrec_open_vswitch_col_managers, 0, "p?(ssl|tcp|unix):.*"},
     {&ovsrec_open_vswitch_col_next_cfg, VSCF_HIDDEN, NULL},
     {&ovsrec_open_vswitch_col_ssl, VSCF_READONLY, NULL},
@@ -1647,7 +1674,7 @@ static void
 die_if_error(char *error)
 {
     if (error) {
-        ovs_fatal(0, "%s", error);
+        vsctl_fatal("%s", error);
     }
 }
 
@@ -1695,9 +1722,9 @@ get_table(const char *table_name)
     if (best_match) {
         return best_match;
     } else if (best_score) {
-        ovs_fatal(0, "multiple table names match \"%s\"", table_name);
+        vsctl_fatal("multiple table names match \"%s\"", table_name);
     } else {
-        ovs_fatal(0, "unknown table \"%s\"", table_name);
+        vsctl_fatal("unknown table \"%s\"", table_name);
     }
 }
 
@@ -1746,6 +1773,10 @@ get_row_by_id(struct vsctl_context *ctx, const struct vsctl_table_class *table,
             }
             ovsdb_datum_destroy(&name, &id->name_column->type);
         }
+        if (best_score && !referrer) {
+            vsctl_fatal("multiple rows in %s match \"%s\"",
+                        table->class->name, record_id);
+        }
     }
     if (!referrer) {
         return NULL;
@@ -1799,8 +1830,8 @@ must_get_row(struct vsctl_context *ctx,
 {
     const struct ovsdb_idl_row *row = get_row(ctx, table, record_id);
     if (!row) {
-        ovs_fatal(0, "no row \"%s\" in table %s",
-                  record_id, table->class->name);
+        vsctl_fatal("no row \"%s\" in table %s",
+                    record_id, table->class->name);
     }
     return row;
 }
@@ -1868,10 +1899,10 @@ parse_column_key_value(const char *arg, const struct vsctl_table_class *table,
             goto error;
         }
         error = get_column(table, column_name, columnp);
+        free(column_name);
         if (error) {
             goto error;
         }
-        free(column_name);
     }
 
     /* Parse key string. */
@@ -1902,8 +1933,8 @@ parse_column_key_value(const char *arg, const struct vsctl_table_class *table,
             *valuep = NULL;
         }
         if (*p != '\0') {
-            error = xasprintf("%s: trailing garbage in argument at offset %td",
-                              arg, p - arg);
+            error = xasprintf("%s: trailing garbage \"%s\" in argument",
+                              arg, p);
             goto error;
         }
     }
@@ -1927,6 +1958,7 @@ error:
 static void
 cmd_get(struct vsctl_context *ctx)
 {
+    bool if_exists = shash_find(&ctx->options, "--if-exists");
     const char *table_name = ctx->argv[1];
     const char *record_id = ctx->argv[2];
     const struct vsctl_table_class *table;
@@ -1950,8 +1982,8 @@ cmd_get(struct vsctl_context *ctx)
             unsigned int idx;
 
             if (column->idl->type.value_type == OVSDB_TYPE_VOID) {
-                ovs_fatal(0, "cannot specify key to get for non-map column %s",
-                          column->idl->name);
+                vsctl_fatal("cannot specify key to get for non-map column %s",
+                            column->idl->name);
             }
 
             die_if_error(ovsdb_atom_from_string(&key,
@@ -1961,14 +1993,15 @@ cmd_get(struct vsctl_context *ctx)
             idx = ovsdb_datum_find_key(&datum, &key,
                                        column->idl->type.key_type);
             if (idx == UINT_MAX) {
-                ovs_fatal(0, "no key %s in %s record \"%s\" column %s",
-                          key_string, table_name, record_id,
-                          column->idl->name);
-
+                if (!if_exists) {
+                    vsctl_fatal("no key \"%s\" in %s record \"%s\" column %s",
+                                key_string, table->class->name, record_id,
+                                column->idl->name);
+                }
+            } else {
+                ovsdb_atom_to_string(&datum.values[idx],
+                                     column->idl->type.value_type, out);
             }
-            ovsdb_atom_to_string(&datum.values[idx],
-                                 column->idl->type.value_type, out);
-
             ovsdb_atom_destroy(&key, column->idl->type.key_type);
         } else {
             ovsdb_datum_to_string(&datum, &column->idl->type, out);
@@ -2052,14 +2085,14 @@ check_string_constraint(const struct ovsdb_datum *datum,
         size_t length = regerror(retval, &re, NULL, 0);
         char *buffer = xmalloc(length);
         regerror(retval, &re, buffer, length);
-        ovs_fatal(0, "internal error compiling regular expression %s: %s",
-                  regex, buffer);
+        vsctl_fatal("internal error compiling regular expression %s: %s",
+                    regex, buffer);
     }
 
     for (i = 0; i < datum->n; i++) {
         const char *key = datum->keys[i].string;
         if (regexec(&re, key, 0, NULL, 0)) {
-            ovs_fatal(0, "%s is not valid (it does not match %s)", key, regex);
+            vsctl_fatal("%s is not valid (it does not match %s)", key, regex);
         }
     }
     free(regex);
@@ -2095,14 +2128,14 @@ check_integer_constraint(const struct ovsdb_datum *datum,
         int64_t value = datum->keys[i].integer;
         if (value < min || value > max) {
             if (max == INT64_MAX) {
-                ovs_fatal(0, "%"PRId64" is less than the minimum "
-                          "allowed value %"PRId64, value, min);
+                vsctl_fatal("%"PRId64" is less than the minimum "
+                            "allowed value %"PRId64, value, min);
             } else if (min == INT64_MIN) {
-                ovs_fatal(0, "%"PRId64" is greater than the maximum "
-                          "allowed value %"PRId64, value, max);
+                vsctl_fatal("%"PRId64" is greater than the maximum "
+                            "allowed value %"PRId64, value, max);
             } else {
-                ovs_fatal(0, "%"PRId64" is outside the valid range %"PRId64" "
-                          "to %"PRId64" (inclusive)", value, min, max);
+                vsctl_fatal("%"PRId64" is outside the valid range %"PRId64" "
+                            "to %"PRId64" (inclusive)", value, min, max);
             }
         }
     }
@@ -2134,11 +2167,11 @@ set_column(const struct vsctl_table_class *table,
                                    &value_string);
     die_if_error(error);
     if (column->flags & VSCF_READONLY && !force) {
-        ovs_fatal(0, "%s: cannot modify read-only column %s in table %s",
-                  arg, column->idl->name, table->class->name);
+        vsctl_fatal("%s: cannot modify read-only column %s in table %s",
+                    arg, column->idl->name, table->class->name);
     }
     if (!value_string) {
-        ovs_fatal(0, "%s: missing value", arg);
+        vsctl_fatal("%s: missing value", arg);
     }
 
     if (key_string) {
@@ -2146,8 +2179,8 @@ set_column(const struct vsctl_table_class *table,
         struct ovsdb_datum old, new;
 
         if (column->idl->type.value_type == OVSDB_TYPE_VOID) {
-            ovs_fatal(0, "cannot specify key to set for non-map column %s",
-                      column->idl->name);
+            vsctl_fatal("cannot specify key to set for non-map column %s",
+                        column->idl->name);
         }
 
         die_if_error(ovsdb_atom_from_string(&key,
@@ -2160,6 +2193,9 @@ set_column(const struct vsctl_table_class *table,
         ovsdb_datum_init_empty(&new);
         ovsdb_datum_add_unsafe(&new, &key, &value, &column->idl->type);
 
+        ovsdb_atom_destroy(&key, column->idl->type.key_type);
+        ovsdb_atom_destroy(&value, column->idl->type.value_type);
+
         ovsdb_idl_txn_read(row, column->idl, &old);
         ovsdb_datum_union(&old, &new, &column->idl->type, true);
         ovsdb_idl_txn_write(row, column->idl, &old);
@@ -2178,6 +2214,7 @@ set_column(const struct vsctl_table_class *table,
     }
 
     free(key_string);
+    free(value_string);
 }
 
 static void
@@ -2214,17 +2251,17 @@ cmd_add(struct vsctl_context *ctx)
     table = get_table(table_name);
     row = must_get_row(ctx, table, record_id);
     die_if_error(get_column(table, column_name, &column));
+    if (column->flags & VSCF_READONLY && !force) {
+        vsctl_fatal("cannot modify read-only column %s in table %s",
+                    column->idl->name, table->class->name);
+    }
+
     type = &column->idl->type;
     ovsdb_idl_txn_read(row, column->idl, &old);
     for (i = 4; i < ctx->argc; i++) {
         struct ovsdb_type add_type;
         struct ovsdb_datum add;
 
-        if (column->flags & VSCF_READONLY && !force) {
-            ovs_fatal(0, "%s: cannot modify read-only column %s in table %s",
-                      ctx->argv[i], column->idl->name, table_name);
-        }
-
         add_type = *type;
         add_type.n_min = 1;
         add_type.n_max = UINT_MAX;
@@ -2233,11 +2270,11 @@ cmd_add(struct vsctl_context *ctx)
         ovsdb_datum_destroy(&add, type);
     }
     if (old.n > type->n_max) {
-        ovs_fatal(0, "\"add\" operation would put %u %s in column %s of "
-                  "table %s but at most %u are allowed",
-                  old.n,
-                  type->value_type == OVSDB_TYPE_VOID ? "values" : "pairs",
-                  column->idl->name, table_name, type->n_max);
+        vsctl_fatal("\"add\" operation would put %u %s in column %s of "
+                    "table %s but the maximum number is %u",
+                    old.n,
+                    type->value_type == OVSDB_TYPE_VOID ? "values" : "pairs",
+                    column->idl->name, table->class->name, type->n_max);
     }
     ovsdb_idl_txn_write(row, column->idl, &old);
 }
@@ -2259,6 +2296,11 @@ cmd_remove(struct vsctl_context *ctx)
     table = get_table(table_name);
     row = must_get_row(ctx, table, record_id);
     die_if_error(get_column(table, column_name, &column));
+    if (column->flags & VSCF_READONLY && !force) {
+        vsctl_fatal("cannot modify read-only column %s in table %s",
+                    column->idl->name, table->class->name);
+    }
+
     type = &column->idl->type;
     ovsdb_idl_txn_read(row, column->idl, &old);
     for (i = 4; i < ctx->argc; i++) {
@@ -2266,11 +2308,6 @@ cmd_remove(struct vsctl_context *ctx)
         struct ovsdb_datum rm;
         char *error;
 
-        if (column->flags & VSCF_READONLY && !force) {
-            ovs_fatal(0, "%s: cannot modify read-only column %s in table %s",
-                      ctx->argv[i], column->idl->name, table_name);
-        }
-
         rm_type = *type;
         rm_type.n_min = 1;
         rm_type.n_max = UINT_MAX;
@@ -2284,11 +2321,11 @@ cmd_remove(struct vsctl_context *ctx)
         ovsdb_datum_destroy(&rm, &rm_type);
     }
     if (old.n < type->n_min) {
-        ovs_fatal(0, "\"remove\" operation would put %u %s in column %s of "
-                  "table %s but at least %u are required",
-                  old.n,
-                  type->value_type == OVSDB_TYPE_VOID ? "values" : "pairs",
-                  column->idl->name, table_name, type->n_min);
+        vsctl_fatal("\"remove\" operation would put %u %s in column %s of "
+                    "table %s but the minimun number is %u",
+                    old.n,
+                    type->value_type == OVSDB_TYPE_VOID ? "values" : "pairs",
+                    column->idl->name, table->class->name, type->n_min);
     }
     ovsdb_idl_txn_write(row, column->idl, &old);
 }
@@ -2314,12 +2351,12 @@ cmd_clear(struct vsctl_context *ctx)
 
         type = &column->idl->type;
         if (column->flags & VSCF_READONLY && !force) {
-            ovs_fatal(0, "%s: cannot modify read-only column %s in table %s",
-                      ctx->argv[i], column->idl->name, table_name);
+            vsctl_fatal("cannot modify read-only column %s in table %s",
+                        column->idl->name, table->class->name);
         } else if (type->n_min > 0) {
-            ovs_fatal(0, "\"clear\" operation cannot be applied to column %s "
-                      "of table %s, which is not allowed to be empty",
-                      column->idl->name, table_name);
+            vsctl_fatal("\"clear\" operation cannot be applied to column %s "
+                        "of table %s, which is not allowed to be empty",
+                        column->idl->name, table->class->name);
         }
 
         ovsdb_datum_init_empty(&datum);
@@ -2337,7 +2374,7 @@ cmd_create(struct vsctl_context *ctx)
     int i;
 
     if (!force) {
-        ovs_fatal(0, "\"create\" requires --force");
+        vsctl_fatal("\"create\" requires --force");
     }
 
     table = get_table(table_name);
@@ -2382,7 +2419,7 @@ cmd_destroy(struct vsctl_context *ctx)
     int i;
 
     if (!force) {
-        ovs_fatal(0, "\"destroy\" requires --force");
+        vsctl_fatal("\"destroy\" requires --force");
     }
 
     table = get_table(table_name);
@@ -2443,7 +2480,7 @@ do_vsctl(const char *args, struct vsctl_command *commands, size_t n_commands,
     int64_t next_cfg = 0;
     char *comment;
 
-    txn = ovsdb_idl_txn_create(idl);
+    txn = the_idl_txn = ovsdb_idl_txn_create(idl);
     if (dry_run) {
         ovsdb_idl_txn_set_dry_run(txn);
     }
@@ -2492,6 +2529,7 @@ do_vsctl(const char *args, struct vsctl_command *commands, size_t n_commands,
         }
     }
     ovsdb_idl_txn_destroy(txn);
+    the_idl_txn = NULL;
 
     switch (status) {
     case TXN_INCOMPLETE:
@@ -2603,7 +2641,7 @@ static const struct vsctl_command_syntax all_commands[] = {
     {"set-ssl", 3, 3, cmd_set_ssl, NULL, "--bootstrap"},
 
     /* Parameter commands. */
-    {"get", 3, INT_MAX, cmd_get, NULL, ""},
+    {"get", 3, INT_MAX, cmd_get, NULL, "--if-exists"},
     {"list", 1, INT_MAX, cmd_list, NULL, ""},
     {"set", 3, INT_MAX, cmd_set, NULL, "--force"},
     {"add", 4, INT_MAX, cmd_add, NULL, "--force"},