[gprs-provision RFCPATCHv6 0/4] Plugin API for provisioning of GPRS context settings

[gprs-provision RFCPATCHv6 0/4] Plugin API for provisioning of GPRS context settings

[Jukka Saunamaki]

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

Hello

Another patchset about automatic provisioning of GPRS context settings.

This time, provisioning driver API is synchronous, and __ofono_gprs_provision_get_settings includes MCC,MNC and SPN as in parameters. 
Per modem probing and all related complexity was removed, since there was really no need for it. 

It is now the gprs-atom that queries SPN from SIM before calling provisioning. Cases when gprs-atom is unregistered or removed while SPN query is in progress, should be handled gracefully (unlike e.g. in netreg atom...)


--Jukka Saunamäki

Jukka Saunamaki (4):
      gprs-provision: add driver API header
      gprs-provision: add driver API sources
      gprs: add gprs context provisioning
      gprs-provision: add example context provisioning driver

 Makefile.am              |    8 ++-
 examples/provision.c     |   97 ++++++++++++++++++++++++++
 include/gprs-provision.h |   69 ++++++++++++++++++
 src/gprs-provision.c     |  104 ++++++++++++++++++++++++++++
 src/gprs.c               |  172 +++++++++++++++++++++++++++++++++++++++++++---
 5 files changed, 438 insertions(+), 12 deletions(-)
 create mode 100644 examples/provision.c
 create mode 100644 include/gprs-provision.h
 create mode 100644 src/gprs-provision.c

[PATCH 1/4] gprs-provision: add driver API header

[Jukka Saunamaki]

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

---
 Makefile.am              |    3 +-
 include/gprs-provision.h |   69 ++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 71 insertions(+), 1 deletions(-)
 create mode 100644 include/gprs-provision.h

diff --git a/Makefile.am b/Makefile.am
index f941a19..f543135 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -13,7 +13,8 @@ pkginclude_HEADERS = include/log.h include/plugin.h include/history.h \
 			include/radio-settings.h include/stk.h \
 			include/audio-settings.h include/nettime.h \
 			include/ctm.h include/cdma-voicecall.h \
-			include/cdma-sms.h include/sim-auth.h
+			include/cdma-sms.h include/sim-auth.h \
+			include/gprs-provision.h
 
 nodist_pkginclude_HEADERS = include/version.h
 
diff --git a/include/gprs-provision.h b/include/gprs-provision.h
new file mode 100644
index 0000000..cc9f5b5
--- /dev/null
+++ b/include/gprs-provision.h
@@ -0,0 +1,69 @@
+/*
+ *
+ *  oFono - Open Telephony stack for Linux
+ *
+ *  Copyright (C) 2011  Nokia Corporation and/or its subsidiary(-ies).
+ *
+ *  This program is free software; you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License version 2 as
+ *  published by the Free Software Foundation.
+ *
+ *  This program is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *  GNU General Public License for more details.
+ *
+ *  You should have received a copy of the GNU General Public License
+ *  along with this program; if not, write to the Free Software
+ *  Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
+ *
+ */
+
+#ifndef __OFONO_GPRS_PROVISION_H
+#define __OFONO_GPRS_PROVISION_H
+
+#ifdef __cplusplus
+extern "C" {
+#endif
+
+#include "gprs-context.h"
+
+struct ofono_gprs_provision_data {
+	enum ofono_gprs_context_type type;
+	enum ofono_gprs_proto proto;
+	char *name;
+	char *apn;
+	char *username;
+	char *password;
+	char *message_proxy;
+	char *message_center;
+};
+
+struct ofono_gprs_provision_driver {
+	const char *name;
+	int priority;
+	void (*get_settings)(const char *mcc, const char *mnc, const char *spn,
+				struct ofono_gprs_provision_data **settings,
+				int *count);
+};
+
+int ofono_gprs_provision_driver_register(
+			const struct ofono_gprs_provision_driver *driver);
+void ofono_gprs_provision_driver_unregister(
+			const struct ofono_gprs_provision_driver *driver);
+
+/* For gprs */
+void __ofono_gprs_provision_get_settings(const char *mcc, const char *mnc,
+			const char *spn,
+			struct ofono_gprs_provision_data **settings,
+			int *count);
+void provision_settings_free(struct ofono_gprs_provision_data *settings,
+				int count);
+
+
+
+#ifdef __cplusplus
+}
+#endif
+
+#endif /* __OFONO_GPRS_PROVISION_H */
-- 
1.7.1

