Hi,
On Fri, 2013-11-29 at 09:13 +0200, Tomasz Bursztyka wrote:
> This prevents to load all iptables modules even though it will never be
> used.
> ---
> src/connman.h | 1 +
> src/firewall.c | 21 ++++++++++++++++++++-
> src/session.c | 20 +++++++++++++-------
> 3 files changed, 34 insertions(+), 8 deletions(-)
>
> diff --git a/src/connman.h b/src/connman.h
> index 0e3ec47..5bb11eb 100644
> --- a/src/connman.h
> +++ b/src/connman.h
> @@ -951,6 +951,7 @@ int __connman_firewall_add_rule(struct firewall_context
> *ctx,
> const char *rule_fmt, ...);
> int __connman_firewall_enable(struct firewall_context *ctx);
> int __connman_firewall_disable(struct firewall_context *ctx);
> +bool __connman_firewall_is_up(void);
>
> int __connman_firewall_init(void);
> void __connman_firewall_cleanup(void);
> diff --git a/src/firewall.c b/src/firewall.c
> index e4443ea..90c3d3c 100644
> --- a/src/firewall.c
> +++ b/src/firewall.c
> @@ -57,6 +57,8 @@ struct firewall_context {
>
> static GSList *managed_tables;
>
> +static bool firewall_is_up;
> +
> static int chain_to_index(const char *chain_name)
> {
> if (!g_strcmp0(builtin_chains[NF_IP_PRE_ROUTING], chain_name))
> @@ -341,6 +343,8 @@ int __connman_firewall_enable(struct firewall_context
> *ctx)
> goto err;
> }
>
> + firewall_is_up = true;
> +
> return 0;
>
> err:
> @@ -356,6 +360,11 @@ int __connman_firewall_disable(struct firewall_context
> *ctx)
> return firewall_disable(g_list_last(ctx->rules));
> }
>
> +bool __connman_firewall_is_up(void)
> +{
> + return firewall_is_up;
> +}
> +
> static void iterate_chains_cb(const char *chain_name, void *user_data)
> {
> GSList **chains = user_data;
> @@ -417,7 +426,17 @@ static void flush_table(const char *table_name)
>
> static void flush_all_tables(void)
> {
> - /* Flush the tables ConnMan might have modified */
> + /* Flush the tables ConnMan might have modified
> + * But do so if only ConnMan has done something with
> + * iptables */
> +
> + if (!g_file_test("/proc/net/ip_tables_names",
> + G_FILE_TEST_EXISTS | G_FILE_TEST_IS_REGULAR)) {
> + firewall_is_up = false;
> + return;
> + }
> +
> + firewall_is_up = true;
>
> flush_table("filter");
> flush_table("mangle");
> diff --git a/src/session.c b/src/session.c
> index 3fca6d6..42e3763 100644
> --- a/src/session.c
> +++ b/src/session.c
> @@ -37,7 +37,7 @@ static GHashTable *session_hash;
> static struct connman_session *ecall_session;
> static GSList *policy_list;
> static uint32_t session_mark = 256;
> -static struct firewall_context *global_firewall;
> +static struct firewall_context *global_firewall = NULL;
>
> enum connman_session_trigger {
> CONNMAN_SESSION_TRIGGER_UNKNOWN = 0,
> @@ -281,6 +281,12 @@ static int init_firewall_session(struct connman_session
> *session)
>
> DBG("");
>
> + if (!global_firewall) {
Let's not poke on global_firewall outside of init_firewall(). Let's fix
this in init_firewall() instead, as global_firewall is internal to that
function. init_firewall() has a bug as it does not check if
global_firewall is set or not and sets it unconditionally every time -
and looses memory when doing so.
> + err = init_firewall();
> + if (!err)
Shouldn't this be if '(err < 0)' ?
> + return err;
> + }
> +
> fw = __connman_firewall_create();
> if (!fw)
> return -ENOMEM;
> @@ -2212,10 +2218,6 @@ int __connman_session_init(void)
>
> DBG("");
>
> - err = init_firewall();
> - if (err < 0)
> - return err;
> -
> connection = connman_dbus_get_connection();
> if (!connection)
> return -1;
> @@ -2223,14 +2225,18 @@ int __connman_session_init(void)
> err = connman_notifier_register(&session_notifier);
> if (err < 0) {
> dbus_connection_unref(connection);
> - cleanup_firewall();
> return err;
> }
>
> session_hash = g_hash_table_new_full(g_str_hash, g_str_equal,
> NULL, cleanup_session);
>
> - __connman_nfacct_flush(session_nfacct_flush_cb, NULL);
> + if (__connman_firewall_is_up()) {
> + err = init_firewall();
Why do we need to init firewall here if firewall has been enabled
already?
> + if (err < 0)
> + return err;
> + __connman_nfacct_flush(session_nfacct_flush_cb, NULL);
> + }
>
> return 0;
> }
Cheers,
Patrik
_______________________________________________
connman mailing list
[email protected]
https://lists.connman.net/mailman/listinfo/connman
