[RFC v4 04/15] service: Add function counting number of sessions using a service type

Patrik Flykt patrik.flykt at linux.intel.com
Tue Sep 3 03:15:02 PDT 2013


	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




More information about the connman mailing list