[PATCH 2/4] gprs-provision: add driver API sources

[Jukka Saunamaki]

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

---
 Makefile.am          |    2 +-
 src/gprs-provision.c |  104 ++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 105 insertions(+), 1 deletions(-)
 create mode 100644 src/gprs-provision.c

diff --git a/Makefile.am b/Makefile.am
index f543135..9c4f383 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -356,7 +356,7 @@ src_ofonod_SOURCES = $(gdbus_sources) $(builtin_sources) src/ofono.ver \
 			src/simfs.c src/simfs.h src/audio-settings.c \
 			src/smsagent.c src/smsagent.h src/ctm.c \
 			src/cdma-voicecall.c src/sim-auth.c \
-			src/message.h src/message.c
+			src/message.h src/message.c src/gprs-provision.c
 
 src_ofonod_LDADD = $(builtin_libadd) @GLIB_LIBS@ @DBUS_LIBS@ @CAPNG_LIBS@ -ldl
 
diff --git a/src/gprs-provision.c b/src/gprs-provision.c
new file mode 100644
index 0000000..75af59b
--- /dev/null
+++ b/src/gprs-provision.c
@@ -0,0 +1,104 @@
+/*
+ *
+ *  oFono - Open Source Telephony
+ *
+ *  Copyright (C) 2011  Nokia Corporation and/or its subsidiary(-ies).
+ *
+ *  This program is free software; you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License version 2 as
+ *  published by the Free Software Foundation.
+ *
+ *  This program is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *  GNU General Public License for more details.
+ *
+ *  You should have received a copy of the GNU General Public License
+ *  along with this program; if not, write to the Free Software
+ *  Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
+ *
+ */
+
+#ifdef HAVE_CONFIG_H
+#include <config.h>
+#endif
+
+#include <stdlib.h>
+#include <glib.h>
+
+#include "ofono.h"
+#include "gprs-provision.h"
+
+static GSList *g_drivers = NULL;
+
+
+void provision_settings_free(struct ofono_gprs_provision_data *settings,
+					int count)
+{
+	int i;
+
+	for (i = 0; i < count; i++) {
+		g_free(settings[i].name);
+		g_free(settings[i].apn);
+		g_free(settings[i].username);
+		g_free(settings[i].password);
+		g_free(settings[i].message_proxy);
+		g_free(settings[i].message_center);
+	}
+
+	g_free(settings);
+}
+
+void __ofono_gprs_provision_get_settings(const char *mcc, const char *mnc,
+			const char *spn,
+			struct ofono_gprs_provision_data **settings,
+			int *count)
+{
+	GSList *d;
+
+	*settings = NULL;
+	*count = 0;
+
+	for (d = g_drivers; d != NULL; d = d->next) {
+		const struct ofono_gprs_provision_driver *driver = d->data;
+
+		DBG("Calling provisioning plugin '%s'", driver->name);
+
+		driver->get_settings(mcc, mnc, spn, settings, count);
+
+		if (*count > 0) {
+			DBG("Plugin '%s' returned %d context settings",
+				driver->name, *count);
+			return;
+		}
+
+		ofono_warn("Provisioning plugin '%s' returned no settings",
+			driver->name);
+	}
+}
+
+static gint compare_priority(gconstpointer a, gconstpointer b)
+{
+	const struct ofono_gprs_provision_driver *plugin1 = a;
+	const struct ofono_gprs_provision_driver *plugin2 = b;
+
+	return plugin2->priority - plugin1->priority;
+}
+
+int ofono_gprs_provision_driver_register(
+			const struct ofono_gprs_provision_driver *driver)
+{
+	DBG("driver: %p name: %s", driver, driver->name);
+
+	g_drivers = g_slist_insert_sorted(g_drivers, (void *) driver,
+						compare_priority);
+	return 0;
+}
+
+void ofono_gprs_provision_driver_unregister(
+			const struct ofono_gprs_provision_driver *driver)
+{
+	DBG("driver: %p name: %s", driver, driver->name);
+
+	g_drivers = g_slist_remove(g_drivers, driver);
+}
-- 
1.7.1

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

