db-ctl-base: make use of user supplied exit function

The user is required to expose the_idl and the_idl_txn global variables,
so that memory can be cleaned up on fatal errors. This patch changes to
ask user to supply an exit function via ctl_init(). What user needs to
do on exit can now remain private.

Signed-off-by: Andy Zhou <azhou@nicira.com>

diff --git a/lib/db-ctl-base.c b/lib/db-ctl-base.c
index 187bc68..e3c0373 100644 (file)
--- a/lib/db-ctl-base.c
+++ b/lib/db-ctl-base.c
@@ -40,11 +40,25 @@
 
 VLOG_DEFINE_THIS_MODULE(db_ctl_base);
 
-/* The IDL we're using and the current transaction, if any.
- * This is for use by ctl_exit() only, to allow it to clean up.
- * Other code should use its context arguments. */
-struct ovsdb_idl *the_idl;
-struct ovsdb_idl_txn *the_idl_txn;
+/* This array defines the 'show' command output format.  User can check the
+ * definition in utilities/ovs-vsctl.c as reference.
+ *
+ * Particularly, if an element in 'columns[]' represents a reference to
+ * another table, the referred table must also be defined as an entry in
+ * in 'cmd_show_tables[]'.
+ *
+ * The definition must end with an all-NULL entry.  It is initalized once
+ * when ctl_init() is called.
+ *
+ * */
+extern struct cmd_show_table cmd_show_tables[];
+
+/* ctl_exit() is called by ctl_fatal(). User can optionally supply an exit
+ * function ctl_exit_func() via ctl_init. If supplied, this function will
+ * be called by ctl_exit()
+ */
+static void (*ctl_exit_func)(int status) = NULL;
+OVS_NO_RETURN static void ctl_exit(int status);
 
 /* Represents all tables in the schema.  User must define 'tables'
  * in implementation and supply via clt_init().  The definition must end
@@ -1942,11 +1956,9 @@ ctl_fatal(const char *format, ...)
 void
 ctl_exit(int status)
 {
-    if (the_idl_txn) {
-        ovsdb_idl_txn_abort(the_idl_txn);
-        ovsdb_idl_txn_destroy(the_idl_txn);
+    if (ctl_exit_func) {
+        ctl_exit_func(status);
     }
-    ovsdb_idl_destroy(the_idl);
     exit(status);
 }
 
@@ -1992,9 +2004,11 @@ ctl_register_commands(const struct ctl_command_syntax *commands)
 
 /* Registers the 'db_ctl_commands' to 'all_commands'. */
 void
-ctl_init(const struct ctl_table_class tables_[])
+ctl_init(const struct ctl_table_class tables_[],
+         void (*ctl_exit_func_)(int status))
 {
     tables = tables_;
+    ctl_exit_func = ctl_exit_func_;
     ctl_register_commands(db_ctl_commands);
 }
 

diff --git a/lib/db-ctl-base.h b/lib/db-ctl-base.h
index 2a66230..9220ece 100644 (file)
--- a/lib/db-ctl-base.h
+++ b/lib/db-ctl-base.h
@@ -33,8 +33,6 @@ struct table;
  * (structs, commands and functions).  To utilize this module, user must
  * define the following:
  *
