[PATCHv2 0/4] Unify SPN reading logic

[PATCHv2 0/4] Unify SPN reading logic

[Oleg Zhurakivskyy]

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

Hello,

Please find the changes in order not to duplicate the SPN reading logic in src/grps.c and src/network.c.

Changes from v1:

- SPN reading logic has been moved into the sim atom in order to avoid the asymmetry in the API.
- netreg and gprs atoms are simple consumers.
- Added display condition to the SPN watch callback.

Regards,
Oleg

Oleg Zhurakivskyy (4):
  sim: Minor style fixes
  sim: Add SPN watch capability
  gprs: Use sim SPN watch API
  network: Use sim SPN watch API

 include/sim.h |    8 ++
 src/gprs.c    |  105 ++++++++++++++---------
 src/network.c |  163 +++++-------------------------------
 src/sim.c     |  264 ++++++++++++++++++++++++++++++++++++++++++++++++++++++---
 4 files changed, 345 insertions(+), 195 deletions(-)

-- 
1.7.5.4

[PATCHv2 1/4] sim: Minor style fixes

[Oleg Zhurakivskyy]

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

---
 src/sim.c |   24 ++++++++++++------------
 1 files changed, 12 insertions(+), 12 deletions(-)

diff --git a/src/sim.c b/src/sim.c
index ceead1e..4ef14e3 100644
--- a/src/sim.c
+++ b/src/sim.c
@@ -46,10 +46,6 @@
 #include "simfs.h"
 #include "stkutil.h"
 
-static GSList *g_drivers = NULL;
-
-static void sim_own_numbers_update(struct ofono_sim *sim);
-
 struct ofono_sim {
 	/* Contents of the SIM file system, in rough initialization order */
 	char *iccid;
@@ -140,6 +136,10 @@ static const char *const passwd_name[] = {
 	[OFONO_SIM_PASSWORD_PHCORP_PUK] = "corppuk",
 };
 
+static void sim_own_numbers_update(struct ofono_sim *sim);
+
+static GSList *g_drivers = NULL;
+
 static const char *sim_passwd_name(enum ofono_sim_password_type type)
 {
 	return passwd_name[type];
@@ -1057,21 +1057,21 @@ static DBusMessage *sim_reset_pin(DBusConnection *conn, DBusMessage *msg,
 }
 
 static GDBusMethodTable sim_methods[] = {
-	{ "GetProperties",	"",	"a{sv}",	sim_get_properties	},
+	{ "GetProperties",	"",	"a{sv}",	sim_get_properties },
 	{ "SetProperty",	"sv",	"",		sim_set_property,
-							G_DBUS_METHOD_FLAG_ASYNC },
+						G_DBUS_METHOD_FLAG_ASYNC },
 	{ "ChangePin",		"sss",	"",		sim_change_pin,
-							G_DBUS_METHOD_FLAG_ASYNC },
+						G_DBUS_METHOD_FLAG_ASYNC },
 	{ "EnterPin",		"ss",	"",		sim_enter_pin,
-							G_DBUS_METHOD_FLAG_ASYNC },
+						G_DBUS_METHOD_FLAG_ASYNC },
 	{ "ResetPin",		"sss",	"",		sim_reset_pin,
-							G_DBUS_METHOD_FLAG_ASYNC },
+						G_DBUS_METHOD_FLAG_ASYNC },
 	{ "LockPin",		"ss",	"",		sim_lock_pin,
-							G_DBUS_METHOD_FLAG_ASYNC },
+						G_DBUS_METHOD_FLAG_ASYNC },
 	{ "UnlockPin",		"ss",	"",		sim_unlock_pin,
-							G_DBUS_METHOD_FLAG_ASYNC },
+						G_DBUS_METHOD_FLAG_ASYNC },
 	{ "GetIcon",		"y",	"ay",		sim_get_icon,
-							G_DBUS_METHOD_FLAG_ASYNC },
+						G_DBUS_METHOD_FLAG_ASYNC },
 	{ }
 };
 
-- 
1.7.5.4

[PATCHv2 2/4] sim: Add SPN watch capability

[Oleg Zhurakivskyy]

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

---
 include/sim.h |    8 ++
 src/sim.c     |  242 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 250 insertions(+), 0 deletions(-)

diff --git a/include/sim.h b/include/sim.h
index 6a5f067..b238868 100644
--- a/include/sim.h
+++ b/include/sim.h
@@ -196,6 +196,14 @@ void ofono_sim_remove_state_watch(struct ofono_sim *sim, unsigned int id);
 
 enum ofono_sim_state ofono_sim_get_state(struct ofono_sim *sim);
 
+typedef void (*ofono_sim_spn_cb_t)(const char *spn, const char *dc, void *data);
+
+gboolean ofono_sim_add_spn_watch(struct ofono_sim *sim, unsigned int *id,
+					ofono_sim_spn_cb_t cb, void *data,
+					ofono_destroy_func destroy);
+
+gboolean ofono_sim_remove_spn_watch(struct ofono_sim *sim, unsigned int *id);
+
 void ofono_sim_inserted_notify(struct ofono_sim *sim, ofono_bool_t inserted);
 
 struct ofono_sim_context *ofono_sim_context_create(struct ofono_sim *sim);
diff --git a/src/sim.c b/src/sim.c
index 4ef14e3..93b3655 100644
--- a/src/sim.c
+++ b/src/sim.c
@@ -46,7 +46,11 @@
 #include "simfs.h"
 #include "stkutil.h"
 