[Jukka Saunamaki]

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

---
 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"
+
 #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;
 };
 
 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;
+	}
 }
 
 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);
+}
+
+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 (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;
+		}
+	}
+
+	ofono_gprs_finish_register(gprs);
 }
 
 void ofono_gprs_remove(struct ofono_gprs *gprs)
-- 
1.7.1

[PATCH 4/4] gprs-provision: add example context provisioning driver

[Jukka Saunamaki]

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

---
 Makefile.am          |    3 ++
 examples/provision.c |   97 ++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 100 insertions(+), 0 deletions(-)
 create mode 100644 examples/provision.c

diff --git a/Makefile.am b/Makefile.am
index 9c4f383..c70dcbf 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -329,6 +329,9 @@ builtin_sources += examples/history.c
 
 builtin_modules += example_nettime
 builtin_sources += examples/nettime.c
+
+builtin_modules += example_provision
+builtin_sources += examples/provision.c
 endif
 
 builtin_modules += smart_messaging
diff --git a/examples/provision.c b/examples/provision.c
new file mode 100644
index 0000000..5a6103b
--- /dev/null
+++ b/examples/provision.c
@@ -0,0 +1,97 @@
+/*
+ *
+ *  oFono - Open Source Telephony
+ *
+ *  Copyright (C) 2011  Nokia Corporation and/or its subsidiary(-ies).
+ *
+ *  This program is free software; you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License version 2 as
+ *  published by the Free Software Foundation.
+ *
+ *  This program is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *  GNU General Public License for more details.
+ *
+ *  You should have received a copy of the GNU General Public License
+ *  along with this program; if not, write to the Free Software
+ *  Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
+ *
+ */
+
+#ifdef HAVE_CONFIG_H
+#include <config.h>
+#endif
+
+#include <string.h>
+#include <glib.h>
+
+#define OFONO_API_SUBJECT_TO_CHANGE
+
+#include <ofono/modem.h>
+#include <ofono/gprs-provision.h>
+#include <ofono/types.h>
+
+#include "ofono.h"
+
+static void example_provision_get_settings(const char *mcc, const char *mnc,
+				const char *spn,
+				struct ofono_gprs_provision_data **settings,
+				int *count)
+{
+	ofono_debug("Provisioning...");
+	*count = 0;
+	*settings = NULL;
+
+	ofono_debug("Finding settings for MCC %s, MNC %s, SPN '%s'",
+			mcc, mnc, spn);
+
+	if (strcmp(mcc, "246") != 0 || strcmp(mnc, "81") != 0 ||
+			spn == NULL || strcmp(spn, "oFono") != 0)
+		return;
+
+	ofono_debug("Creating example settings for phonesim");
+
+	*settings = g_try_new0(struct ofono_gprs_provision_data, 2);
+	if (*settings == NULL)
+		return;
+
+	*count = 2;
+
+	/* Internet context settings */
+	(*settings)[0].proto = OFONO_GPRS_PROTO_IP;
+	(*settings)[0].type = OFONO_GPRS_CONTEXT_TYPE_INTERNET;
+	(*settings)[0].name = g_strdup("Phonesim Internet");
+	(*settings)[0].apn = g_strdup("internetapn");
+
+	/* MMS context settings */
+	(*settings)[1].proto = OFONO_GPRS_PROTO_IP;
+	(*settings)[1].type = OFONO_GPRS_CONTEXT_TYPE_MMS;
+	(*settings)[1].name = g_strdup("Phonesim MMS");
+	(*settings)[1].apn = g_strdup("mmsapn");
+	(*settings)[1].username = g_strdup("mmsuser");
+	(*settings)[1].password = g_strdup("mmspass");
+	(*settings)[1].message_proxy = g_strdup("10.11.12.13:8080");
+	(*settings)[1].message_center = g_strdup("http://mms.example.com:8000");
+}
+
+static struct ofono_gprs_provision_driver example_driver = {
+	.name		= "Example GPRS context provisioning",
+	.priority       = OFONO_PLUGIN_PRIORITY_LOW,
+	.get_settings	= example_provision_get_settings,
+};
+
+static int example_provision_init(void)
+{
+	return ofono_gprs_provision_driver_register(&example_driver);
+}
+
+static void example_provision_exit(void)
+{
+	ofono_gprs_provision_driver_unregister(&example_driver);
+}
+
+OFONO_PLUGIN_DEFINE(example_provision, "Example Provisioning Plugin",
+			VERSION, OFONO_PLUGIN_PRIORITY_DEFAULT,
+			example_provision_init,
+			example_provision_exit)
-- 
1.7.1

