Re: [PATCH 3/4] gprs: add gprs context provisioning

[Denis Kenzior <denkenz@gmail.com>]

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

Hi Jukka,

On 01/25/2011 06:15 AM, Jukka Saunamaki wrote:
> ---
>  src/gprs.c |  172 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++----
>  1 files changed, 162 insertions(+), 10 deletions(-)
> 
> diff --git a/src/gprs.c b/src/gprs.c
> index 92d0b1a..3e281e5 100644
> --- a/src/gprs.c
> +++ b/src/gprs.c
> @@ -44,6 +44,11 @@
>  #include "storage.h"
>  #include "idmap.h"
>  
> +#include "sim.h"
> +#include "simutil.h"
> +#include "util.h"
> +#include "gprs-provision.h"
> +

At least gprs-provision and sim.h should not be needed here as they
should be included in ofono.h.

>  #define GPRS_FLAG_ATTACHING 0x1
>  #define GPRS_FLAG_RECHECK 0x2
>  
> @@ -95,6 +100,7 @@ struct ofono_gprs {
>  	const struct ofono_gprs_driver *driver;
>  	void *driver_data;
>  	struct ofono_atom *atom;
> +	struct spn_read_request *reading_spn;

Please remov this for now, we need a general solution to making
ofono_sim_read/write safer.

>  };
>  
>  struct ofono_gprs_context {
> @@ -135,6 +141,12 @@ struct pri_context {
>  	struct ofono_gprs *gprs;
>  };
>  
> +struct spn_read_request {
> +	struct ofono_gprs *gprs;
> +	char mcc[OFONO_MAX_MCC_LENGTH + 1];
> +	char mnc[OFONO_MAX_MNC_LENGTH + 1];
> +};
> +
>  static void gprs_netreg_update(struct ofono_gprs *gprs);
>  static void gprs_deactivate_next(struct ofono_gprs *gprs);
>  
> @@ -2251,10 +2263,15 @@ static void gprs_unregister(struct ofono_atom *atom)
>  		gprs->netreg = NULL;
>  	}
>  
> -	ofono_modem_remove_interface(modem,
> +	if (gprs->reading_spn == NULL) {
> +		ofono_modem_remove_interface(modem,
>  					OFONO_CONNECTION_MANAGER_INTERFACE);
> -	g_dbus_unregister_interface(conn, path,
> +		g_dbus_unregister_interface(conn, path,
>  					OFONO_CONNECTION_MANAGER_INTERFACE);
> +	} else {
> +		gprs->reading_spn->gprs = NULL;
> +		gprs->reading_spn = NULL;
> +	}

Please don't be too smart here, unregister is only called if the atom is
registered in the first place.  See below for more on this.

>  }
>  
>  static void gprs_remove(struct ofono_atom *atom)
> @@ -2279,6 +2296,9 @@ static void gprs_remove(struct ofono_atom *atom)
>  	if (gprs->driver && gprs->driver->remove)
>  		gprs->driver->remove(gprs);
>  
> +	if (gprs->reading_spn != NULL)
> +		gprs->reading_spn->gprs = NULL;
> +
>  	g_free(gprs);
>  }
>  
> @@ -2531,13 +2551,14 @@ remove:
>  		storage_sync(imsi, SETTINGS_STORE, gprs->settings);
>  }
>  
> -void ofono_gprs_register(struct ofono_gprs *gprs)
> +static void ofono_gprs_finish_register(struct ofono_gprs *gprs)
>  {
>  	DBusConnection *conn = ofono_dbus_get_connection();
>  	struct ofono_modem *modem = __ofono_atom_get_modem(gprs->atom);
>  	const char *path = __ofono_atom_get_path(gprs->atom);
> -	struct ofono_atom *netreg_atom;
> -	struct ofono_atom *sim_atom;
> +
> +	if (gprs->contexts == NULL) /* Automatic provisioning failed */
> +		add_context(gprs, NULL, OFONO_GPRS_CONTEXT_TYPE_INTERNET);
>  
>  	if (!g_dbus_register_interface(conn, path,
>  					OFONO_CONNECTION_MANAGER_INTERFACE,
> @@ -2546,24 +2567,134 @@ void ofono_gprs_register(struct ofono_gprs *gprs)
>  		ofono_error("Could not create %s interface",
>  				OFONO_CONNECTION_MANAGER_INTERFACE);
>  
> +		gprs_unregister(gprs->atom);
>  		return;
>  	}
>  
>  	ofono_modem_add_interface(modem,
>  				OFONO_CONNECTION_MANAGER_INTERFACE);
> +}
> +
> +static void provision_context(const struct ofono_gprs_provision_data *ap,
> +				struct ofono_gprs *gprs)
> +{
> +	unsigned int id;
> +	struct pri_context *context = NULL;
> +
> +	/* Sanity check */
> +	if (ap == NULL || ap->name == NULL)
> +		return;
> +
> +	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;
> +
> +	context = pri_context_create(gprs, ap->name, ap->type);
> +	DBG("%s context%d '%s' %s", gprs_context_default_name(ap->type),
> +		id, ap->name, context ? "created" : "creation failed");
> +
> +	if (context == NULL)
> +		return;
> +
> +	context->id = id;
> +
> +	if (ap->username != NULL)
> +		strncpy(context->context.username, ap->username,
> +			OFONO_GPRS_MAX_USERNAME_LENGTH);
> +
> +	if (ap->password != NULL)
> +		strncpy(context->context.password, ap->password,
> +			OFONO_GPRS_MAX_PASSWORD_LENGTH);
> +
> +	if (ap->apn != NULL)
> +		strncpy(context->context.apn, ap->apn,
> +			OFONO_GPRS_MAX_APN_LENGTH);
> +
> +	context->context.proto = ap->proto;
> +
> +	if (ap->type == OFONO_GPRS_CONTEXT_TYPE_MMS &&
> +			ap->message_proxy != NULL)
> +		strncpy(context->message_proxy, ap->message_proxy,
> +			MAX_MESSAGE_PROXY_LENGTH);
> +
> +	if (ap->type == OFONO_GPRS_CONTEXT_TYPE_MMS &&
> +			ap->message_center != NULL)
> +		strncpy(context->message_center, ap->message_center,
> +			MAX_MESSAGE_CENTER_LENGTH);
> +
> +	if (context_dbus_register(context) == TRUE) {
> +		gprs->last_context_id = id;
> +		gprs->contexts = g_slist_append(gprs->contexts, context);
> +		write_context_settings(gprs, context);
> +
> +		if (context->type == OFONO_GPRS_CONTEXT_TYPE_MMS) {
> +			g_key_file_set_string(gprs->settings, context->key,
> +						"MessageProxy",
> +						context->message_proxy);
> +			g_key_file_set_string(gprs->settings, context->key,
> +						"MessageCenter",
> +						context->message_center);
> +		}
> +
> +		storage_sync(gprs->imsi, SETTINGS_STORE, gprs->settings);
> +	}
> +}
> +
> +static void sim_spn_read_cb(int ok, int length, int record,
> +				const unsigned char *data,
> +				int record_length, void *user_data)
> +{
> +	struct spn_read_request *req = user_data;
> +	struct ofono_gprs *gprs = req->gprs;
> +	char *spn = NULL;
> +	struct ofono_gprs_provision_data *settings = NULL;
> +	int count = 0;
> +	int i;
> +
> +	if (gprs == NULL)
> +		goto out;
> +
> +	gprs->reading_spn = NULL;
> +
> +	if (ok)
> +		spn = sim_string_to_utf8(data + 1, length - 1);
> +
> +	__ofono_gprs_provision_get_settings(req->mcc, req->mnc, spn,
> +						&settings, &count);
> +
> +	if (count > 0)
> +		DBG("Provisioning %d contexts", count);
> +
> +	for (i = 0; i < count; i++)
> +		provision_context(&settings[i], gprs);
> +
> +	provision_settings_free(settings, count);
> +	ofono_gprs_finish_register(gprs);
> +out:
> +	g_free(req);
> +}
> +

I'm still not happy that we're reading SPN twice, once here and once in
netreg.

> +void ofono_gprs_register(struct ofono_gprs *gprs)
> +{
> +	struct ofono_modem *modem = __ofono_atom_get_modem(gprs->atom);
> +	struct ofono_atom *sim_atom;
> +	struct ofono_sim *sim = NULL;
> +	struct ofono_atom *netreg_atom;
>  
>  	sim_atom = __ofono_modem_find_atom(modem, OFONO_ATOM_TYPE_SIM);
>  
>  	if (sim_atom) {
> -		struct ofono_sim *sim = __ofono_atom_get_data(sim_atom);
> -		const char *imsi = ofono_sim_get_imsi(sim);
> +		const char *imsi;
>  
> +		sim = __ofono_atom_get_data(sim_atom);
> +		imsi = ofono_sim_get_imsi(sim);
>  		gprs_load_settings(gprs, imsi);
>  	}
>  
> -	if (gprs->contexts == NULL)
> -		add_context(gprs, NULL, OFONO_GPRS_CONTEXT_TYPE_INTERNET);
> -
>  	gprs->netreg_watch = __ofono_modem_add_atom_watch(modem,
>  					OFONO_ATOM_TYPE_NETREG,
>  					netreg_watch, gprs, NULL);
> @@ -2575,6 +2706,27 @@ void ofono_gprs_register(struct ofono_gprs *gprs)
>  				OFONO_ATOM_WATCH_CONDITION_REGISTERED, gprs);
>  
>  	__ofono_atom_register(gprs->atom, gprs_unregister);

If you're going to provision the contexts then don't register the atom
yet.  Registering implies it is fully 'ready' including exposing the
D-Bus interface.

> +
> +	if (gprs->contexts == NULL && sim != NULL) {
> +		/* Start reading spn for provisioning */
> +		struct spn_read_request *req;
> +
> +		DBG("start reading EF-SPN");
> +
> +		req = g_try_new0(struct spn_read_request, 1);
> +		if (req != NULL) {
> +			req->gprs = gprs;
> +			gprs->reading_spn = req;
> +			strcpy(req->mcc, ofono_sim_get_mcc(sim));
> +			strcpy(req->mnc, ofono_sim_get_mnc(sim));
> +			if (ofono_sim_read(sim, SIM_EFSPN_FILEID,
> +					OFONO_SIM_FILE_STRUCTURE_TRANSPARENT,
> +					sim_spn_read_cb, req) >= 0)
> +				return;

Coding style M1

> +		}
> +	}
> +
> +	ofono_gprs_finish_register(gprs);
>  }
>  
>  void ofono_gprs_remove(struct ofono_gprs *gprs)

Regards,
-Denis