[PATCHv3 0/4] Unify SPN reading logic

[PATCHv3 0/4] Unify SPN reading logic

[Oleg Zhurakivskyy]

[-- Attachment #1: Type: text/plain, Size: 764 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 v2:

- Removed sim_watch() capability from gprs.c
- Corrected the operator name emission logic.
- In order to save some memory, netreg accesses the spn directly
  from the sim atom.

Regards,
Oleg

Oleg Zhurakivskyy (4):
  gprs: Use sim SPN watch API
  network: Use sim SPN watch API
  sim: Add ofono_sim_get_spn()
  network: Access SPN directly from the sim atom

 include/sim.h |    1 +
 src/gprs.c    |   71 ++++++++++-------------
 src/network.c |  171 ++++++---------------------------------------------------
 src/sim.c     |    8 +++
 4 files changed, 58 insertions(+), 193 deletions(-)

-- 
1.7.5.4

[PATCHv3 1/4] gprs: Use sim SPN watch API

[Oleg Zhurakivskyy]

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

---
 src/gprs.c |   71 ++++++++++++++++++++++++++---------------------------------
 1 files changed, 31 insertions(+), 40 deletions(-)

diff --git a/src/gprs.c b/src/gprs.c
index 4e46743..bf7c66f 100644
--- a/src/gprs.c
+++ b/src/gprs.c
@@ -94,7 +94,7 @@ struct ofono_gprs {
 	const struct ofono_gprs_driver *driver;
 	void *driver_data;
 	struct ofono_atom *atom;
-	struct ofono_sim_context *sim_context;
+	unsigned int spn_watch;
 };
 
 struct ipv4_settings {
@@ -2502,6 +2502,17 @@ static void free_contexts(struct ofono_gprs *gprs)
 	g_slist_free(gprs->contexts);
 }
 
+static inline struct ofono_sim *ofono_gprs_get_sim(struct ofono_gprs *gprs)
+{
+	struct ofono_atom *atom = __ofono_modem_find_atom(
+					__ofono_atom_get_modem(gprs->atom),
+					OFONO_ATOM_TYPE_SIM);
+	if (atom)
+		return __ofono_atom_get_data(atom);
+
+	return NULL;
+}
+
 static void gprs_unregister(struct ofono_atom *atom)
 {
 	DBusConnection *conn = ofono_dbus_get_connection();
@@ -2530,6 +2541,10 @@ static void gprs_unregister(struct ofono_atom *atom)
 		gprs->netreg = NULL;
 	}
 
+	if (gprs->spn_watch)
+		ofono_sim_remove_spn_watch(ofono_gprs_get_sim(gprs),
+							&gprs->spn_watch);
+
 	ofono_modem_remove_interface(modem,
 					OFONO_CONNECTION_MANAGER_INTERFACE);
 	g_dbus_unregister_interface(conn, path,
@@ -2565,9 +2580,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);
 }
 
@@ -2955,57 +2967,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;
+	struct ofono_sim *sim = ofono_gprs_get_sim(gprs);
 
-	if (ok)
-		spn = sim_string_to_utf8(data + 1, length - 1);
-
-	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),
+	provision_contexts(gprs, ofono_sim_get_mcc(sim),
 					ofono_sim_get_mnc(sim), spn);
-	}
 
-	g_free(spn);
+	ofono_sim_remove_spn_watch(sim, &gprs->spn_watch);
+
 	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);
+	struct ofono_sim *sim = ofono_gprs_get_sim(gprs);
 
-		imsi = ofono_sim_get_imsi(sim);
-		gprs_load_settings(gprs, imsi);
+	if (sim == NULL) {
+		ofono_gprs_finish_register(gprs);
+		return;
 	}
 
-	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(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(sim, &gprs->spn_watch, spn_read_cb, gprs, NULL);
 }
 
 void ofono_gprs_remove(struct ofono_gprs *gprs)