Re: [gprs-provision RFCPATCHv6 0/4] Plugin API for provisioning of GPRS context settings

[Marcel Holtmann]

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

Hi Jukka,

> Another patchset about automatic provisioning of GPRS context settings.
> 
> This time, provisioning driver API is synchronous, and __ofono_gprs_provision_get_settings includes MCC,MNC and SPN as in parameters. 
> Per modem probing and all related complexity was removed, since there was really no need for it. 
> 
> It is now the gprs-atom that queries SPN from SIM before calling provisioning. Cases when gprs-atom is unregistered or removed while SPN query is in progress, should be handled gracefully (unlike e.g. in netreg atom...)

I think that our only leftover is how we handle the SPN detail.

Denis, what is your preference here? My take would be that we store it
inside network registration and provide it to external modules. I don't
see a point in reading it twice.

Regards

Marcel

Re: [gprs-provision RFCPATCHv6 0/4] Plugin API for provisioning of GPRS context settings

[Jukka Saunamaki]

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

Hi

On Tue, 2011-01-25 at 13:28 +0100, ext Marcel Holtmann wrote:
> I think that our only leftover is how we handle the SPN detail.
> 
> Denis, what is your preference here? My take would be that we store it
> inside network registration and provide it to external modules. I don't
> see a point in reading it twice.

New SPN specific API in SIM atom? Asynchronous with callback, but SIM
would need to read it only once.
Unless there is some simple way to start reading it right after SIM is
ready/unlocked and netreg and gprs are registered only after SPN read
returns?

--Jukka

Re: [PATCH 1/4] gprs-provision: add driver API header

[Denis Kenzior]

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

Hi Jukka,

> +/* For gprs */
> +void __ofono_gprs_provision_get_settings(const char *mcc, const char *mnc,
> +			const char *spn,
> +			struct ofono_gprs_provision_data **settings,
> +			int *count);

The __ofono prefix implies that this is private ofono API.  Things
declared in include/foo.h are public oFono API.  Since this does not
seem to be a good candidate for public consumption, can you move this
down to src/ofono.h?

> +void provision_settings_free(struct ofono_gprs_provision_data *settings,
> +				int count);

I suggest renaming this to __ofono_gprs_provision_free_settings() and
moving this to src/ofono.h.

Regards,
-Denis

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

[Denis Kenzior]

