Hi,
On Fri, 2013-08-30 at 16:51 +0200, Daniel Wagner wrote:
> On 08/30/2013 01:00 PM, Patrik Flykt wrote:
> > ---
> > include/service.h | 2 ++
> > src/connman.h | 1 +
> > src/service.c | 46 ++++++++++++++++++++++++++++++++++++++++++++++
> > 3 files changed, 49 insertions(+)
> >
> > diff --git a/include/service.h b/include/service.h
> > index 9706dee..dc9d36d 100644
> > --- a/include/service.h
> > +++ b/include/service.h
> > @@ -45,6 +45,8 @@ enum connman_service_type {
> > CONNMAN_SERVICE_TYPE_VPN = 7,
> > CONNMAN_SERVICE_TYPE_GADGET = 8,
> > };
> > +#define MAX_CONNMAN_SERVICE_TYPES 9
> > +
>
> Not really related to this patch but I was wondering if we should also move
> the
>
> #define MAX_TECHNOLOGIES 10
>
> from src/notifier.c at this point.
Indeed, that should be consolidated also.
> > enum connman_service_security {
> > CONNMAN_SERVICE_SECURITY_UNKNOWN = 0,
> > diff --git a/src/connman.h b/src/connman.h
> > index 38b59dd..80e499c 100644
> > --- a/src/connman.h
> > +++ b/src/connman.h
> > @@ -694,6 +694,7 @@ int __connman_service_indicate_default(struct
> > connman_service *service);
> > int __connman_service_connect(struct connman_service *service);
> > int __connman_service_disconnect(struct connman_service *service);
> > int __connman_service_disconnect_all(void);
> > +void __connman_service_indicate_session(bool add, GSList *list);
>
> Could we agree on some other name? 'indicate' what? Basically it counts
> the active sessions, no?
__connman_service_active_session() ?
> > void __connman_service_auto_connect(void);
> > 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 9bb2f04..7d3d33f 100644
> > --- a/src/service.c
> > +++ b/src/service.c
> > @@ -3400,6 +3400,52 @@ static bool is_ignore(struct connman_service
> > *service)
> > return false;
> > }
> >
> > +static int active_sessions[MAX_CONNMAN_SERVICE_TYPES] = {};
> > +static int active_count = 0;
> > +
> > +void __connman_service_indicate_session(bool add, GSList *list)
>
> We have a lot of functions around with something like 'add' and 'remove' or
> 'enable' and 'disable'. Therefore I would propose to add two functions
> which call this as private function.
Nah, either separate add/remove functions or then one function to set
this correctly, no need to do an interim hop in between.
> > +{
> > + if (!list)
> > + return;
> > +
> > + if (add)
> > + active_count++;
> > + else
> > + active_count--;
>
> active_count is always modified whereas the active_sessions[type]
> might not be updated. What is the purpose of active_count compared
> to active_session? Maybe something for the commit message.
There is a NULL list check above. If there is something non-NULL it must
be a list with a length of at least 1 and thus the active count can be
modified.
> > +
> > + while (list != NULL) {
>
> Any special reason not going for a for loop as we do mostly of the time?
Probably not. Except that every item is supposed to be looked at.
> > + enum connman_service_type type = GPOINTER_TO_INT(list->data);
> > +
> > + switch (type) {
> > + case CONNMAN_SERVICE_TYPE_ETHERNET:
> > + case CONNMAN_SERVICE_TYPE_WIFI:
> > + case CONNMAN_SERVICE_TYPE_BLUETOOTH:
> > + case CONNMAN_SERVICE_TYPE_CELLULAR:
> > + if (add)
> > + active_sessions[type]++;
> > + else
> > + active_sessions[type]--;
> > + break;
> > +
> > + case CONNMAN_SERVICE_TYPE_UNKNOWN:
> > + case CONNMAN_SERVICE_TYPE_SYSTEM:
> > + case CONNMAN_SERVICE_TYPE_GPS:
> > + case CONNMAN_SERVICE_TYPE_VPN:
> > + case CONNMAN_SERVICE_TYPE_GADGET:
> > + break;
> > + }
> > +
> > + list = g_slist_next(list);
> > + }
> > +
> > + DBG("eth %d wifi %d bt %d cellular %d sessions %d",
> > + active_sessions[CONNMAN_SERVICE_TYPE_ETHERNET],
> > + active_sessions[CONNMAN_SERVICE_TYPE_WIFI],
> > + active_sessions[CONNMAN_SERVICE_TYPE_BLUETOOTH],
> > + active_sessions[CONNMAN_SERVICE_TYPE_CELLULAR],
> > + active_count);
> > +}
> > +
> > struct preferred_tech_data {
> > GList *preferred_list;
> > enum connman_service_type type;
> >
Cheers,
Patrik
_______________________________________________
connman mailing list
connman@connman.net
https://lists.connman.net/mailman/listinfo/connman