-- 
1.7.5.4

[PATCHv3 2/4] network: Use sim SPN watch API

[Oleg Zhurakivskyy]

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

---
 src/network.c |  166 ++++++--------------------------------------------------
 1 files changed, 18 insertions(+), 148 deletions(-)

diff --git a/src/network.c b/src/network.c
index 92256a8..e5f67a8 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,140 +1648,22 @@ 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;
+	struct ofono_netreg *netreg = data;
 
-	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);
 
-	g_free(netreg->spn);
-	netreg->spn = NULL;
+	if (dc)
+		sim_spn_display_condition_parse(netreg, *dc);
 
-	/*
-	 * We can't determine whether the property really changed
-	 * without checking the name, before and after.  Instead we use a
-	 * simple heuristic, which will not always be correct
-	 */
-	if (had_spn && netreg->current_operator)
-		netreg_emit_operator_display_name(netreg);
+	netreg->spn = g_strdup(spn);
 
-	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 (netreg->current_operator)
+		netreg_emit_operator_display_name(netreg);
 }
 
 int ofono_netreg_get_location(struct ofono_netreg *netreg)
@@ -1919,6 +1801,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 +1835,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 +2086,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

[PATCHv3 3/4] sim: Add ofono_sim_get_spn()

[Oleg Zhurakivskyy]

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

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

diff --git a/include/sim.h b/include/sim.h
index b238868..605164e 100644
--- a/include/sim.h
+++ b/include/sim.h
@@ -183,6 +183,7 @@ void *ofono_sim_get_data(struct ofono_sim *sim);
 const char *ofono_sim_get_imsi(struct ofono_sim *sim);
 const char *ofono_sim_get_mcc(struct ofono_sim *sim);
 const char *ofono_sim_get_mnc(struct ofono_sim *sim);
+const char *ofono_sim_get_spn(struct ofono_sim *sim);
 enum ofono_sim_phase ofono_sim_get_phase(struct ofono_sim *sim);
 
 enum ofono_sim_cphs_phase ofono_sim_get_cphs_phase(struct ofono_sim *sim);
diff --git a/src/sim.c b/src/sim.c
index 93b3655..e4bd378 100644
--- a/src/sim.c
+++ b/src/sim.c
@@ -2151,6 +2151,14 @@ const char *ofono_sim_get_mnc(struct ofono_sim *sim)
 	return sim->mnc;
 }
 