[-- 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

Re: [gprs-provision RFCPATCHv6 0/4] Plugin API for provisioning of GPRS context settings

[Marcel Holtmann]

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

Hi Jukka,

> > I think that our only leftover is how we handle the SPN detail.
> > 
> > Denis, what is your preference here? My take would be that we store it
> > inside network registration and provide it to external modules. I don't
> > see a point in reading it twice.
> 
> New SPN specific API in SIM atom? Asynchronous with callback, but SIM
> would need to read it only once.
> Unless there is some simple way to start reading it right after SIM is
> ready/unlocked and netreg and gprs are registered only after SPN read
> returns?

lets get this merged without support for SPN for now. We can easily add
this later. So please fix Denis' comments and re-submit this without the
SPN change.

Andrew is currently looking into fixing the SIM reading race. Once that
is done we can tackle the SPN part. Feel free to add a TODO item for
adding access to SPN information.

Right now I think we need to do that in the SIM atom, store it, and then
provide it for netreg and other plugins that might want it. However we
might need to discuss this a bit further.

Regards

Marcel

Re: [gprs-provision RFCPATCHv6 0/4] Plugin API for provisioning of GPRS context settings

[Aki Niemi]

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

Hi Marcel,

2011/1/26 Marcel Holtmann <marcel@holtmann.org>:
> lets get this merged without support for SPN for now. We can easily add
> this later. So please fix Denis' comments and re-submit this without the
> SPN change.
>
> Andrew is currently looking into fixing the SIM reading race. Once that
> is done we can tackle the SPN part. Feel free to add a TODO item for
> adding access to SPN information.

By SIM race do you mean an atom getting removed while it has a pending
ofono_sim_read?

> Right now I think we need to do that in the SIM atom, store it, and then
> provide it for netreg and other plugins that might want it. However we
> might need to discuss this a bit further.

I think this is actually easy to fix internally to the sim atom. The
first ofono_sim_read() to EFspn would initiate an async read, and any
call to ofono_sim_read() after the fact would pend on that single read
results, or return immediately with cached results if available.

There is still the SIM race problem, but I would really rather solve
that problem by always having all atoms created and removed in the
same callbacks, without returning to mainloop.
Whether atoms register any D-Bus API in certain modem states is then a
different matter.

But the lifetime of all atoms should be the same, and then the SIM
race is again a local matter to the SIM atom to fix. E.g., cancelling
any pending reads it has on the SIM when it gets removed. Simply
removing the driver should take care of this, in fact (at least
isimodem should handle such a thing gracefully).

So I don't see the point in removing the SPN code from provisioning
right now. It is a necessary part of the solution, so at least we need
to keep the task open as long as SPN is not in.

Cheers,
Aki

Re: [gprs-provision RFCPATCHv6 0/4] Plugin API for provisioning of GPRS context settings

[Marcel Holtmann]

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

Hi Aki,

> > lets get this merged without support for SPN for now. We can easily add
> > this later. So please fix Denis' comments and re-submit this without the
> > SPN change.
> >
> > Andrew is currently looking into fixing the SIM reading race. Once that
> > is done we can tackle the SPN part. Feel free to add a TODO item for
> > adding access to SPN information.
> 
> By SIM race do you mean an atom getting removed while it has a pending
> ofono_sim_read?

exactly that one. See discussion on IRC.

> > Right now I think we need to do that in the SIM atom, store it, and then
> > provide it for netreg and other plugins that might want it. However we
> > might need to discuss this a bit further.
> 
> I think this is actually easy to fix internally to the sim atom. The
> first ofono_sim_read() to EFspn would initiate an async read, and any
> call to ofono_sim_read() after the fact would pend on that single read
> results, or return immediately with cached results if available.

<snip>

I let Andrew to comment on this. He is already looking into this.

> So I don't see the point in removing the SPN code from provisioning
> right now. It is a necessary part of the solution, so at least we need
> to keep the task open as long as SPN is not in.

The SPN is not required by provisioning itself. It is required by the
provisioning plugins. That is why I said we should put a new item in the
TODO list to track it.

Or maybe just storing SPN in the SIM atom with a proper API for plugins
and netreg to access it. However I do wanna decouple this from this set
of patches now. Otherwise we are turning in circles and not getting
anywhere. Same as we decoupled the SIM MCC/MNC getter patches.

Regards

Marcel