+#define SIM_FLAG_READING_SPN	0x1
+
 struct ofono_sim {
+	int flags;
+
 	/* Contents of the SIM file system, in rough initialization order */
 	char *iccid;
 
@@ -91,6 +95,13 @@ struct ofono_sim {
 	enum ofono_sim_state state;
 	struct ofono_watchlist *state_watches;
 
+	char *spn;
+	char *spn_dc;
+	struct ofono_watchlist *spn_watches;
+	unsigned int ef_spn_watch;
+	unsigned int cphs_spn_watch;
+	unsigned int cphs_spn_short_watch;
+
 	struct sim_fs *simfs;
 	struct ofono_sim_context *context;
 	struct ofono_sim_context *early_context;
@@ -2358,6 +2369,234 @@ enum ofono_sim_state ofono_sim_get_state(struct ofono_sim *sim)
 	return sim->state;
 }
 
+static void spn_watch_cb(gpointer data, gpointer user_data)
+{
+	struct ofono_watchlist_item *item = data;
+	struct ofono_sim *sim = user_data;
+
+	if (item->notify)
+		((ofono_sim_spn_cb_t) item->notify)(sim->spn, sim->spn_dc,
+							item->notify_data);
+}
+
+static inline void spn_watches_notify(struct ofono_sim *sim)
+{
+	if (sim->spn_watches->items)
+		g_slist_foreach(sim->spn_watches->items, spn_watch_cb, sim);
+
+	sim->flags &= ~SIM_FLAG_READING_SPN;
+}
+
+static void sim_spn_set(struct ofono_sim *sim, const void *data, int length,
+						const unsigned char *dc)
+{
+	char *spn;
+
+	/*
+	 * TS 31.102 says:
+	 *
+	 * the string shall use:
+	 *
+	 * - either the SMS default 7-bit coded alphabet as defined in
+	 *   TS 23.038 [5] with bit 8 set to 0. The string shall be left
+	 *   justified. Unused bytes shall be set to 'FF'.
+	 *
+	 * - or one of the UCS2 code options defined in the annex of TS
+	 *   31.101 [11].
+	 *
+	 * 31.101 has no such annex though.  51.101 refers to Annex B of
+	 * itself which is not there either.  11.11 contains the same
+	 * paragraph as 51.101 and has an Annex B which we implement.
+	 */
+	spn = sim_string_to_utf8(data, length);
+	if (spn == NULL) {
+		ofono_error("EFspn read successfully, but couldn't parse");
+		goto notify;
+	}
+
+	if (strlen(spn) == 0) {
+		g_free(spn);
+		goto notify;
+	}
+
+	sim->spn = spn;
+
+	if (dc)
+		sim->spn_dc = g_memdup(dc, 1);
+
+notify:
+	spn_watches_notify(sim);
+}
+
+static void sim_cphs_spn_short_read_cb(int ok, int length, int record,
+					const unsigned char *data,
+					int record_length, void *user_data)
+{
+	struct ofono_sim *sim = user_data;
+
+	if (!ok) {
+		spn_watches_notify(sim);
+		return;
+	}
+
+	sim_spn_set(sim, data, length, NULL);
+}
+
+static void sim_cphs_spn_read_cb(int ok, int length, int record,
+					const unsigned char *data,
+					int record_length, void *user_data)
+{
+	struct ofono_sim *sim = user_data;
+
+	if (!ok) {
+		if (__ofono_sim_cphs_service_available(sim,
+						SIM_CPHS_SERVICE_SHORT_SPN))
+			ofono_sim_read(sim->context,
+					SIM_EF_CPHS_SPN_SHORT_FILEID,
+					OFONO_SIM_FILE_STRUCTURE_TRANSPARENT,
+					sim_cphs_spn_short_read_cb, sim);
+		else
+			spn_watches_notify(sim);
+
+		return;
+	}
+
+	sim_spn_set(sim, data, length, NULL);
+}
+
+static void sim_spn_read_cb(int ok, int length, int record,
+				const unsigned char *data,
+				int record_length, void *user_data)
+{
+	struct ofono_sim *sim = user_data;
+
+	if (!ok) {
+		ofono_sim_read(sim->context, SIM_EF_CPHS_SPN_FILEID,
+				OFONO_SIM_FILE_STRUCTURE_TRANSPARENT,
+				sim_cphs_spn_read_cb, sim);
+
+		return;
+	}
+
+	sim_spn_set(sim, data + 1, length - 1, data);
+}
+
+static void sim_spn_changed(int id, void *userdata)
+{
+	struct ofono_sim *sim = userdata;
+
+	if (sim->flags & SIM_FLAG_READING_SPN)
+		return;
+
+	g_free(sim->spn);
+	sim->spn = NULL;
+
+	g_free(sim->spn_dc);
+	sim->spn_dc = NULL;
+
+	sim->flags |= SIM_FLAG_READING_SPN;
+	ofono_sim_read(sim->context, SIM_EFSPN_FILEID,
+			OFONO_SIM_FILE_STRUCTURE_TRANSPARENT,
+			sim_spn_read_cb, sim);
+}
+
+static void sim_spn_init(struct ofono_sim *sim)
+{
+	sim->ef_spn_watch = ofono_sim_add_file_watch(sim->context,
+					SIM_EFSPN_FILEID, sim_spn_changed, sim,
+					NULL);
+
+	sim->cphs_spn_watch = ofono_sim_add_file_watch(sim->context,
+					SIM_EF_CPHS_SPN_FILEID,
+					sim_spn_changed, sim, NULL);
+
+	if (__ofono_sim_cphs_service_available(sim,
+						SIM_CPHS_SERVICE_SHORT_SPN))
+		sim->cphs_spn_short_watch = ofono_sim_add_file_watch(
+				sim->context, SIM_EF_CPHS_SPN_SHORT_FILEID,
+				sim_spn_changed, sim, NULL);
+}
+
+static void sim_spn_close(struct ofono_sim *sim)
+{
+	__ofono_watchlist_free(sim->spn_watches);
+	sim->spn_watches = NULL;
+
+	ofono_sim_remove_file_watch(sim->context, sim->ef_spn_watch);
+	sim->ef_spn_watch = 0;
+
+	ofono_sim_remove_file_watch(sim->context, sim->cphs_spn_watch);
+	sim->cphs_spn_watch = 0;
+
+	if (sim->cphs_spn_short_watch) {
+		ofono_sim_remove_file_watch(sim->context,
+						sim->cphs_spn_short_watch);
+		sim->cphs_spn_short_watch = 0;
+	}
+
+	sim->flags &= ~SIM_FLAG_READING_SPN;
+
+	g_free(sim->spn);
+	sim->spn = NULL;
+
+	g_free(sim->spn_dc);
+	sim->spn_dc = NULL;
+}
+
+gboolean ofono_sim_add_spn_watch(struct ofono_sim *sim, unsigned int *id,
+					ofono_sim_spn_cb_t cb, void *data,
+					ofono_destroy_func destroy)
+{
+	struct ofono_watchlist_item *item;
+	unsigned int watch_id;
+
+	DBG("%p", sim);
+
+	if (sim == NULL)
+		return 0;
+
+	item = g_new0(struct ofono_watchlist_item, 1);
+
+	item->notify = cb;
+	item->destroy = destroy;
+	item->notify_data = data;
+
+	watch_id = __ofono_watchlist_add_item(sim->spn_watches, item);
+	if (watch_id == 0)
+		return FALSE;
+
+	*id = watch_id;
+
+	if (sim->ef_spn_watch == 0) {
+		sim_spn_init(sim);
+		sim_spn_changed(0, sim);
+		return TRUE;
+	}
+
+	if (sim->flags & SIM_FLAG_READING_SPN)
+		return TRUE;
+
+	((ofono_sim_spn_cb_t) item->notify)(sim->spn, sim->spn_dc,
+							item->notify_data);
+	return TRUE;
+}
+
+gboolean ofono_sim_remove_spn_watch(struct ofono_sim *sim, unsigned int *id)
+{
+	gboolean ret;
+
+	DBG("%p", sim);
+
+	if (sim == NULL)
+		return FALSE;
+
+	ret = __ofono_watchlist_remove_item(sim->spn_watches, *id);
+	if (ret == TRUE)
+		*id = 0;
+
+	return ret;
+}
+
 static void sim_pin_query_cb(const struct ofono_error *error,
 				enum ofono_sim_password_type pin_type,
 				void *data)
@@ -2477,6 +2716,8 @@ static void sim_unregister(struct ofono_atom *atom)
 	__ofono_watchlist_free(sim->state_watches);
 	sim->state_watches = NULL;
 
+	sim_spn_close(sim);
+
 	g_dbus_unregister_interface(conn, path, OFONO_SIM_MANAGER_INTERFACE);
 	ofono_modem_remove_interface(modem, OFONO_SIM_MANAGER_INTERFACE);
 }
