Re: [PATCH v4 4/8] gprs: Add private APIs for adding/activating/removing hidden PDP contexts

[Denis Kenzior <denkenz@gmail.com>]

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

Hi Philippe,

On 05/20/2011 11:26 AM, Philippe Nunes wrote:
> ---
>  src/gprs.c  |  239 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  src/ofono.h |   24 ++++++
>  2 files changed, 263 insertions(+), 0 deletions(-)
> 
> diff --git a/src/gprs.c b/src/gprs.c
> index 9657a3e..86d95bc 100644
> --- a/src/gprs.c
> +++ b/src/gprs.c
> @@ -147,6 +147,8 @@ struct pri_context {
>  	struct ofono_gprs_primary_context context;
>  	struct ofono_gprs_context *context_driver;
>  	struct ofono_gprs *gprs;
> +	void *notify;
> +	void *notify_data;
>  };
>  

Modifying pri_context for this does not seem like a good idea, I suggest
you make your own structure since pretty much all the members of this
structure are not relevant to what you're trying to accomplish.

As a general philosophy we prefer clearer code (even if there's more of
it) than trying to complicate the logic of code designed for a
completely different purpose.

>  static void gprs_netreg_update(struct ofono_gprs *gprs);
> @@ -356,6 +358,35 @@ static struct pri_context *gprs_context_by_path(struct ofono_gprs *gprs,
>  	return NULL;
>  }
>  
> +static struct pri_context *gprs_context_by_id(struct ofono_gprs *gprs,
> +						unsigned int id)
> +{
> +	GSList *l;
> +
> +	for (l = gprs->contexts; l; l = l->next) {
> +		struct pri_context *ctx = l->data;
> +
> +		if (ctx->id == id)
> +			return ctx;
> +	}
> +
> +	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) {
> @@ -537,6 +568,9 @@ static void signal_settings(struct pri_context *ctx, const char *prop,
>  	DBusMessageIter iter;
>  	struct context_settings *settings;
>  
> +	if (path == NULL)
> +		return;
> +
>  	signal = dbus_message_new_signal(path,
>  					OFONO_CONNECTION_CONTEXT_INTERFACE,
>  					"PropertyChanged");
> @@ -2968,3 +3002,208 @@ void *ofono_gprs_get_data(struct ofono_gprs *gprs)
>  {
>  	return gprs->driver_data;
>  }
> +
> +unsigned int __ofono_gprs_add_pdp_context(struct ofono_gprs *gprs,
> +				enum ofono_gprs_context_type context_type,
> +				enum ofono_gprs_proto proto,
> +				const char *apn,
> +				const char *username,
> +				const char *password,
> +				const char *host)

What is the purpose of the host parameter? Doesn't STK use IP addresses
for the destination?

> +{

Returning int here would be better:

> +	unsigned int id;
> +	struct pri_context *context;
> +	struct pri_context *default_ctx = NULL;
> +	const char *name;
> +
> +	/* Sanity check */
> +	if (apn && strlen(apn) > OFONO_GPRS_MAX_APN_LENGTH)
> +		return 0;

Then you can return meaningful error here, e.g. -EINVAL

> +
> +	if (username && strlen(username) > OFONO_GPRS_MAX_USERNAME_LENGTH)
> +		return 0;
> +
> +	if (password && strlen(password) > OFONO_GPRS_MAX_PASSWORD_LENGTH)
> +		return 0;
> +
> +	if (apn == NULL || (username == NULL && password == NULL)) {
> +		/* take the default primary internet context */
> +		default_ctx = gprs_get_default_context(gprs);
> +
> +		if (default_ctx == NULL && apn == NULL)
> +			return 0;

Maybe ENOENT here

> +	}
> +
> +	if (apn && is_valid_apn(apn) == FALSE)
> +		return 0;
> +

etc

This will allow you to return meaningful terminal responses in case
things fail.

> +	name = gprs_context_default_name(context_type);
> +	if (name == NULL)
> +		return 0;
> +
> +	if (gprs->last_context_id)
> +		id = idmap_alloc_next(gprs->pid_map, gprs->last_context_id);
> +	else
> +		id = idmap_alloc(gprs->pid_map);
> +
> +	if (id > idmap_get_max(gprs->pid_map))
> +		return 0;
> +
> +	context = pri_context_create(gprs, name, context_type);
> +	if (context == NULL) {
> +		idmap_put(gprs->pid_map, id);
> +		return 0;
> +	}
> +

As mentioned previously, I don't see the point of creating pri_context
structure for this.  So you can rip much of this stuff out.

> +	context->id = id;
> +
> +	if (username != NULL)
> +		strcpy(context->context.username, username);
> +
> +	if (password != NULL)
> +		strcpy(context->context.password, password);
> +
> +	if (username == NULL && password == NULL && default_ctx) {
> +		if (default_ctx->context.username)
> +			strcpy(context->context.username,
> +					default_ctx->context.username);
> +		if (default_ctx->context.password)
> +			strcpy(context->context.password,
> +					default_ctx->context.password);
> +	}
> +
> +	if (apn)
> +		strcpy(context->context.apn, apn);
> +	else if (default_ctx) {
> +		strcpy(context->context.apn, default_ctx->context.apn);
> +		if (default_ctx->context.username)
> +			strcpy(context->context.username,
> +					default_ctx->context.username);
> +		if (default_ctx->context.password)
> +			strcpy(context->context.password,
> +					default_ctx->context.password);
> +	}
> +
> +	context->context.proto = proto;
> +
> +	gprs->last_context_id = id;
> +	gprs->contexts = g_slist_append(gprs->contexts, context);
> +
> +	return id;
> +}
> +
> +static void activate_request_callback(const struct ofono_error *error,
> +					void *data)
> +{
> +	struct pri_context *ctx = data;
> +	struct ofono_gprs_context *gc = ctx->context_driver;
> +
> +	DBG("%p", ctx);
> +
> +	if (error->type != OFONO_ERROR_TYPE_NO_ERROR) {
> +		DBG("Activating context failed with error: %s",
> +				telephony_error_to_str(error));
> +		context_settings_free(ctx->context_driver->settings);
> +		release_context(ctx);
> +
> +		if (ctx->notify)
> +			((__ofono_gprs_add_pdp_context_cb_t) ctx->notify)
> +				(-ENOSYS, NULL, NULL, ctx->notify_data);
> +		return;
> +	}
> +
> +	ctx->active = TRUE;
> +
> +	if (gc->settings->interface != NULL) {
> +		pri_ifupdown(gc->settings->interface, TRUE);
> +
> +		if (ctx->type == OFONO_GPRS_CONTEXT_TYPE_STK &&
> +						gc->settings->ipv4) {
> +			pri_set_ipv4_addr(gc->settings->interface,
> +							gc->settings->ipv4->ip);
> +		}
> +	}
> +
> +	if (ctx->notify && gc->settings->ipv4)
> +		((__ofono_gprs_add_pdp_context_cb_t) ctx->notify)
> +					(0, gc->settings->interface,
> +						gc->settings->ipv4->ip,
> +						ctx->notify_data);
> +}
> +
> +int __ofono_gprs_activate_pdp_context(struct ofono_gprs *gprs, unsigned int id,
> +					__ofono_gprs_add_pdp_context_cb_t cb,
> +					void *data)
> +{
> +	struct pri_context *ctx;
> +	struct ofono_gprs_context *gc;
> +
> +	ctx = gprs_context_by_id(gprs, id);
> +	if (ctx == NULL)
> +		return -EINVAL;
> +
> +	if (ctx->active == TRUE)
> +		return 0;
> +
> +	if (assign_context(ctx) == FALSE)
> +		return -ENOSYS;
> +
> +	gc = ctx->context_driver;
> +
> +	ctx->notify = cb;
> +	ctx->notify_data = data;
> +
> +	gc->driver->activate_primary(gc, &ctx->context,
> +					activate_request_callback, ctx);
> +	return 0;
> +}
> +
> +static void remove_request_callback(const struct ofono_error *error,
> +					void *data)
> +{
> +	struct pri_context *ctx = data;
> +	struct ofono_gprs *gprs = ctx->gprs;
> +	int err = 0;
> +
> +	if (error->type != OFONO_ERROR_TYPE_NO_ERROR) {
> +		DBG("Removing context failed with error: %s",
> +				telephony_error_to_str(error));
> +		err = -ENOSYS;
> +	}
> +
> +	pri_reset_context_settings(ctx);
> +	release_context(ctx);
> +	idmap_put(gprs->pid_map, ctx->id);
> +	gprs->contexts = g_slist_remove(gprs->contexts, ctx);
> +
> +	if (ctx->notify)
> +		((__ofono_gprs_remove_pdp_context_cb_t) ctx->notify)
> +				(err, ctx->notify_data);
> +}
> +
> +int __ofono_gprs_remove_pdp_context(struct ofono_gprs *gprs, unsigned int id,
> +					__ofono_gprs_remove_pdp_context_cb_t cb,
> +					void *data)
> +{
> +	struct pri_context *ctx;
> +
> +	ctx = gprs_context_by_id(gprs, id);
> +	if (ctx == NULL)
> +		return -EINVAL;
> +
> +	if (ctx->active) {
> +		struct ofono_gprs_context *gc = ctx->context_driver;
> +
> +		ctx->notify = cb;
> +		ctx->notify_data = data;
> +
> +		gc->driver->deactivate_primary(gc, ctx->context.cid,
> +					remove_request_callback, ctx);
> +		return -EINPROGRESS;
> +	}
> +
> +	idmap_put(gprs->pid_map, ctx->id);
> +	gprs->contexts = g_slist_remove(gprs->contexts, ctx);
> +
> +	return 0;
> +}
> diff --git a/src/ofono.h b/src/ofono.h
> index 82d7e34..ee5864d 100644
> --- a/src/ofono.h
> +++ b/src/ofono.h
> @@ -239,6 +239,30 @@ 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_add_pdp_context_cb_t)(int error,
> +						const char *interface,
> +						const char *ip,
> +						void *data);
> +
> +typedef void (*__ofono_gprs_remove_pdp_context_cb_t)(int error, void *data);
> +
> +unsigned int __ofono_gprs_add_pdp_context(struct ofono_gprs *gprs,
> +					enum ofono_gprs_context_type type,
> +					enum ofono_gprs_proto proto,
> +					const char *apn,
> +					const char *username,
> +					const char *password,
> +					const char *host);
> +

I don't think this is needed at all.  All STK needs to do is just say
'please activate a context with these params'.  You can pretty much get
away with __ofono_gprs_activate_context and __ofono_gprs_deactivate_context.

> +int __ofono_gprs_remove_pdp_context(struct ofono_gprs *gprs, unsigned int id,
> +					__ofono_gprs_remove_pdp_context_cb_t cb,
> +					void *data);
> +

Are you sure you really need the callback function here?  The spec is a
little fuzzy on when the terminal response is actually sent.

> +int __ofono_gprs_activate_pdp_context(struct ofono_gprs *gprs, unsigned int id,
> +					__ofono_gprs_add_pdp_context_cb_t cb,
> +					void *data);
> +
>  #include <ofono/radio-settings.h>
>  #include <ofono/audio-settings.h>
>  #include <ofono/ctm.h>

Regards,
-Denis