Hello Daniel,
Thanks for your comments. We shall work on them and by the time we will also
try to explain you exactly what we are trying to do.
>> ---
>> include/service.h | 1 +
>> src/service.c | 9 ++
>> src/session.c | 228
>> +++++++++++++++++++++++++++++++++++++++++++++++++++--
>> 3 files changed, 231 insertions(+), 7 deletions(-)
>>
>> diff --git a/include/service.h b/include/service.h
>> index 36c8658..d041ead 100644
>> --- a/include/service.h
>> +++ b/include/service.h
>> @@ -94,6 +94,7 @@ void connman_service_unref(struct connman_service
>> *service);
>>
>> enum connman_service_type connman_service_get_type(struct connman_service
>> *service);
>> char *connman_service_get_interface(struct connman_service *service);
>> +void connman_service_packet_stats(struct connman_service *service, unsigned
>> int *packets_transmitted);
>>
>> const char *connman_service_get_domainname(struct connman_service *service);
>> char **connman_service_get_nameservers(struct connman_service *service);
>> diff --git a/src/service.c b/src/service.c
>> index 1b95995..31e17be 100644
>> --- a/src/service.c
>> +++ b/src/service.c
>> @@ -1153,6 +1153,15 @@ static void reset_stats(struct connman_service
>> *service)
>> g_timer_reset(service->stats_roaming.timer);
>> }
>>
>> +void connman_service_packet_stats(struct connman_service *service,
>> + unsigned int *packets_transmitted)
>> +{
>> + struct connman_stats_data *data;
>> + data = &stats_get(service)->data;
>> +
>> + *packets_transmitted = (data->rx_packets + data->tx_packets);
>> +}
>> +
>
>I think you should add the appropriate notify callback and register
>session.c to the callbacks. This would adhere to the style we are using
>to inform about service.c changes.
>
Ok we shall make it as a notification.
>> static struct connman_service *get_default(void)
>> {
>> struct connman_service *service;
>> diff --git a/src/session.c b/src/session.c
>> index 990caea..31aed24 100644
>> --- a/src/session.c
>> +++ b/src/session.c
>> @@ -30,8 +30,15 @@
>>
>> #include "connman.h"
>>
>> +#define MIN_PACKET_THRESHOLD (1) /*min packet count before notify*/
>> +
>> +#define STATS_UPDATE_INTERVAL (1) /*Macro to change update stats*/
>> +
>> +#define MONITOR_INTERVAL (2) /*Macro to check traffic*/
>> +
>> static DBusConnection *connection;
>> static GHashTable *session_hash;
>> +GSList *session_master_list = NULL;
>> static connman_bool_t sessionmode;
>> static struct session_info *ecall_info;
>>
>> @@ -109,6 +116,12 @@ struct bearer_info {
>> enum connman_service_type service_type;
>> };
>>
>> +struct service_session_info {
>> + struct connman_service *service;
>> + GSList *list;
>> + unsigned int packets;
>> +};
>> +
>> static const char *trigger2string(enum connman_session_trigger trigger)
>> {
>> switch (trigger) {
>> @@ -1117,16 +1130,79 @@ static DBusMessage
>> *disconnect_session(DBusConnection *conn,
>> return g_dbus_create_reply(msg, DBUS_TYPE_INVALID);
>> }
>>
>> +static void insert_at_position(GSList **session_list, guint position,
>> + struct connman_session *new_session,
>> + gboolean *appended)
>> +{
>> + struct connman_session *session = NULL;
>> + struct session_info *info = NULL;
>> + struct session_info *new_info = NULL;
>> +
>> + if (session_list == NULL || new_session == NULL) {
>> + DBG("session list and session NULL");
>
>This is not true. Either session_list or session is NULL.
>
Yes that is correct but it is always better to validate the input parameters
(specially pointers) before using them.
>> + return;
>> + }
>> +
>> + if (*session_list == NULL) {
>> + return;
>> + }
>
>You don't need to add the the brackets for a single line statement.
>
Adding brackets in one liners makes it more readable but true its of no use.
>> +
>> + session =
>> + (struct connman_session *)g_slist_nth_data(*session_list,
>> + position);
>> + info = session->info;
>> +
>> + if (session == NULL) {
>> + return;
>> + }
>Also no brackets please.
>
Yes :)
>> +
>> + new_info = new_session->info;
>> +
>> + if (info == NULL || new_info == NULL) {
>> + DBG("info and new_info NULL");
>
>The comment is also not valid.
>
>> + return;
>> + }
>> +
>> + if (new_info->priority != info->priority) {
>> + if (new_info->priority == TRUE) {
>> + *session_list = g_slist_insert(*session_list,
>> + new_session, position);
>> + *appended = TRUE;
>> + }
>> + } else{
>
>A space after the "else"
>
>> + if (g_slist_index(session_master_list, new_session) <
>> + g_slist_index(session_master_list, session)) {
>
>You might use a temporary variable. The intention looks kind of wierd.
>
>> + *session_list = g_slist_insert(*session_list,
>> + new_session, position);
>> + *appended = TRUE;
>> + }
>> + }
>> +}
>> +
>> +static void session_called(gpointer data, gpointer user_data)
>> +{
>> + struct connman_session *session = data;
>> + gboolean *ecall = user_data;
>
>ecall as pointer? GINT_TO_POINTER & friends might help here. Looking at
>the way you are calling session_called() you do pass in only the value
>not the pointer.
>
>> + if (ecall)
>> + session_changed(session, CONNMAN_SESSION_TRIGGER_ECALL);
>> + else
>> + session_changed(session, CONNMAN_SESSION_TRIGGER_SERVICE);
>> +}
>> +
>> static void update_ecall_sessions(struct connman_session *session)
>> {
>> struct session_info *info = session->info;
>> struct connman_session *session_iter;
>> GHashTableIter iter;
>> gpointer key, value;
>> + GSList *session_list = NULL;
>> + gboolean ecall = TRUE;
>>
>> g_hash_table_iter_init(&iter, session_hash);
>>
>> while (g_hash_table_iter_next(&iter, &key, &value) == TRUE) {
>> + gboolean appended = FALSE;
>> + guint pos = 0;
>> session_iter = value;
>>
>> if (session_iter == session)
>> @@ -1135,8 +1211,21 @@ static void update_ecall_sessions(struct
>> connman_session *session)
>> session_iter->info->ecall = info->ecall;
>> session_iter->info_dirty = TRUE;
>>
>> - session_changed(session_iter, CONNMAN_SESSION_TRIGGER_ECALL);
>> + for (pos = 0; pos < g_slist_length(session_list); pos++) {
>> + insert_at_position(&session_list,
>> + pos, session, &appended);
>> + if (appended == TRUE) {
>> + break;
>> + }
>> + }
>> + if (appended == FALSE) {
>> + session_list = g_slist_append(session_list, session);
>> + }
>> }
>> + g_slist_foreach(session_list, (GFunc)session_called, (gpointer *)ecall);
>> +
>> + g_slist_free(session_list);
>> + session_list = NULL;
>> }
>
>Okay, you lost me here. Why do you have to update the session_list here?
>append what? The orignal idea of this function was, that one of the
>session has changed the ecall setting and now we have to update all
>sessions.
>
We wanted to discuss this with you. Do we need to maintain the session priority
list at all the places where the sessions are notified. As you might have seen,
we have implemented delay only when the service state changes to online. So
would it suffice if the list is only maintained there or is it good to notify
everywhere in an ordered way(I guess this is not needed in other places as
there will hardly be any delay between the notifications)? Please suggest.
>> static void update_ecall(struct connman_session *session)
>> @@ -1321,6 +1410,8 @@ static int session_disconnect(struct connman_session
>> *session)
>> deselect_and_disconnect(session,
>> CONNMAN_SESSION_REASON_DISCONNECT);
>>
>> + session_master_list = g_slist_remove(session_master_list, session);
>> +
>> g_hash_table_remove(session_hash, session->session_path);
>>
>> return 0;
>> @@ -1518,7 +1609,7 @@ int __connman_session_create(DBusMessage *msg)
>> }
>>
>> g_hash_table_replace(session_hash, session->session_path, session);
>> -
>> + session_master_list = g_slist_append(session_master_list, session);
>> DBG("add %s", session->session_path);
>>
>> if (g_dbus_register_interface(connection, session->session_path,
>> @@ -1631,12 +1722,15 @@ static void service_add(struct connman_service
>> *service,
>> gpointer key, value;
>> struct connman_session *session;
>> struct service_entry *entry;
>> + GSList *session_list = NULL;
>>
>> DBG("service %p", service);
>>
>> g_hash_table_iter_init(&iter, session_hash);
>>
>> while (g_hash_table_iter_next(&iter, &key, &value) == TRUE) {
>> + gboolean appended = FALSE;
>> + guint pos = 0;
>> session = value;
>>
>> if (service_match(session, service) == FALSE)
>> @@ -1654,9 +1748,22 @@ static void service_add(struct connman_service
>> *service,
>>
>> g_hash_table_replace(session->service_hash, service,
>> iter_service_list);
>> -
>> - session_changed(session, CONNMAN_SESSION_TRIGGER_SERVICE);
>> + for (pos = 0; pos < g_slist_length(session_list); pos++) {
>> + insert_at_position(&session_list,
>> + pos, session, &appended);
>> + if (appended == TRUE) {
>> + break;
>> + }
>> + }
>> + if (appended == FALSE) {
>> + session_list = g_slist_append(session_list, session);
>> + }
>> }
>> +
>> + g_slist_foreach(session_list, (GFunc) session_called, NULL);
>> +
>> + g_slist_free(session_list);
>> + session_list = NULL;
>> }
>
>I would suggest you create for each bearer a ordered list with session
>which accepts this bearer. The ordering would be according the Priorty
>setting. First all session which have the Priority set to true and then
>the once which don't have it set. First you notify the sessions which
>have it set and after the delay you update the rest.
>
>
If we understand it correctly, we have also done the same thing but probably in
a different manner. We felt that maintaining a single session list is easier.
>> static void service_remove(struct connman_service *service)
>> @@ -1666,6 +1773,7 @@ static void service_remove(struct connman_service
>> *service)
>> gpointer key, value;
>> struct connman_session *session;
>> struct session_info *info;
>> + GSList *session_list = NULL;
>>
>> DBG("service %p", service);
>>
>> @@ -1675,6 +1783,8 @@ static void service_remove(struct connman_service
>> *service)
>> GSequenceIter *iter;
>> session = value;
>> info = session->info;
>> + gboolean appended = FALSE;
>> + guint pos = 0;
>>
>> iter = g_hash_table_lookup(session->service_hash, service);
>> if (iter == NULL)
>> @@ -1684,8 +1794,57 @@ static void service_remove(struct connman_service
>> *service)
>>
>> if (info->entry != NULL && info->entry->service == service)
>> info->entry = NULL;
>> - session_changed(session, CONNMAN_SESSION_TRIGGER_SERVICE);
>> +
>> + for (pos = 0; pos < g_slist_length(session_list); pos++) {
>> + insert_at_position(&session_list,
>> + pos, session, &appended);
>> + if (appended == TRUE) {
>> + break;
>> + }
>> + }
>> + if (appended == FALSE) {
>> + session_list = g_slist_append(session_list, session);
>> + }
>> + }
>> +
>> + g_slist_foreach(session_list, (GFunc) session_called, NULL);
>> +
>> + g_slist_free(session_list);
>> + session_list = NULL;
>> +}
>
>Did you really test this? service_remove() should propably remove the
>session from the session_list, no?
>
service_remove should not remove sessions or have any impact on sessions, except
notifying them. Only when a session is destroyed, it should be removed.
>> +static gboolean monitor_traffic_and_notify(gpointer user_data)
>> +{
>> + struct service_session_info *service_session_info =
>> + (struct service_session_info *)user_data;
>> + struct connman_service *service = service_session_info->service;
>> + GSList *list = service_session_info->list;
>> + unsigned int packets_old = service_session_info->packets, packets = 0;
>> + connman_service_packet_stats(service, &packets);
>> + service_session_info->packets = packets;
>> + packets_old = packets - packets_old;
>> +
>> + if (packets_old <= MIN_PACKET_THRESHOLD) {
>> + __connman_rtnl_update_interval_remove(STATS_UPDATE_INTERVAL);
>> + g_slist_foreach(list, (GFunc) session_called, NULL);
>> + g_slist_free(list);
>> + list = NULL;
>> + g_free(service_session_info);
>> + service_session_info = NULL;
>> + return (FALSE);
>> }
>> + return (TRUE);
>
>no brackets for the returns
>
checkpatch.pl showed it as an error without the brackets.
>> +}
>> +
>> +static void monitor_interface_usage(gpointer user_data)
>> +{
>> + struct service_session_info *service_session_info =
>> + (struct service_session_info *)user_data;
>> + __connman_rtnl_update_interval_add(STATS_UPDATE_INTERVAL);
>> + connman_service_packet_stats(service_session_info->service,
>> + &service_session_info->packets);
>> + g_timeout_add_seconds(MONITOR_INTERVAL, monitor_traffic_and_notify,
>> + service_session_info);
>> }
>
>I would say it is time to add the proper iptable rules. Just the other
>day we had the discussion on IdleTimeout on this mailing list. And as it
>turnes out we have kernel support for this.
>
>http://kerneltrap.org/mailarchive/linux-netdev/2010/6/15/6279376/thread
>
>So instead of polling for activity you could wait for the IDLETIMER to
>fire up. Seems much simpler than your approach.
>
Thanks for the link, we will go through it in detail.
>BTW: there is no __connman_rtnl_update_remove() in your patch.
>
We have used it in monitor_traffic_and_notify after notifying the non priority
sessions.
__connman_rtnl_update_interval_remove(STATS_UPDATE_INTERVAL)
>> static void service_state_changed(struct connman_service *service,
>> @@ -1693,14 +1852,25 @@ static void service_state_changed(struct
>> connman_service *service,
>> {
>> GHashTableIter iter;
>> gpointer key, value;
>> + struct service_session_info *service_session_info;
>> + GSList *priority_session_list = NULL;
>> + GSList *non_priority_session_list = NULL;
>>
>> DBG("service %p state %d", service, state);
>>
>> + service_session_info = g_try_new0(struct service_session_info, 1);
>> + if (NULL == service_session_info) {
>> + DBG("Service Session List memory allocation failed");
>> + }
>> +
>
>"if (service_session_info == NULL)"
>
>You are changing style in the middle of your patch. And since the rest
>of the code base always uses the "if (variable == NULL)" pattern, you
>should do it too.
>
>And then, I would expect a return here. Why not? (and again no brackets
>for single line statements)
yes we missed that.
>
>> g_hash_table_iter_init(&iter, session_hash);
>>
>> while (g_hash_table_iter_next(&iter, &key, &value) == TRUE) {
>> struct connman_session *session = value;
>> + struct session_info *info = session->info;
>> GSequenceIter *service_iter;
>> + gboolean appended = FALSE;
>> + guint pos = 0;
>>
>> service_iter = g_hash_table_lookup(session->service_hash, service);
>> if (service_iter != NULL) {
>> @@ -1709,10 +1879,54 @@ static void service_state_changed(struct
>> connman_service *service,
>> entry = g_sequence_get(service_iter);
>> entry->state = state;
>> }
>> + if (info->priority == TRUE) {
>> + for (pos = 0;
>> + pos < g_slist_length(priority_session_list);
>> + pos++) {
>
>check with how we indent such code.
>
>> + insert_at_position(&priority_session_list,
>> + pos, session, &appended);
>> + if (TRUE == appended) {
>> + break;
>> + }
>> + }
>> + if (FALSE == appended) {
>> + priority_session_list =
>> + g_slist_append(priority_session_list, session);
>> + }
>> + } else {
>> + for (pos = 0; pos < g_slist_length(
>> + non_priority_session_list); pos++) {
>> + insert_at_position(&non_priority_session_list,
>> + pos, session, &appended);
>> + if (TRUE == appended) {
>> + break;
>> + }
>> + }
>> + if (FALSE == appended) {
>> + non_priority_session_list =
>> + g_slist_append(non_priority_session_list,
>> + session);
>> + }
>> + }
>> + }
>
>You should propably move the if and else body into a function. Kind of
>hard to read it this way. And the function gets too long anyway :)
>
>> +
>> + g_slist_foreach(priority_session_list, (GFunc) session_called, NULL);
>>
>> - session_changed(session,
>> - CONNMAN_SESSION_TRIGGER_SERVICE);
>> + if ((state == CONNMAN_SERVICE_STATE_ONLINE) &&
>> + (priority_session_list != NULL) &&
>> + (non_priority_session_list != NULL)) {
>> + service_session_info->service = service;
>> + service_session_info->list = non_priority_session_list;
>> +
>> + monitor_interface_usage(service_session_info);
>> + } else {
>> + g_slist_foreach(non_priority_session_list,
>> + (GFunc) session_called, NULL);
>> + g_slist_free(non_priority_session_list);
>> + non_priority_session_list = NULL;
>> }
>> + g_slist_free(priority_session_list);
>> + priority_session_list = NULL;
>> }
>>
>> static void ipconfig_changed(struct connman_service *service,
>
>So what would happen if you have only session marked with priority?
>
yes we have taken care of it. only those sessions will be notified and there
will be no delay included.
>daniel
We shall make the changes and send an updated patch.
Regards,
Somisetty Sreegowri
=====-----=====-----=====
Notice: The information contained in this e-mail
message and/or attachments to it may contain
confidential or privileged information. If you are
not the intended recipient, any dissemination, use,
review, distribution, printing or copying of the
information contained in this e-mail message
and/or attachments to it are strictly prohibited. If
you have received this communication in error,
please notify us by reply e-mail or telephone and
immediately and permanently delete the message
and any attachments. Thank you
_______________________________________________
connman mailing list
[email protected]
http://lists.connman.net/listinfo/connman