@@ -2606,6 +2847,7 @@ void ofono_sim_register(struct ofono_sim *sim)
 
 	ofono_modem_add_interface(modem, OFONO_SIM_MANAGER_INTERFACE);
 	sim->state_watches = __ofono_watchlist_new(g_free);
+	sim->spn_watches = __ofono_watchlist_new(g_free);
 	sim->simfs = sim_fs_new(sim, sim->driver);
 
 	__ofono_atom_register(sim->atom, sim_unregister);
-- 
1.7.5.4

[PATCHv2 3/4] gprs: Use sim SPN watch API

[Oleg Zhurakivskyy]

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

---
 src/gprs.c |  105 ++++++++++++++++++++++++++++++++++++------------------------
 1 files changed, 63 insertions(+), 42 deletions(-)

diff --git a/src/gprs.c b/src/gprs.c
index 4e46743..c4ea974 100644
--- a/src/gprs.c
+++ b/src/gprs.c
@@ -94,7 +94,9 @@ struct ofono_gprs {
 	const struct ofono_gprs_driver *driver;
 	void *driver_data;
 	struct ofono_atom *atom;
-	struct ofono_sim_context *sim_context;
+	struct ofono_sim *sim;
+	unsigned int sim_watch;
+	unsigned int spn_watch;
 };
 
 struct ipv4_settings {
@@ -2502,6 +2504,39 @@ static void free_contexts(struct ofono_gprs *gprs)
 	g_slist_free(gprs->contexts);
 }
 
+static inline void sim_unregister(struct ofono_gprs *gprs,
+			unsigned int *spn_watch, unsigned int *sim_watch,
+			struct ofono_sim **sim)
+{
+	if (gprs->sim == NULL)
+		return;
+
+	if (*spn_watch)
+		ofono_sim_remove_spn_watch(*sim, spn_watch);
+
+	if (*sim_watch) {
+		__ofono_modem_remove_atom_watch(
+				__ofono_atom_get_modem(gprs->atom), *sim_watch);
+		*sim_watch = 0;
+	}
+
+	if (*sim)
+		*sim = NULL;
+}
+
+static void sim_watch(struct ofono_atom *atom,
+			enum ofono_atom_watch_condition cond, void *data)
+{
+	struct ofono_gprs *gprs = data;
+
+	if (cond == OFONO_ATOM_WATCH_CONDITION_UNREGISTERED) {
+		sim_unregister(gprs, &gprs->spn_watch, NULL, &gprs->sim);
+		return;
+	}
+
+	gprs->sim = __ofono_atom_get_data(atom);
+}
+
 static void gprs_unregister(struct ofono_atom *atom)
 {
 	DBusConnection *conn = ofono_dbus_get_connection();
@@ -2513,6 +2548,9 @@ static void gprs_unregister(struct ofono_atom *atom)
 
 	free_contexts(gprs);
 
+	if (gprs->sim && gprs->spn_watch)
+		sim_unregister(gprs, &gprs->spn_watch, NULL, NULL);
+
 	if (gprs->cid_map) {
 		idmap_free(gprs->cid_map);
 		gprs->cid_map = NULL;
@@ -2554,6 +2592,10 @@ static void gprs_remove(struct ofono_atom *atom)
 		gprs->pid_map = NULL;
 	}
 
+	if (gprs->sim)
+		sim_unregister(gprs, &gprs->spn_watch, &gprs->sim_watch,
+				&gprs->sim);
+
 	for (l = gprs->context_drivers; l; l = l->next) {
 		struct ofono_gprs_context *gc = l->data;
 
@@ -2565,9 +2607,6 @@ static void gprs_remove(struct ofono_atom *atom)
 	if (gprs->driver && gprs->driver->remove)
 		gprs->driver->remove(gprs);
 
-	if (gprs->sim_context)
-		ofono_sim_context_free(gprs->sim_context);
-
 	g_free(gprs);
 }
 
@@ -2605,6 +2644,9 @@ struct ofono_gprs *ofono_gprs_create(struct ofono_modem *modem,
 	gprs->netreg_status = NETWORK_REGISTRATION_STATUS_UNKNOWN;
 	gprs->pid_map = idmap_new(MAX_CONTEXTS);
 
+	gprs->sim_watch = __ofono_modem_add_atom_watch(modem,
+				OFONO_ATOM_TYPE_SIM, sim_watch, gprs, NULL);
+
 	return gprs;
 }
 
@@ -2955,57 +2997,36 @@ static void ofono_gprs_finish_register(struct ofono_gprs *gprs)
 	__ofono_atom_register(gprs->atom, gprs_unregister);
 }
 
