Re: [PATCH 01/11] gprs: Add private APIs to activate/deactivate private contexts

[Denis Kenzior <denkenz@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 1716 bytes --]

Hi Philippe,

>>>   static GSList *g_drivers = NULL;
>>>   static GSList *g_context_drivers = NULL;
>>> +static GSList *g_private_contexts = NULL;
>>
>> So this part is quite wrong ;)  What you have done here is to make stk
>> activated contexts shared across all gprs atoms, with potentially
>> duplicate ids.
>>
> 
> The gprs_private_contexts are linked to one specific gprs atom as for
> ofono_gprs_contexts. So, I don't understand your point.
> Also, could we think to have more than one gprs atom? In which condition?
> 

You are using a global variable, such that this list is shared across
all atoms.  Remember, oFono has multiple modem objects, each one having
a gprs atom.  The idmap for each gprs atom is independent, so your
gprs_private_context_by_id implementation will fall flat on its face as
soon as you have multiple STK enabled modems in the system ;)

> The idea of this list was to offer the possibility to active/deactivate
> a private context from any atom, not only STK. Hence, the private
> context is not specifically an STK context.
> If we consider that this is not really useful, I can handle only one
> static _gprs_private_context_ pointer.
> This way, it means also that we don't need to return a cid to the STK atom.
> 
> So, I presume, I should simplify and remove this list?
> 

This thought did come across my mind when I reviewed this patch.
Honestly I don't ever see a case where we would need to activate a
context outside of stk.  And I don't think stk is ever going to activate
multiple contexts...

Certainly this simplification will make the API and implementation a bit
cleaner without having any negative impact.

Regards,
-Denis