On Mon, Jul 13, 2015 at 11:48 PM, Andy Zhou <az...@nicira.com> wrote:
> 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 <az...@nicira.com>
> ---
> lib/db-ctl-base.c | 24 +++++++++++++-----------
> lib/db-ctl-base.h | 11 ++---------
> utilities/ovs-vsctl.c | 29 +++++++++++++++++++++++++++--
> vtep/vtep-ctl.c | 32 ++++++++++++++++++++++++++++----
> 4 files changed, 70 insertions(+), 26 deletions(-)
>
> diff --git a/lib/db-ctl-base.c b/lib/db-ctl-base.c
> index e3ba0c5..a137af6 100644
> --- a/lib/db-ctl-base.c
> +++ b/lib/db-ctl-base.c
> @@ -40,12 +40,6 @@
>
> 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.
> *
> @@ -59,6 +53,14 @@ struct ovsdb_idl_txn *the_idl_txn;
> * */
> static const 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
> * with an all-NULL entry. */
> @@ -1956,11 +1958,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);
> }
>
> @@ -2007,10 +2007,12 @@ 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_[],
> - const struct cmd_show_table cmd_show_tables_[])
> + const struct cmd_show_table cmd_show_tables_[],
> + void (*ctl_exit_func_)(int status))
> {
> tables = tables_;
> cmd_show_tables = cmd_show_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 e750599..034f29d 100644
> --- 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 command syntaxes for each command. (See 'struct
> ctl_command_syntax'
> * for more info) and regiters them using ctl_register_commands().
> *
> @@ -46,17 +44,12 @@ 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;
> struct cmd_show_table;
> void ctl_init(const struct ctl_table_class *tables,
> - const struct cmd_show_table *cmd_show_tables);
> + const struct cmd_show_table *cmd_show_tables,
> + void (*ctl_exit_func)(int status));
>
Minor indentation issue,
Thx so much for making it clean, really like it!
Acked-by: Alex Wang <al...@nicira.com>
> 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 7794106..ccb9270 100644
> --- 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,
> @@ -1343,7 +1351,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);
> }
> }
>
> @@ -2648,6 +2656,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
> @@ -2749,6 +2774,6 @@ static const struct ctl_command_syntax
> vsctl_commands[] = {
> static void
> vsctl_cmd_init(void)
> {
> - ctl_init(tables, cmd_show_tables);
> + ctl_init(tables, cmd_show_tables, vsctl_exit);
> ctl_register_commands(vsctl_commands);
> }
> diff --git a/vtep/vtep-ctl.c b/vtep/vtep-ctl.c
> index d9c4b26..d3e042e 100644
> --- 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)
> {
> @@ -1194,7 +1218,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);
> }
> }
>
> @@ -1368,7 +1392,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);
> }
> }
>
> @@ -2310,6 +2334,6 @@ static const struct ctl_command_syntax
> vtep_commands[] = {
> static void
> vtep_ctl_cmd_init(void)
> {
> - ctl_init(tables, cmd_show_tables);
> + ctl_init(tables, cmd_show_tables, vtep_ctl_exit);
> ctl_register_commands(vtep_commands);
> }
> --
> 1.9.1
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
>
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