-static void sim_spn_read_cb(int ok, int length, int record,
-				const unsigned char *data,
-				int record_length, void *userdata)
+static void spn_read_cb(const char *spn, const char *dc, void *data)
 {
-	struct ofono_gprs *gprs	= userdata;
-	char *spn = NULL;
-	struct ofono_atom *sim_atom;
-	struct ofono_sim *sim = NULL;
+	struct ofono_gprs *gprs	= data;
+
+	if (gprs->spn_watch == 0)
+		return;
 
-	if (ok)
-		spn = sim_string_to_utf8(data + 1, length - 1);
+	ofono_sim_remove_spn_watch(gprs->sim, &gprs->spn_watch);
 
-	sim_atom = __ofono_modem_find_atom(__ofono_atom_get_modem(gprs->atom),
-						OFONO_ATOM_TYPE_SIM);
-	if (sim_atom) {
-		sim = __ofono_atom_get_data(sim_atom);
-		provision_contexts(gprs, ofono_sim_get_mcc(sim),
-					ofono_sim_get_mnc(sim), spn);
-	}
+	if (gprs->sim != NULL && spn != NULL)
+		provision_contexts(gprs, ofono_sim_get_mcc(gprs->sim),
+					ofono_sim_get_mnc(gprs->sim), spn);
 
-	g_free(spn);
 	ofono_gprs_finish_register(gprs);
 }
 
 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;
-
-	sim_atom = __ofono_modem_find_atom(modem, OFONO_ATOM_TYPE_SIM);
-
-	if (sim_atom) {
-		const char *imsi;
-		sim = __ofono_atom_get_data(sim_atom);
-
-		imsi = ofono_sim_get_imsi(sim);
-		gprs_load_settings(gprs, imsi);
-	}
+	if (gprs->sim == NULL)
+		ofono_gprs_finish_register(gprs);
 
-	if (gprs->contexts == NULL && sim != NULL) {
-		/* Get Service Provider Name from SIM for provisioning */
-		gprs->sim_context = ofono_sim_context_create(sim);
+	gprs_load_settings(gprs, ofono_sim_get_imsi(gprs->sim));
 
-		if (ofono_sim_read(gprs->sim_context, SIM_EFSPN_FILEID,
-				OFONO_SIM_FILE_STRUCTURE_TRANSPARENT,
-					sim_spn_read_cb, gprs) >= 0)
-			return;
+	if (gprs->contexts) {
+		ofono_gprs_finish_register(gprs);
+		return;
 	}
 
-	ofono_gprs_finish_register(gprs);
+	ofono_sim_add_spn_watch(gprs->sim, &gprs->spn_watch, spn_read_cb,
+								gprs, NULL);
 }
 
 void ofono_gprs_remove(struct ofono_gprs *gprs)
-- 
1.7.5.4

[PATCHv2 4/4] network: Use sim SPN watch API

[Oleg Zhurakivskyy]

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

---
 src/network.c |  163 +++++++-------------------------------------------------
 1 files changed, 21 insertions(+), 142 deletions(-)

diff --git a/src/network.c b/src/network.c
index 92256a8..0b30c13 100644
--- a/src/network.c
+++ b/src/network.c
@@ -43,7 +43,6 @@
 #define NETWORK_REGISTRATION_FLAG_HOME_SHOW_PLMN	0x1
 #define NETWORK_REGISTRATION_FLAG_ROAMING_SHOW_SPN	0x2
 #define NETWORK_REGISTRATION_FLAG_READING_PNN		0x4
