Re: [PATCH v3 05/10] service: Store config per session
Hi, On Fri, 2014-02-07 at 14:02 +0100, Daniel Wagner wrote: > The main > objection in v2 was that I had to track all services in session.c via > the notify API. That is not needed in v3 as we live can update the > association directly here. service.c assigns services to sessions in > patch 7 by iterating over the service list. How should session.c know > which services are there if it is not tracking them? The main objection against the notifiers in v2 was the to my understanding unnecessary tracking of all services independent of service state. I.e. service_add and service_remove members of struct connman_notifier should not be needed. Less services need to be tracked if service_state_changed is implemented and only the services in ready and online state are remembered in a service hash in src/session.c. Does this make it any clearer or am I forgetting some detail regarding service handling/iteration that cannot be exported from service.c? Cheers, Patrik ___ connman mailing list connman@connman.net https://lists.connman.net/mailman/listinfo/connman
Re: [PATCH v3 05/10] service: Store config per session
Hi Patrik, On 02/05/2014 12:42 PM, Patrik Flykt wrote: >> static GList *service_list = NULL; >> static GHashTable *service_hash = NULL; >> +static GHashTable *session_hash = NULL; > > This looks a little misplaced. What if session_hash is kept in > src/session.c? The usage introduced in patches 07 and 08 can be > expressed also from src/session.c by using connman_service_get_type() > and introducing a connman_service_get_connect_reason()? v1 had session_hash in service.c, v2 had in it in session.c and in v3 is back in service.c based on our discussion we had on IRC. The main objection in v2 was that I had to track all services in session.c via the notify API. That is not needed in v3 as we live can update the association directly here. service.c assigns services to sessions in patch 7 by iterating over the service list. How should session.c know which services are there if it is not tracking them? cheers, daniel ___ connman mailing list connman@connman.net https://lists.connman.net/mailman/listinfo/connman
Re: [PATCH v3 05/10] service: Store config per session
Hi,
On Tue, 2014-02-04 at 09:02 +0100, Daniel Wagner wrote:
> From: Daniel Wagner
>
> Currently, service has no idea which session needs to be updated
> after the auto connect algorithm has choosen a service. Updated means
> that service will add and remove services to a session. First
> step in this support this, we start storing the bearer list per session
> in service.
>
> Also pass in the complete configuration, because we need later to
> know the ConnectionType or RoamingPolicy information anyway.
> ---
> src/connman.h | 6 +-
> src/service.c | 42 +++---
> src/session.c | 31 +++
> 3 files changed, 55 insertions(+), 24 deletions(-)
>
> diff --git a/src/connman.h b/src/connman.h
> index 87c5835..74ff797 100644
> --- a/src/connman.h
> +++ b/src/connman.h
> @@ -700,11 +700,15 @@ enum connman_service_connect_reason {
> CONNMAN_SERVICE_CONNECT_REASON_SESSION = 3,
> };
>
> +struct connman_session;
> +struct connman_session_config;
> +
> int __connman_service_connect(struct connman_service *service,
> enum connman_service_connect_reason reason);
> int __connman_service_disconnect(struct connman_service *service);
> int __connman_service_disconnect_all(void);
> -void __connman_service_set_active_session(bool enable, GSList *list);
> +void __connman_service_set_active_session(struct connman_session *session,
> + struct connman_session_config *config, bool enable);
> void __connman_service_auto_connect(enum connman_service_connect_reason
> reason);
> bool __connman_service_remove(struct connman_service *service);
> bool __connman_service_is_provider_pending(struct connman_service *service);
> diff --git a/src/service.c b/src/service.c
> index 0f294ee..3d65cae 100644
> --- a/src/service.c
> +++ b/src/service.c
> @@ -43,6 +43,7 @@ static DBusConnection *connection = NULL;
>
> static GList *service_list = NULL;
> static GHashTable *service_hash = NULL;
> +static GHashTable *session_hash = NULL;
This looks a little misplaced. What if session_hash is kept in
src/session.c? The usage introduced in patches 07 and 08 can be
expressed also from src/session.c by using connman_service_get_type()
and introducing a connman_service_get_connect_reason()?
> static GSList *counter_list = NULL;
> static unsigned int autoconnect_timeout = 0;
> static unsigned int vpn_autoconnect_timeout = 0;
> @@ -126,6 +127,10 @@ struct connman_service {
> char *config_entry;
> };
>
> +struct session_info {
> + struct connman_session_config *config;
> +};
> +
> static bool allow_property_changed(struct connman_service *service);
>
> static struct connman_ipconfig *create_ip4config(struct connman_service
> *service,
> @@ -139,6 +144,13 @@ struct find_data {
> struct connman_service *service;
> };
>
> +static void session_free(gpointer data)
> +{
> + struct session_info *info = data;
> +
> + g_free(info);
> +}
> +
> static void compare_path(gpointer value, gpointer user_data)
> {
> struct connman_service *service = value;
> @@ -3471,17 +3483,29 @@ static bool is_ignore(struct connman_service *service)
> static int active_sessions[MAX_CONNMAN_SERVICE_TYPES] = {};
> static int active_count = 0;
>
> -void __connman_service_set_active_session(bool enable, GSList *list)
> +void __connman_service_set_active_session(struct connman_session *session,
> + struct connman_session_config *config, bool enable)
> {
> - if (!list)
> + struct session_info *info;
> + GSList *list;
> +
> + if (!config)
> return;
>
> - if (enable)
> + if (enable) {
> active_count++;
> - else
> +
> + info = g_new0(struct session_info, 1);
> + info->config = config;
> +
> + g_hash_table_replace(session_hash, session, info);
> + } else {
> active_count--;
>
> - while (list != NULL) {
> + g_hash_table_remove(session_hash, session);
> + }
> +
> + for (list = config->allowed_bearers; list; list = list->next) {
> enum connman_service_type type = GPOINTER_TO_INT(list->data);
>
> switch (type) {
> @@ -3502,8 +3526,6 @@ void __connman_service_set_active_session(bool enable,
> GSList *list)
> case CONNMAN_SERVICE_TYPE_GADGET:
> break;
> }
> -
> - list = g_slist_next(list);
> }
>
> DBG("eth %d wifi %d bt %d cellular %d sessions %d",
> @@ -6987,6 +7009,9 @@ int __connman_service_init(void)
> service_hash = g_hash_table_new_full(g_str_hash, g_str_equal,
> NULL, service_free);
>
> + session_hash = g_hash_table_new_full(g_direct_hash, g_direct_equal,
> + NULL, session_free);
> +
> services_notify = g_new0(struct _service
[PATCH v3 05/10] service: Store config per session
From: Daniel Wagner
Currently, service has no idea which session needs to be updated
after the auto connect algorithm has choosen a service. Updated means
that service will add and remove services to a session. First
step in this support this, we start storing the bearer list per session
in service.
Also pass in the complete configuration, because we need later to
know the ConnectionType or RoamingPolicy information anyway.
---
src/connman.h | 6 +-
src/service.c | 42 +++---
src/session.c | 31 +++
3 files changed, 55 insertions(+), 24 deletions(-)
diff --git a/src/connman.h b/src/connman.h
index 87c5835..74ff797 100644
--- a/src/connman.h
+++ b/src/connman.h
@@ -700,11 +700,15 @@ enum connman_service_connect_reason {
CONNMAN_SERVICE_CONNECT_REASON_SESSION = 3,
};
+struct connman_session;
+struct connman_session_config;
+
int __connman_service_connect(struct connman_service *service,
enum connman_service_connect_reason reason);
int __connman_service_disconnect(struct connman_service *service);
int __connman_service_disconnect_all(void);
-void __connman_service_set_active_session(bool enable, GSList *list);
+void __connman_service_set_active_session(struct connman_session *session,
+ struct connman_session_config *config, bool enable);
void __connman_service_auto_connect(enum connman_service_connect_reason
reason);
bool __connman_service_remove(struct connman_service *service);
bool __connman_service_is_provider_pending(struct connman_service *service);
diff --git a/src/service.c b/src/service.c
index 0f294ee..3d65cae 100644
--- a/src/service.c
+++ b/src/service.c
@@ -43,6 +43,7 @@ static DBusConnection *connection = NULL;
static GList *service_list = NULL;
static GHashTable *service_hash = NULL;
+static GHashTable *session_hash = NULL;
static GSList *counter_list = NULL;
static unsigned int autoconnect_timeout = 0;
static unsigned int vpn_autoconnect_timeout = 0;
@@ -126,6 +127,10 @@ struct connman_service {
char *config_entry;
};
+struct session_info {
+ struct connman_session_config *config;
+};
+
static bool allow_property_changed(struct connman_service *service);
static struct connman_ipconfig *create_ip4config(struct connman_service
*service,
@@ -139,6 +144,13 @@ struct find_data {
struct connman_service *service;
};
+static void session_free(gpointer data)
+{
+ struct session_info *info = data;
+
+ g_free(info);
+}
+
static void compare_path(gpointer value, gpointer user_data)
{
struct connman_service *service = value;
@@ -3471,17 +3483,29 @@ static bool is_ignore(struct connman_service *service)
static int active_sessions[MAX_CONNMAN_SERVICE_TYPES] = {};
static int active_count = 0;
-void __connman_service_set_active_session(bool enable, GSList *list)
+void __connman_service_set_active_session(struct connman_session *session,
+ struct connman_session_config *config, bool enable)
{
- if (!list)
+ struct session_info *info;
+ GSList *list;
+
+ if (!config)
return;
- if (enable)
+ if (enable) {
active_count++;
- else
+
+ info = g_new0(struct session_info, 1);
+ info->config = config;
+
+ g_hash_table_replace(session_hash, session, info);
+ } else {
active_count--;
- while (list != NULL) {
+ g_hash_table_remove(session_hash, session);
+ }
+
+ for (list = config->allowed_bearers; list; list = list->next) {
enum connman_service_type type = GPOINTER_TO_INT(list->data);
switch (type) {
@@ -3502,8 +3526,6 @@ void __connman_service_set_active_session(bool enable,
GSList *list)
case CONNMAN_SERVICE_TYPE_GADGET:
break;
}
-
- list = g_slist_next(list);
}
DBG("eth %d wifi %d bt %d cellular %d sessions %d",
@@ -6987,6 +7009,9 @@ int __connman_service_init(void)
service_hash = g_hash_table_new_full(g_str_hash, g_str_equal,
NULL, service_free);
+ session_hash = g_hash_table_new_full(g_direct_hash, g_direct_equal,
+ NULL, session_free);
+
services_notify = g_new0(struct _services_notify, 1);
services_notify->remove = g_hash_table_new_full(g_str_hash,
g_str_equal, g_free, NULL);
@@ -7019,6 +7044,9 @@ void __connman_service_cleanup(void)
g_hash_table_destroy(service_hash);
service_hash = NULL;
+ g_hash_table_destroy(session_hash);
+ session_hash = NULL;
+
g_slist_free(counter_list);
counter_list = NULL;
diff --git a/src/session.c b/src/session.c
index 72ed5c9..7ed5d08 100644
--- a/src/session.
