Re: [PATCH 2/4] gprs: Add new external APIs to create, activate and remove a PDP context

[Philippe Nunes <philippe.nunes@linux.intel.com>]

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

Hi Denis,

On 05/05/2011 05:42 AM, Denis Kenzior wrote:
> Hi Philippe,
>
> On 05/04/2011 12:26 PM, Philippe Nunes wrote:
>> ---
>>   include/gprs.h |   16 ++++
>>   src/gprs.c     |  234 +++++++++++++++++++++++++++++++++++++++++++++++++++++++-
>>   2 files changed, 246 insertions(+), 4 deletions(-)
>>
>> diff --git a/include/gprs.h b/include/gprs.h
>> index 157a6f9..0bb37f7 100644
>> --- a/include/gprs.h
>> +++ b/include/gprs.h
>> @@ -36,6 +36,10 @@ typedef void (*ofono_gprs_status_cb_t)(const struct ofono_error *error,
>>
>>   typedef void (*ofono_gprs_cb_t)(const struct ofono_error *error, void *data);
>>
>> +typedef void (*ofono_gprs_add_pdp_context_cb_t)(int error, char *interface,
>> +						char *ipv4, char *ipv6,
>> +						void *data);
>> +
>>   struct ofono_gprs_driver {
>>   	const char *name;
>>   	int (*probe)(struct ofono_gprs *gprs, unsigned int vendor,
>> @@ -77,7 +81,19 @@ void ofono_gprs_set_cid_range(struct ofono_gprs *gprs,
>>   				unsigned int min, unsigned int max);
>>   void ofono_gprs_add_context(struct ofono_gprs *gprs,
>>   				struct ofono_gprs_context *gc);
>> +unsigned int ofono_gprs_add_pdp_context(struct ofono_gprs *gprs,
>> +					const char *type,
>> +					const char *protocol,
>> +					const char *apn,
>> +					const char *username,
>> +					const char *password,
>> +					const char *host);
>> +
>> +int ofono_gprs_remove_pdp_context(struct ofono_gprs *gprs, unsigned int id);
>>
>> +int ofono_gprs_activate_pdp_context(struct ofono_gprs *gprs, unsigned int id,
>> +					ofono_gprs_add_pdp_context_cb_t cb,
>> +					void *data);
>
> Why do you want to expose STK activated contexts over D-Bus and try to
> deal with the consequences of this decision?  Having oFono handle them
> completely internally would simplify the design quite a bit, no?

Yes, indeed. I thought that the PDP context created by and for oFono 
should be however visible from outside. But I agree,
STK PDP context should not be handled from outside. So, I will remove 
the D-BUS interface registration for this kind of context.

Should I also consider to not add this context to the gprs->contexts list?

As you can see, I'm using a callback to notify the STK atom when the 
context is activated. Since the context is no more subject to D-BUS 
handling, do you think I could map this callback directly as the 
callback for the gprs_context driver (id 
ofono_stk_activate_pdp_context_cb = ofono_gprs_context_cb_t)?

It implies that I need to setup the interface directly in the STK atom 
but so far, I don't know if you agree with the modifications I did in 
gprs.c regarding how this interface is updated when this is linked to a 
STK PDP context (particularly for the host route).
Perhaps, even if it duplicates some code, you would prefer to see the 
STK atom updating the interface directly?

Regards

Philippe.