-#define NETWORK_REGISTRATION_FLAG_READING_SPN		0x8
 
 enum network_registration_mode {
 	NETWORK_REGISTRATION_MODE_AUTO =	0,
@@ -84,6 +83,7 @@ struct ofono_netreg {
 	struct ofono_atom *atom;
 	unsigned int hfp_watch;
 	char *spn;
+	unsigned int spn_watch;
 };
 
 struct network_operator_data {
@@ -1648,127 +1648,15 @@ static void sim_spn_display_condition_parse(struct ofono_netreg *netreg,
 		netreg->flags |= NETWORK_REGISTRATION_FLAG_ROAMING_SHOW_SPN;
 }
 
-static gboolean sim_spn_parse(const void *data, int length, char **dst)
+static void spn_read_cb(const char *spn, const char *dc, void *data)
 {
-	char *spn;
-
-	/*
-	 * TS 31.102 says:
-	 *
-	 * the string shall use:
-	 *
-	 * - either the SMS default 7-bit coded alphabet as defined in
-	 *   TS 23.038 [5] with bit 8 set to 0. The string shall be left
-	 *   justified. Unused bytes shall be set to 'FF'.
-	 *
-	 * - or one of the UCS2 code options defined in the annex of TS
-	 *   31.101 [11].
-	 *
-	 * 31.101 has no such annex though.  51.101 refers to Annex B of
-	 * itself which is not there either.  11.11 contains the same
-	 * paragraph as 51.101 and has an Annex B which we implement.
-	 */
-	spn = sim_string_to_utf8(data, length);
-	if (spn == NULL) {
-		ofono_error("EFspn read successfully, but couldn't parse");
-		return FALSE;
-	}
-
-	if (strlen(spn) == 0) {
-		g_free(spn);
-		return FALSE;
-	}
-
-	*dst = spn;
-	return TRUE;
-}
-
-static void sim_cphs_spn_short_read_cb(int ok, int length, int record,
-					const unsigned char *data,
-					int record_length, void *user_data)
-{
-	struct ofono_netreg *netreg = user_data;
-
-	netreg->flags &= ~NETWORK_REGISTRATION_FLAG_READING_SPN;
-
-	if (!ok)
-		return;
-
-	if (!sim_spn_parse(data, length, &netreg->spn))
-		return;
-
-	if (netreg->current_operator)
-		netreg_emit_operator_display_name(netreg);
-}
-
-static void sim_cphs_spn_read_cb(int ok, int length, int record,
-					const unsigned char *data,
-					int record_length, void *user_data)
-{
-	struct ofono_netreg *netreg = user_data;
-
-	if (!ok) {
-		if (__ofono_sim_cphs_service_available(netreg->sim,
-						SIM_CPHS_SERVICE_SHORT_SPN))
-			ofono_sim_read(netreg->sim_context,
-					SIM_EF_CPHS_SPN_SHORT_FILEID,
-					OFONO_SIM_FILE_STRUCTURE_TRANSPARENT,
-					sim_cphs_spn_short_read_cb, netreg);
-		else
-			netreg->flags &= ~NETWORK_REGISTRATION_FLAG_READING_SPN;
-
-		return;
-	}
-
-	netreg->flags &= ~NETWORK_REGISTRATION_FLAG_READING_SPN;
-
-	if (!sim_spn_parse(data, length, &netreg->spn))
-		return;
-
-	if (netreg->current_operator)
-		netreg_emit_operator_display_name(netreg);
-}
-
-static void sim_spn_read_cb(int ok, int length, int record,
-				const unsigned char *data,
-				int record_length, void *user_data)
-{
-	struct ofono_netreg *netreg = user_data;
-
-	if (!ok) {
-		ofono_sim_read(netreg->sim_context,
-				SIM_EF_CPHS_SPN_FILEID,
-				OFONO_SIM_FILE_STRUCTURE_TRANSPARENT,
-				sim_cphs_spn_read_cb, netreg);
-
-		return;
-	}
-
-	netreg->flags &= ~NETWORK_REGISTRATION_FLAG_READING_SPN;
-
-	if (!sim_spn_parse(data + 1, length - 1, &netreg->spn))
-		return;
-
-	sim_spn_display_condition_parse(netreg, data[0]);
-
-	if (netreg->current_operator)
-		netreg_emit_operator_display_name(netreg);
-}
-
-static void sim_spn_changed(int id, void *userdata)
-{
-	struct ofono_netreg *netreg = userdata;
-	gboolean had_spn;
-
-	if (netreg->flags & NETWORK_REGISTRATION_FLAG_READING_SPN)
-		return;
-
-	had_spn = netreg->spn != NULL && strlen(netreg->spn) > 0;
-	netreg->flags &= ~(NETWORK_REGISTRATION_FLAG_HOME_SHOW_PLMN |
-				NETWORK_REGISTRATION_FLAG_ROAMING_SHOW_SPN);
+	struct ofono_netreg *netreg = data;
+	gboolean had_spn = netreg->spn != NULL && strlen(netreg->spn) > 0;
 
 	g_free(netreg->spn);
 	netreg->spn = NULL;
+	netreg->flags &= ~(NETWORK_REGISTRATION_FLAG_HOME_SHOW_PLMN |
+				NETWORK_REGISTRATION_FLAG_ROAMING_SHOW_SPN);
 
 	/*
 	 * We can't determine whether the property really changed
@@ -1778,10 +1666,13 @@ static void sim_spn_changed(int id, void *userdata)
 	if (had_spn && netreg->current_operator)
 		netreg_emit_operator_display_name(netreg);
 
-	netreg->flags |= NETWORK_REGISTRATION_FLAG_READING_SPN;
-	ofono_sim_read(netreg->sim_context, SIM_EFSPN_FILEID,
-			OFONO_SIM_FILE_STRUCTURE_TRANSPARENT,
-			sim_spn_read_cb, netreg);
+	if (dc)
+		sim_spn_display_condition_parse(netreg, *dc);
+
+	netreg->spn = g_strdup(spn);
+
+	if (netreg->current_operator)
+		netreg_emit_operator_display_name(netreg);
 }
 
 int ofono_netreg_get_location(struct ofono_netreg *netreg)
@@ -1919,6 +1810,12 @@ static void netreg_unregister(struct ofono_atom *atom)
 		netreg->settings = NULL;
 	}
 
+	if (netreg->spn_watch)
+		ofono_sim_remove_spn_watch(netreg->sim, &netreg->spn_watch);
+
+	g_free(netreg->spn);
+	netreg->spn = NULL;
+
 	if (netreg->sim_context) {
 		ofono_sim_context_free(netreg->sim_context);
 		netreg->sim_context = NULL;
@@ -1947,7 +1844,6 @@ static void netreg_remove(struct ofono_atom *atom)
 	sim_eons_free(netreg->eons);
 	sim_spdi_free(netreg->spdi);
 
-	g_free(netreg->spn);
 	g_free(netreg);
 }
 
@@ -2199,25 +2095,8 @@ void ofono_netreg_register(struct ofono_netreg *netreg)
 						sim_pnn_opl_changed, netreg,
 						NULL);
 
-		netreg->flags |= NETWORK_REGISTRATION_FLAG_READING_SPN;
-		ofono_sim_read(netreg->sim_context, SIM_EFSPN_FILEID,
-				OFONO_SIM_FILE_STRUCTURE_TRANSPARENT,
-				sim_spn_read_cb, netreg);
-
-		ofono_sim_add_file_watch(netreg->sim_context, SIM_EFSPN_FILEID,
-						sim_spn_changed, netreg,
-						NULL);
-		ofono_sim_add_file_watch(netreg->sim_context,
-						SIM_EF_CPHS_SPN_FILEID,
-						sim_spn_changed, netreg,
-						NULL);
-
-		if (__ofono_sim_cphs_service_available(netreg->sim,
-						SIM_CPHS_SERVICE_SHORT_SPN))
-			ofono_sim_add_file_watch(netreg->sim_context,
-						SIM_EF_CPHS_SPN_SHORT_FILEID,
-						sim_spn_changed,
-						netreg, NULL);
+		ofono_sim_add_spn_watch(netreg->sim, &netreg->spn_watch,
+						spn_read_cb, netreg, NULL);
 
 		if (__ofono_sim_service_available(netreg->sim,
 				SIM_UST_SERVICE_PROVIDER_DISPLAY_INFO,
-- 
1.7.5.4

Re: [PATCHv2 4/4] network: Use sim SPN watch API

[Denis Kenzior]

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

Hi Oleg,

On 01/09/2012 04:30 AM, Oleg Zhurakivskyy wrote:
> ---
>  src/network.c |  163 +++++++-------------------------------------------------
>  1 files changed, 21 insertions(+), 142 deletions(-)

<snip>

> 
> +static void spn_read_cb(const char *spn, const char *dc, void *data)
> -static void sim_spn_changed(int id, void *userdata)
> -{
> -	struct ofono_netreg *netreg = userdata;
> -	gboolean had_spn;
> -
> -	if (netreg->flags & NETWORK_REGISTRATION_FLAG_READING_SPN)
> -		return;
> -
> -	had_spn = netreg->spn != NULL && strlen(netreg->spn) > 0;
> -	netreg->flags &= ~(NETWORK_REGISTRATION_FLAG_HOME_SHOW_PLMN |
> -				NETWORK_REGISTRATION_FLAG_ROAMING_SHOW_SPN);
> +	struct ofono_netreg *netreg = data;
> +	gboolean had_spn = netreg->spn != NULL && strlen(netreg->spn) > 0;
>  
>  	g_free(netreg->spn);
>  	netreg->spn = NULL;
> +	netreg->flags &= ~(NETWORK_REGISTRATION_FLAG_HOME_SHOW_PLMN |
> +				NETWORK_REGISTRATION_FLAG_ROAMING_SHOW_SPN);
>  
>  	/*
>  	 * We can't determine whether the property really changed
> @@ -1778,10 +1666,13 @@ static void sim_spn_changed(int id, void *userdata)
>  	if (had_spn && netreg->current_operator)
>  		netreg_emit_operator_display_name(netreg);
>  
> -	netreg->flags |= NETWORK_REGISTRATION_FLAG_READING_SPN;
> -	ofono_sim_read(netreg->sim_context, SIM_EFSPN_FILEID,
> -			OFONO_SIM_FILE_STRUCTURE_TRANSPARENT,
> -			sim_spn_read_cb, netreg);
> +	if (dc)
> +		sim_spn_display_condition_parse(netreg, *dc);
> +
> +	netreg->spn = g_strdup(spn);
> +
> +	if (netreg->current_operator)
> +		netreg_emit_operator_display_name(netreg);

This function ends up calling netreg_emit_operator_display_name twice,
once if had_spn is true and once here.  That doesn't seem quite right.

The original implementation was actually reacting to a SIM refresh,
basically if any of the SPN files was updated, we'd switch to operator
name as reported by NITZ (e.g. from +COPS) and wait for the SPN/CPHS SPN
reading to finish.  Then issue another signal.

In your implementation it is likely safe to simply always emit, in which
case you do not need any of the above logic.  Given the likelihood of
the above scenario it is probably fine to simply wait for the new
SPN/CPHS name to be read before emitting the signal.

>  }
>  
>  int ofono_netreg_get_location(struct ofono_netreg *netreg)
> @@ -1919,6 +1810,12 @@ static void netreg_unregister(struct ofono_atom *atom)
>  		netreg->settings = NULL;
>  	}
>  
> +	if (netreg->spn_watch)
> +		ofono_sim_remove_spn_watch(netreg->sim, &netreg->spn_watch);
> +
> +	g_free(netreg->spn);
> +	netreg->spn = NULL;
> +

With the above in mind, you could actually access netreg->spn directly
from the sim atom and save some memory, but the current implementation
is fine as well.

Regards,
-Denis

Re: [PATCHv2 3/4] gprs: Use sim SPN watch API

[Denis Kenzior]

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

Hi Oleg,

On 01/09/2012 04:30 AM, Oleg Zhurakivskyy wrote:
> ---
>  src/gprs.c |  105 ++++++++++++++++++++++++++++++++++++------------------------
>  1 files changed, 63 insertions(+), 42 deletions(-)
> 
> diff --git a/src/gprs.c b/src/gprs.c
> index 4e46743..c4ea974 100644
> --- a/src/gprs.c
> +++ b/src/gprs.c
> @@ -94,7 +94,9 @@ struct ofono_gprs {
>  	const struct ofono_gprs_driver *driver;
>  	void *driver_data;
>  	struct ofono_atom *atom;
> -	struct ofono_sim_context *sim_context;
> +	struct ofono_sim *sim;
> +	unsigned int sim_watch;

This is actually not necessary, the sim atom is always guaranteed to be
there prior to the gprs atom.

> +	unsigned int spn_watch;
>  };
>  
>  struct ipv4_settings {
> @@ -2502,6 +2504,39 @@ static void free_contexts(struct ofono_gprs *gprs)
>  	g_slist_free(gprs->contexts);
>  }
>  
> +static inline void sim_unregister(struct ofono_gprs *gprs,
> +			unsigned int *spn_watch, unsigned int *sim_watch,
> +			struct ofono_sim **sim)
> +{
> +	if (gprs->sim == NULL)
> +		return;
> +
> +	if (*spn_watch)
> +		ofono_sim_remove_spn_watch(*sim, spn_watch);
> +
> +	if (*sim_watch) {
> +		__ofono_modem_remove_atom_watch(
> +				__ofono_atom_get_modem(gprs->atom), *sim_watch);
> +		*sim_watch = 0;
> +	}

So all you really need here is this statement moved down to gprs_unregister

> +
> +	if (*sim)
> +		*sim = NULL;
> +}
> +
> +static void sim_watch(struct ofono_atom *atom,
> +			enum ofono_atom_watch_condition cond, void *data)
> +{
> +	struct ofono_gprs *gprs = data;
> +
> +	if (cond == OFONO_ATOM_WATCH_CONDITION_UNREGISTERED) {
> +		sim_unregister(gprs, &gprs->spn_watch, NULL, &gprs->sim);
> +		return;
> +	}
> +
> +	gprs->sim = __ofono_atom_get_data(atom);
> +}
> +

This isn't needed

>  static void gprs_unregister(struct ofono_atom *atom)
>  {
>  	DBusConnection *conn = ofono_dbus_get_connection();
> @@ -2513,6 +2548,9 @@ static void gprs_unregister(struct ofono_atom *atom)
>  
>  	free_contexts(gprs);
>  
> +	if (gprs->sim && gprs->spn_watch)
> +		sim_unregister(gprs, &gprs->spn_watch, NULL, NULL);
> +
>  	if (gprs->cid_map) {
>  		idmap_free(gprs->cid_map);
>  		gprs->cid_map = NULL;
> @@ -2554,6 +2592,10 @@ static void gprs_remove(struct ofono_atom *atom)
>  		gprs->pid_map = NULL;
>  	}
>  
> +	if (gprs->sim)
> +		sim_unregister(gprs, &gprs->spn_watch, &gprs->sim_watch,
> +				&gprs->sim);

Why do you have this here and in gprs_unregister?

> +
>  	for (l = gprs->context_drivers; l; l = l->next) {
>  		struct ofono_gprs_context *gc = l->data;
>  
> @@ -2565,9 +2607,6 @@ static void gprs_remove(struct ofono_atom *atom)
>  	if (gprs->driver && gprs->driver->remove)
>  		gprs->driver->remove(gprs);
>  
> -	if (gprs->sim_context)
> -		ofono_sim_context_free(gprs->sim_context);
> -
>  	g_free(gprs);
>  }
>  
> @@ -2605,6 +2644,9 @@ struct ofono_gprs *ofono_gprs_create(struct ofono_modem *modem,
>  	gprs->netreg_status = NETWORK_REGISTRATION_STATUS_UNKNOWN;
>  	gprs->pid_map = idmap_new(MAX_CONTEXTS);
>  
> +	gprs->sim_watch = __ofono_modem_add_atom_watch(modem,
> +				OFONO_ATOM_TYPE_SIM, sim_watch, gprs, NULL);
> +
>  	return gprs;
>  }
>  
> @@ -2955,57 +2997,36 @@ static void ofono_gprs_finish_register(struct ofono_gprs *gprs)
>  	__ofono_atom_register(gprs->atom, gprs_unregister);
>  }
>  
> -static void sim_spn_read_cb(int ok, int length, int record,
> -				const unsigned char *data,
> -				int record_length, void *userdata)
> +static void spn_read_cb(const char *spn, const char *dc, void *data)
>  {
> -	struct ofono_gprs *gprs	= userdata;
> -	char *spn = NULL;
> -	struct ofono_atom *sim_atom;
> -	struct ofono_sim *sim = NULL;
> +	struct ofono_gprs *gprs	= data;
> +
> +	if (gprs->spn_watch == 0)
> +		return;

Not sure I follow why you need this?

>  
> -	if (ok)
> -		spn = sim_string_to_utf8(data + 1, length - 1);
> +	ofono_sim_remove_spn_watch(gprs->sim, &gprs->spn_watch);
>  
> -	sim_atom = __ofono_modem_find_atom(__ofono_atom_get_modem(gprs->atom),
> -						OFONO_ATOM_TYPE_SIM);
> -	if (sim_atom) {
> -		sim = __ofono_atom_get_data(sim_atom);
> -		provision_contexts(gprs, ofono_sim_get_mcc(sim),
> -					ofono_sim_get_mnc(sim), spn);
> -	}
> +	if (gprs->sim != NULL && spn != NULL)

And this might be overly paranoid given that you're getting called by
the sim atom.

I tend to like the philosophy of 'crash early'.  This lets you know very
quickly if something is wrong without obfuscating the cause.

> +		provision_contexts(gprs, ofono_sim_get_mcc(gprs->sim),
> +					ofono_sim_get_mnc(gprs->sim), spn);
>  
> -	g_free(spn);
>  	ofono_gprs_finish_register(gprs);
>  }
>  
>  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;
> -
> -	sim_atom = __ofono_modem_find_atom(modem, OFONO_ATOM_TYPE_SIM);
> -
> -	if (sim_atom) {
> -		const char *imsi;
> -		sim = __ofono_atom_get_data(sim_atom);
> -
> -		imsi = ofono_sim_get_imsi(sim);
> -		gprs_load_settings(gprs, imsi);
> -	}
> +	if (gprs->sim == NULL)
> +		ofono_gprs_finish_register(gprs);
>  
> -	if (gprs->contexts == NULL && sim != NULL) {
> -		/* Get Service Provider Name from SIM for provisioning */
> -		gprs->sim_context = ofono_sim_context_create(sim);
> +	gprs_load_settings(gprs, ofono_sim_get_imsi(gprs->sim));

If sim is null we still try to load the settings? Wouldn't this crash?
>  
> -		if (ofono_sim_read(gprs->sim_context, SIM_EFSPN_FILEID,
> -				OFONO_SIM_FILE_STRUCTURE_TRANSPARENT,
> -					sim_spn_read_cb, gprs) >= 0)
> -			return;
> +	if (gprs->contexts) {
> +		ofono_gprs_finish_register(gprs);
> +		return;
>  	}
>  
> -	ofono_gprs_finish_register(gprs);
> +	ofono_sim_add_spn_watch(gprs->sim, &gprs->spn_watch, spn_read_cb,
> +								gprs, NULL);

And this is probably useless if the sim atom isn't around...

>  }
>  
>  void ofono_gprs_remove(struct ofono_gprs *gprs)

Regards,
-Denis

Re: [PATCHv2 1/4] sim: Minor style fixes

[Denis Kenzior]

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

Hi Oleg,

On 01/09/2012 04:30 AM, Oleg Zhurakivskyy wrote:
> ---
>  src/sim.c |   24 ++++++++++++------------
>  1 files changed, 12 insertions(+), 12 deletions(-)
> 

Patch has been applied, thanks.

Regards,
-Denis

Re: [PATCHv2 2/4] sim: Add SPN watch capability

[Denis Kenzior]

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

Hi Oleg,

On 01/09/2012 04:30 AM, Oleg Zhurakivskyy wrote:
> ---
>  include/sim.h |    8 ++
>  src/sim.c     |  242 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 250 insertions(+), 0 deletions(-)
> 

Patch has been applied, thanks.

Regards,
-Denis

Re: [PATCHv2 3/4] gprs: Use sim SPN watch API

[Oleg Zhurakivskyy]

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

Hello Denis,

On 01/16/2012 04:29 AM, Denis Kenzior wrote:
>> @@ -94,7 +94,9 @@ struct ofono_gprs {
[...]
>> +	unsigned int sim_watch;
>
> This is actually not necessary, the sim atom is always guaranteed to be
> there prior to the gprs atom.

Thanks, this should make it simpler.

>>   static void gprs_unregister(struct ofono_atom *atom)
[...]
>> +	if (gprs->sim)
>> +		sim_unregister(gprs,&gprs->spn_watch,&gprs->sim_watch,
>> +				&gprs->sim);
>
> Why do you have this here and in gprs_unregister?

Good point, this can be removed.

>> +static void spn_read_cb(const char *spn, const char *dc, void *data)
>>   {
>> -	struct ofono_gprs *gprs	= userdata;
>> -	char *spn = NULL;
>> -	struct ofono_atom *sim_atom;
>> -	struct ofono_sim *sim = NULL;
>> +	struct ofono_gprs *gprs	= data;
>> +
>> +	if (gprs->spn_watch == 0)
>> +		return;
>
> Not sure I follow why you need this?

You are right, this isn't necessary.

>> +	ofono_sim_remove_spn_watch(gprs->sim,&gprs->spn_watch);
>>
>> -	sim_atom = __ofono_modem_find_atom(__ofono_atom_get_modem(gprs->atom),
>> -						OFONO_ATOM_TYPE_SIM);
>> -	if (sim_atom) {
>> -		sim = __ofono_atom_get_data(sim_atom);
>> -		provision_contexts(gprs, ofono_sim_get_mcc(sim),
>> -					ofono_sim_get_mnc(sim), spn);
>> -	}
>> +	if (gprs->sim != NULL&&  spn != NULL)
>
> And this might be overly paranoid given that you're getting called by
> the sim atom.
>
> I tend to like the philosophy of 'crash early'.  This lets you know very
> quickly if something is wrong without obfuscating the cause.

Good catch, these checks are redundant indeed, thanks.

>>   void ofono_gprs_register(struct ofono_gprs *gprs)
>>   {
[...]
>> +	if (gprs->sim == NULL)
>> +		ofono_gprs_finish_register(gprs);
>>
>> -	if (gprs->contexts == NULL&&  sim != NULL) {
>> -		/* Get Service Provider Name from SIM for provisioning */
>> -		gprs->sim_context = ofono_sim_context_create(sim);
>> +	gprs_load_settings(gprs, ofono_sim_get_imsi(gprs->sim));
>
> If sim is null we still try to load the settings? Wouldn't this crash?

Thanks for the comments, I will correct all these.

Regards,
Oleg
-- 
Intel Finland Oy
Registered Address: PL 281, 00181 Helsinki
Business Identity Code: 0357606 - 4
Domiciled in Helsinki

Re: [PATCHv2 4/4] network: Use sim SPN watch API

[Oleg Zhurakivskyy]

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

Hello Denis,

On 01/16/2012 04:14 AM, Denis Kenzior wrote:
>> +static void spn_read_cb(const char *spn, const char *dc, void *data)
[...]
>> +	gboolean had_spn = netreg->spn != NULL&&  strlen(netreg->spn)>  0;
>>
>>   	g_free(netreg->spn);
>>   	netreg->spn = NULL;
>> +	netreg->flags&= ~(NETWORK_REGISTRATION_FLAG_HOME_SHOW_PLMN |
>> +				NETWORK_REGISTRATION_FLAG_ROAMING_SHOW_SPN);
[...]
>> +	if (netreg->current_operator)
>> +		netreg_emit_operator_display_name(netreg);
>
> This function ends up calling netreg_emit_operator_display_name twice,
> once if had_spn is true and once here.  That doesn't seem quite right.
>
> The original implementation was actually reacting to a SIM refresh,
> basically if any of the SPN files was updated, we'd switch to operator
> name as reported by NITZ (e.g. from +COPS) and wait for the SPN/CPHS SPN
> reading to finish.  Then issue another signal.
>
> In your implementation it is likely safe to simply always emit, in which
> case you do not need any of the above logic.  Given the likelihood of
> the above scenario it is probably fine to simply wait for the new
> SPN/CPHS name to be read before emitting the signal.

Thanks for the explanation here. You are right, I just missed this. This would 
be better approach indeed.

>> +	g_free(netreg->spn);
>> +	netreg->spn = NULL;
>
> With the above in mind, you could actually access netreg->spn directly
> from the sim atom and save some memory, but the current implementation
> is fine as well.

I had this in mind, just wasn't sure whether this would keep the logic intact. I 
will try to do so.

Thanks for the help! I will prepare and send another patch.

Regards,
Oleg
-- 
Intel Finland Oy
Registered Address: PL 281, 00181 Helsinki
Business Identity Code: 0357606 - 4
Domiciled in Helsinki