Hi Somisetty,
Sorry for the late response. I was on the LinuxCon and now on a two day
workshop. Not really time to look at emails :)
On 10/21/2011 07:27 AM, Somisetty Sreegowri wrote:
> 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.
Sure thing. Just to make sure, take my suggestion with a grain of salt.
Not all my ideas might work. So if you think they are not working for you
(for example the code gets too complex) then tell me so.
>>> ---
>>> 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.
Maybe if you are using the iptables code (Thomasz just added the match support
to
it, which is needed for IDLETIMER) then the notify call back is propably not
needed
anymore.
So the first thing would be to find out how the iptables should look like for
getting this use case working. Then we can figure how we can implement it in
ConnMan. Thomasz is _the_ expert here :)
>>> 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.
I tend to disagree. If the input paremeter are passed in from some unknown
source (e.g. user input) we clearly have to make sure that all parameters
have to be checked for validity. But here we have our own code, in the middle
of where no external source can modify it. If we add on in all functions
validity checks most functions will consist only of checking code.
My suggestion is to start without having random checks. If there is really
need for it then add one. In this example here, you should make the NULL
test where you allocated the lists and then pass only valid pointers in it.
It is also noteworthy, that glib handles NULL lists gracefully. Adding NULL
checks is duplicating code.
>>> + 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.
We have the follow coding style. Of course there are some unclear corner
cases but this one is quite clear :)
>>> +
>>> + 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.
>From the description of Property I would assume that we only prioritize the
>Online
state update. So you don't have to update the list all the time. If it turns out
that this is not good enough, we can extend it later. Let's start small and see
if
it works as expected. Does this work for your?
>>> 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.
We should also look at the runtime overhead. That means even if the list is
easier
to maintain there is a cost. If the code runs in O(n²) that might not be good
enough.
That was one reason why the hash table approach might work faster.
Let me try to explain it in code:
static GHashTable *bearer_hash;
static void service_add(struct connman_service *service,
const char *name)
{
GSList *list = NULL;
while() {
[...]
if (connman_service_get_type(service) ==
CONNMAN_SERVICE_TYPE_CELLULAR)
list = g_slist_add(list, session);
}
if (list == NULL)
return;
/* sort the list here */
g_hash_table_replace(bearer_hash, service, list);
}
/*
* In populate_service_list() you have to update the hash table too.
* Don't forget to sort the list correctly.
*/
/* Don't forget to update the hash table in service_remove */
static void service_state_changed(struct connman_service *service,
enum connman_service_state state)
{
GSList *session_list;
session_list = g_hash_table_lookup(bearer_hash, service);
if (session_list != NULL) {
/*
* check first if we have to defer, that means there is a
* sesssion which has the Priority flag set to TRUE.
* If this the case we have to defer the update.
* Since the list is ordered and the Priority session are in
* in front we have only too look at the first entry.
*/
struct *session = session_list->data;
if (session->info->entry->priority == TRUE) {
defer_update(session_list); /* here the logic for idle
timeout is started */
return;
}
}
[...]
}
I think this way it should work.
Generally speaking you don't have to update all the time the session list. Only
if the
user changes the Priority (resort the list) or on service_remove/service_add.
In service_state_changed() there should only be a single lookup and the at max
one
list iteration. Of course you have to make sure that all stays consistent
(e.g. no invalid pointers) if the list changes while you have pending actions.
Have a look on Patrik's patch "Fix for BMC#22879".
>>> 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.
Sure. I got it wrong. I see you recreate your list from scratch. I still think
you
make it too complex.
>>> +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.
Okay, in this case checkpatch.pl is wrong. My version of checkpatch.pl
is not complaining. Maybe I am lucky or have an updated version :)
The brackets are not needed for the gboolean return value.
>>> +}
>>> +
>>> +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)
I overlooked that part. Anyway the __connman_rtnl_update_interval_*() will go
away. We want to to poll, instead we want to relay on the kernel to wake
up us on interesting events. Meanwhile the necessary infrastructure code
is there (e.g. iptables code). We just have to find a time to fix it,
then we don't poll at all :)
>>> 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.
cool.
thanks,
daniel
_______________________________________________
connman mailing list
[email protected]
http://lists.connman.net/listinfo/connman