+const char *ofono_sim_get_spn(struct ofono_sim *sim)
+{
+	if (sim == NULL)
+		return NULL;
+
+	return sim->spn;
+}
+
 enum ofono_sim_phase ofono_sim_get_phase(struct ofono_sim *sim)
 {
 	if (sim == NULL)
-- 
1.7.5.4

[PATCHv3 4/4] network: Access SPN directly from the sim atom

[Oleg Zhurakivskyy]

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

---
 src/network.c |   19 +++++++------------
 1 files changed, 7 insertions(+), 12 deletions(-)

diff --git a/src/network.c b/src/network.c
index e5f67a8..0699ff2 100644
--- a/src/network.c
+++ b/src/network.c
@@ -82,7 +82,6 @@ struct ofono_netreg {
 	void *driver_data;
 	struct ofono_atom *atom;
 	unsigned int hfp_watch;
-	char *spn;
 	unsigned int spn_watch;
 };
 
@@ -362,6 +361,7 @@ static char *get_operator_display_name(struct ofono_netreg *netreg)
 {
 	struct network_operator_data *opd = netreg->current_operator;
 	const char *plmn;
+	const char *spn;
 	static char name[1024];
 	static char mccmnc[OFONO_MAX_MCC_LENGTH + OFONO_MAX_MNC_LENGTH + 1];
 	int len = sizeof(name);
@@ -392,7 +392,9 @@ static char *get_operator_display_name(struct ofono_netreg *netreg)
 	if (opd->eons_info && opd->eons_info->longname)
 		plmn = opd->eons_info->longname;
 
-	if (netreg->spn == NULL || strlen(netreg->spn) == 0) {
+	spn = ofono_sim_get_spn(netreg->sim);
+
+	if (spn == NULL || strlen(spn) == 0) {
 		g_strlcpy(name, plmn, len);
 		return name;
 	}
@@ -406,14 +408,14 @@ static char *get_operator_display_name(struct ofono_netreg *netreg)
 	if (home_or_spdi)
 		if (netreg->flags & NETWORK_REGISTRATION_FLAG_HOME_SHOW_PLMN)
 			/* Case 1 */
-			snprintf(name, len, "%s (%s)", netreg->spn, plmn);
+			snprintf(name, len, "%s (%s)", spn, plmn);
 		else
 			/* Case 2 */
-			snprintf(name, len, "%s", netreg->spn);
+			snprintf(name, len, "%s", spn);
 	else
 		if (netreg->flags & NETWORK_REGISTRATION_FLAG_ROAMING_SHOW_SPN)
 			/* Case 3 */
-			snprintf(name, len, "%s (%s)", netreg->spn, plmn);
+			snprintf(name, len, "%s (%s)", spn, plmn);
 		else
 			/* Case 4 */
 			snprintf(name, len, "%s", plmn);
@@ -1652,16 +1654,12 @@ static void spn_read_cb(const char *spn, const char *dc, void *data)
 {
 	struct ofono_netreg *netreg = data;
 
-	g_free(netreg->spn);
-	netreg->spn = NULL;
 	netreg->flags &= ~(NETWORK_REGISTRATION_FLAG_HOME_SHOW_PLMN |
 				NETWORK_REGISTRATION_FLAG_ROAMING_SHOW_SPN);
 
 	if (dc)
 		sim_spn_display_condition_parse(netreg, *dc);
 
-	netreg->spn = g_strdup(spn);
-
 	if (netreg->current_operator)
 		netreg_emit_operator_display_name(netreg);
 }
@@ -1804,9 +1802,6 @@ static void netreg_unregister(struct ofono_atom *atom)
 	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;
-- 
1.7.5.4

Re: [PATCHv3 0/4] Unify SPN reading logic

[Denis Kenzior]

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

Hi Oleg,

On 01/18/2012 05:56 AM, Oleg Zhurakivskyy wrote:
> Hello,
> 
> Please find the changes in order not to duplicate the SPN reading logic in src/grps.c and src/network.c.
> 
> Changes from v2:
> 
> - Removed sim_watch() capability from gprs.c
> - Corrected the operator name emission logic.
> - In order to save some memory, netreg accesses the spn directly
>   from the sim atom.
> 
> Regards,
> Oleg
> 
> Oleg Zhurakivskyy (4):
>   gprs: Use sim SPN watch API
>   network: Use sim SPN watch API
>   sim: Add ofono_sim_get_spn()
>   network: Access SPN directly from the sim atom
> 
>  include/sim.h |    1 +
>  src/gprs.c    |   71 ++++++++++-------------
>  src/network.c |  171 ++++++---------------------------------------------------
>  src/sim.c     |    8 +++
>  4 files changed, 58 insertions(+), 193 deletions(-)
> 

All four patches have been applied, thanks.  I did break up patch 3 into
two patches and performed some minor refactoring afterwards.  Please
review and make sure I didn't screw anything up.

Regards,
-Denis

Re: [PATCHv3 0/4] Unify SPN reading logic

[Oleg Zhurakivskyy]

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

Hello Denis,

On 01/18/2012 09:04 PM, Denis Kenzior wrote:
> All four patches have been applied, thanks.  I did break up patch 3 into
> two patches and performed some minor refactoring afterwards.  Please
> review and make sure I didn't screw anything up.

The changes look good, I like the idea. Thanks!

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