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

[Denis Kenzior <denkenz@gmail.com>]

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

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.

>  
>  /* 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