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

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

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

On 06/30/2011 12:22 AM, Denis Kenzior wrote:
> Hi Philippe,
>
> On 06/28/2011 12:16 PM, Philippe Nunes wrote:
>> ---
>>   src/gprs.c  |  211 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>   src/ofono.h |   17 +++++
>>   2 files changed, 228 insertions(+), 0 deletions(-)
>>
>> diff --git a/src/gprs.c b/src/gprs.c
>> index acbfa56..b3e6869 100644
>> --- a/src/gprs.c
>> +++ b/src/gprs.c
>> @@ -59,6 +59,7 @@
>>
>>   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?

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?

Regards,

Philippe.


>>
>>   /* 27.007 Section 7.29 */
>>   enum packet_bearer {
>> @@ -149,6 +150,13 @@ struct pri_context {
>>   	struct ofono_gprs *gprs;
>>   };
>>
>> +struct gprs_private_context {
>> +	unsigned int cid;
>> +	struct ofono_gprs_context *context_driver;
>> +	void *notify;
>
> I suggest you use a union so as to avoid casting all over the place.
>
> e.g.
>
> union {
> 	activate_cb act;
> 	deactivate_cb deact;
> };
>
> or something
>
>> +	void *notify_data;
>> +};
>> +
>>   static void gprs_netreg_update(struct ofono_gprs *gprs);
>>   static void gprs_deactivate_next(struct ofono_gprs *gprs);
>>
>> @@ -346,6 +354,20 @@ static struct pri_context *gprs_context_by_path(struct ofono_gprs *gprs,
>>   	return NULL;
>>   }
>>
>> +static struct pri_context *gprs_get_default_context(struct ofono_gprs *gprs)
>> +{
>> +	GSList *l;
>> +
>> +	for (l = gprs->contexts; l; l = l->next) {
>> +		struct pri_context *ctx = l->data;
>> +
>> +		if (ctx->type == OFONO_GPRS_CONTEXT_TYPE_INTERNET)
>> +			return ctx;
>> +	}
>> +
>> +	return NULL;
>> +}
>> +
>>   static void context_settings_free(struct context_settings *settings)
>>   {
>>   	if (settings->ipv4) {
>> @@ -872,6 +894,8 @@ static void pri_activate_callback(const struct ofono_error *error, void *data)
>>   				dbus_message_new_method_return(ctx->pending));
>>
>>   	if (gc->settings->interface != NULL) {
>> +		DBG("Interface %s", gc->settings->interface);
>> +
>>   		pri_ifupdown(gc->settings->interface, TRUE);
>>
>>   		if (ctx->type == OFONO_GPRS_CONTEXT_TYPE_MMS&&
>> @@ -3012,3 +3036,190 @@ void *ofono_gprs_get_data(struct ofono_gprs *gprs)
>>   {
>>   	return gprs->driver_data;
>>   }
>> +
>> +static struct gprs_private_context *gprs_private_context_by_id(unsigned int id)
>> +{
>> +	GSList *l;
>> +
>> +	for (l = g_private_contexts; l; l = l->next) {
>> +		struct gprs_private_context *ctx = l->data;
>> +
>> +		if (ctx->cid == id)
>> +			return ctx;
>> +	}
>> +
>> +	return NULL;
>> +}
>> +
>> +static void activate_request_callback(const struct ofono_error *error,
>> +					void *data)
>> +{
>> +	struct gprs_private_context *ctx = data;
>> +	struct context_settings *settings = ctx->context_driver->settings;
>> +
>> +	if (error->type != OFONO_ERROR_TYPE_NO_ERROR) {
>> +		DBG("Activating context failed with error: %s",
>> +				telephony_error_to_str(error));
>> +		context_settings_free(settings);
>> +		gprs_cid_release(ctx->context_driver->gprs, ctx->cid);
>> +		ctx->context_driver->inuse = FALSE;
>> +
>> +		if (ctx->notify)
>> +			((__ofono_gprs_activate_context_cb_t) ctx->notify)
>> +				(-ENOSYS, NULL, NULL, ctx->notify_data);
>> +
>> +		g_private_contexts = g_slist_remove(g_private_contexts, ctx);
>> +		return;
>> +	}
>> +
>> +	if (settings->interface != NULL) {
>> +		pri_ifupdown(settings->interface, TRUE);
>> +
>> +		if (settings->ipv4)
>> +			pri_set_ipv4_addr(settings->interface,
>> +							settings->ipv4->ip);
>> +	}
>> +
>> +	if (ctx->notify&&  settings->ipv4)
>> +		((__ofono_gprs_activate_context_cb_t) ctx->notify)
>> +					(0, settings->interface,
>> +						settings->ipv4->ip,
>> +						ctx->notify_data);
>> +}
>> +
>> +int __ofono_gprs_activate_context(struct ofono_gprs *gprs,
>> +				struct ofono_gprs_primary_context *context,
>> +				__ofono_gprs_activate_context_cb_t cb,
>> +				void *data)
>> +{
>> +	struct gprs_private_context *private_ctx;
>> +	struct pri_context *default_ctx = NULL;
>> +	struct ofono_gprs_context *gc = NULL;
>> +	struct idmap *cidmap = gprs->cid_map;
>> +	GSList *l;
>> +
>> +	if (context->apn[0] == '\0' || (context->username[0] == '\0'&&
>> +			context->password[0] == '\0')) {
>> +		/* take the default primary internet context */
>> +		default_ctx = gprs_get_default_context(gprs);
>> +
>> +		if (default_ctx == NULL&&  context->apn[0] == '\0')
>> +			return -ENOENT;
>> +	}
>> +
>> +	if (context->apn[0] != '\0'&&  is_valid_apn(context->apn) == FALSE)
>> +		return EINVAL;
>> +
>> +	if (context->proto != OFONO_GPRS_PROTO_IPV4V6&&
>> +			context->proto != OFONO_GPRS_PROTO_IP)
>> +		return -ENOSYS;
>> +
>> +	if (cidmap == NULL)
>> +		return -ENOSYS;
>> +
>> +	if (context->apn[0] == '\0') {
>> +		strcpy(context->apn, default_ctx->context.apn);
>> +		strcpy(context->username, default_ctx->context.username);
>> +		strcpy(context->password, default_ctx->context.password);
>> +	}
>> +
>> +	private_ctx = g_try_new0(struct gprs_private_context, 1);
>> +	if (private_ctx == NULL)
>> +		return -ENOMEM;
>> +
>> +	private_ctx->cid = gprs_cid_alloc(gprs);
>> +	if (private_ctx->cid == 0) {
>> +		g_free(private_ctx);
>> +		return -EBUSY;
>> +	}
>> +
>> +	context->cid = private_ctx->cid;
>> +
>> +	for (l = gprs->context_drivers; l; l = l->next) {
>> +		gc = l->data;
>> +
>> +		if (gc->inuse == TRUE)
>> +			continue;
>> +
>> +		if (gc->driver == NULL)
>> +			continue;
>> +
>> +		if (gc->driver->activate_primary == NULL ||
>> +				gc->driver->deactivate_primary == NULL)
>> +			continue;
>> +
>> +		if (gc->type != OFONO_GPRS_CONTEXT_TYPE_ANY&&
>> +				gc->type != OFONO_GPRS_CONTEXT_TYPE_INTERNET)
>> +			continue;
>> +
>> +		break;
>> +	}
>> +
>> +	if (gc == NULL) {
>> +		g_free(private_ctx);
>> +		return -EBUSY;
>> +	}
>> +
>> +	gc->inuse = TRUE;
>> +	gc->settings->ipv4 = g_new0(struct ipv4_settings, 1);
>> +	private_ctx->context_driver = gc;
>> +	private_ctx->notify = cb;
>> +	private_ctx->notify_data = data;
>> +
>> +	g_private_contexts = g_slist_append(g_private_contexts, private_ctx);
>> +
>> +	gc->driver->activate_primary(gc, context, activate_request_callback,
>> +					private_ctx);
>> +	return 0;
>> +}
>> +
>> +static void deactivate_request_callback(const struct ofono_error *error,
>> +					void *data)
>> +{
>> +	struct gprs_private_context *ctx = data;
>> +	struct context_settings *settings = ctx->context_driver->settings;
>> +	int err = 0;
>> +
>> +	if (error->type != OFONO_ERROR_TYPE_NO_ERROR) {
>> +		DBG("Deactivating context failed with error: %s",
>> +				telephony_error_to_str(error));
>> +		err = -ENOSYS;
>> +	}
>> +
>> +	pri_set_ipv4_addr(settings->interface, NULL);
>> +	pri_ifupdown(settings->interface, FALSE);
>> +	context_settings_free(settings);
>> +
>> +	DBG("Release Context cid %d", ctx->cid);
>> +
>> +	gprs_cid_release(ctx->context_driver->gprs, ctx->cid);
>> +	ctx->context_driver->inuse = FALSE;
>> +
>> +	if (ctx->notify)
>> +		((__ofono_gprs_deactivate_context_cb_t) ctx->notify)
>> +				(err, ctx->notify_data);
>> +
>> +	g_private_contexts = g_slist_remove(g_private_contexts, ctx);
>> +}
>> +
>> +int __ofono_gprs_deactivate_context(unsigned int id,
>> +					__ofono_gprs_deactivate_context_cb_t cb,
>> +					void *data)
>> +{
>> +	struct gprs_private_context *private_ctx;
>> +	struct ofono_gprs_context *gc;
>> +
>> +	DBG("Deactivate context with cid %d", id);
>> +
>> +	private_ctx = gprs_private_context_by_id(id);
>> +	if (private_ctx == NULL)
>> +		return -EINVAL;
>> +
>> +	private_ctx->notify = cb;
>> +	private_ctx->notify_data = data;
>> +
>> +	gc = private_ctx->context_driver;
>> +	gc->driver->deactivate_primary(gc, private_ctx->cid,
>> +				deactivate_request_callback, private_ctx);
>> +	return 0;
>> +}
>> diff --git a/src/ofono.h b/src/ofono.h
>> index 6524806..6b72816 100644
>> --- a/src/ofono.h
>> +++ b/src/ofono.h
>> @@ -240,6 +240,23 @@ gboolean __ofono_call_settings_is_busy(struct ofono_call_settings *cs);
>>   #include<ofono/phonebook.h>
>>   #include<ofono/gprs.h>
>>   #include<ofono/gprs-context.h>
>> +
>> +typedef void (*__ofono_gprs_activate_context_cb_t)(int error,
>> +						const char *interface,
>> +						const char *ip,
>> +						void *data);
>> +
>> +typedef void (*__ofono_gprs_deactivate_context_cb_t)(int error, void *data);
>> +
>> +int __ofono_gprs_deactivate_context(unsigned int id,
>> +					__ofono_gprs_deactivate_context_cb_t cb,
>> +					void *data);
>> +
>> +int __ofono_gprs_activate_context(struct ofono_gprs *gprs,
>> +				struct ofono_gprs_primary_context *context,
>> +				__ofono_gprs_activate_context_cb_t cb,
>> +				void *data);
>> +
>>   #include<ofono/radio-settings.h>
>>   #include<ofono/audio-settings.h>
>>   #include<ofono/ctm.h>
>
> Regards,
> -Denis
>