- * - the 'the_idl' and 'the_idl_txn'.
- *
  * - the 'cmd_show_tables'.  (See 'struct cmd_show_table' for more info).
  *
  * - the command syntaxes for each command.  (See 'struct ctl_command_syntax'
@@ -48,15 +46,10 @@ struct table;
 /* ctl_fatal() also logs the error, so it is preferred in this file. */
 #define ovs_fatal please_use_ctl_fatal_instead_of_ovs_fatal
 
-/* idl and idl transaction to be used for the *ctl command execution.
- * User must provide them.  */
-extern struct ovsdb_idl *the_idl;
-extern struct ovsdb_idl_txn *the_idl_txn;
-
 struct ctl_table_class;
-void ctl_init(const struct ctl_table_class *tables);
+void ctl_init(const struct ctl_table_class *tables,
+             void (*ctl_exit_func)(int status));
 char *ctl_default_db(void);
-OVS_NO_RETURN void ctl_exit(int status);
 OVS_NO_RETURN void ctl_fatal(const char *, ...) OVS_PRINTF_FORMAT(1, 2);
 
 /* *ctl command syntax structure, to be defined by each command implementation.

diff --git a/utilities/ovs-vsctl.c b/utilities/ovs-vsctl.c
index c71398e..4fb88b1 100644 (file)
--- a/utilities/ovs-vsctl.c
+++ b/utilities/ovs-vsctl.c
@@ -84,6 +84,14 @@ static bool retry;
 static struct table_style table_style = TABLE_STYLE_DEFAULT;
 
 static void vsctl_cmd_init(void);
+
+/* 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;
+OVS_NO_RETURN static void vsctl_exit(int status);
+
 OVS_NO_RETURN static void usage(void);
 static void parse_options(int argc, char *argv[], struct shash *local_options);
 static void run_prerequisites(struct ctl_command[], size_t n_commands,
@@ -1340,7 +1348,7 @@ cmd_br_exists(struct ctl_context *ctx)
 
     vsctl_context_populate_cache(ctx);
     if (!find_bridge(vsctl_ctx, ctx->argv[1], false)) {
-        ctl_exit(2);
+        vsctl_exit(2);
     }
 }
 
@@ -2645,6 +2653,23 @@ try_again:
     free(error);
 }
 
+/* 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);
+}
+
 /*
  * Developers who add new commands to the 'struct ctl_command_syntax' must
  * define the 'arguments' member of the struct.  The following keywords are
@@ -2746,6 +2771,6 @@ static const struct ctl_command_syntax vsctl_commands[] = {
 static void
 vsctl_cmd_init(void)
 {
-    ctl_init(tables);
+    ctl_init(tables, vsctl_exit);
     ctl_register_commands(vsctl_commands);
 }

diff --git a/vtep/vtep-ctl.c b/vtep/vtep-ctl.c
index 8dfd3ad..be8fd56 100644 (file)
--- a/vtep/vtep-ctl.c
+++ b/vtep/vtep-ctl.c
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2009, 2010, 2011, 2012, 2014 Nicira, Inc.
+ * Copyright (c) 2009, 2010, 2011, 2012, 2014, 2015 Nicira, Inc.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -70,6 +70,13 @@ static int timeout;
 /* Format for table output. */
 static struct table_style table_style = TABLE_STYLE_DEFAULT;
 
+/* The IDL we're using and the current transaction, if any.
+ * This is for use by vtep_ctl_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;
+
+OVS_NO_RETURN static void vtep_ctl_exit(int status);
 static void vtep_ctl_cmd_init(void);
 OVS_NO_RETURN static void usage(void);
 static void parse_options(int argc, char *argv[], struct shash *local_options);
@@ -270,6 +277,23 @@ parse_options(int argc, char *argv[], struct shash *local_options)
     free(options);
 }
 
+/* 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
+vtep_ctl_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
 usage(void)
 {
@@ -1189,7 +1213,7 @@ cmd_ps_exists(struct ctl_context *ctx)
 
     vtep_ctl_context_populate_cache(ctx);
     if (!find_pswitch(vtepctl_ctx, ctx->argv[1], false)) {
-        ctl_exit(2);
+        vtep_ctl_exit(2);
     }
 }
 
@@ -1363,7 +1387,7 @@ cmd_ls_exists(struct ctl_context *ctx)
 
     vtep_ctl_context_populate_cache(ctx);
     if (!find_lswitch(vtepctl_ctx, ctx->argv[1], false)) {
-        ctl_exit(2);
+        vtep_ctl_exit(2);
     }
 }
 
@@ -2305,6 +2329,6 @@ static const struct ctl_command_syntax vtep_commands[] = {
 static void
 vtep_ctl_cmd_init(void)
 {
-    ctl_init(tables);
+    ctl_init(tables, vtep_ctl_exit);
     ctl_register_commands(vtep_commands);
 }