[PATCH 0/9] CPHS SPN, short-SPN support

[PATCH 0/9] CPHS SPN, short-SPN support

[Oleg Zhurakivskyy]

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

Hello,

Please find the initial version of the CPHS SPN and short-SPN TODO items.

What's missing:

- changes to SPN callback in src/grps.c in order not to duplicate the logic for SPN, CPHS SPN and short SPN in both files.

Regards,
Oleg

Oleg Zhurakivskyy (9):
  network: M9 coding style corrections
  network: Refactor sim_spn_read_cb()
  network: Rename spname to spn
  gprs-provision: Fix crash if no SPN present
  simutil: Add CPHS SPN and short-SPN IDs
  network: Add CPHS SPN, short-SPN flags
  network: Add CPHS SPN fallback
  network: Add CPHS short-SPN fallback
  TODO: Remove completed CPHS SPN and short-SPN tasks

 TODO                 |   14 -----
 examples/provision.c |    2 +-
 src/network.c        |  155 ++++++++++++++++++++++++++++++++++----------------
 src/simutil.h        |    2 +
 4 files changed, 109 insertions(+), 64 deletions(-)

-- 
1.7.5.4

[PATCH 1/9] network: M9 coding style corrections

[Oleg Zhurakivskyy]

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

---
 src/network.c |   10 +++++-----
 1 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/src/network.c b/src/network.c
index 19f9c00..e6636bb 100644
--- a/src/network.c
+++ b/src/network.c
@@ -37,6 +37,9 @@
 #include "util.h"
 #include "storage.h"
 
+#define SETTINGS_STORE "netreg"
+#define SETTINGS_GROUP "Settings"
+
 #define NETWORK_REGISTRATION_FLAG_HOME_SHOW_PLMN 0x1
 #define NETWORK_REGISTRATION_FLAG_ROAMING_SHOW_SPN 0x2
 
@@ -46,11 +49,6 @@ enum network_registration_mode {
 	NETWORK_REGISTRATION_MODE_AUTO_ONLY =	5, /* Out of range of 27.007 */
 };
 
-#define SETTINGS_STORE "netreg"
-#define SETTINGS_GROUP "Settings"
-
-static GSList *g_drivers = NULL;
-
 /* 27.007 Section 7.3 <stat> */
 enum operator_status {
 	OPERATOR_STATUS_UNKNOWN =	0,
@@ -96,6 +94,8 @@ struct network_operator_data {
 	struct ofono_netreg *netreg;
 };
 
+static GSList *g_drivers = NULL;
+
 static const char *registration_mode_to_string(int mode)
 {
 	switch (mode) {
-- 
1.7.5.4

[PATCH 2/9] network: Refactor sim_spn_read_cb()

[Oleg Zhurakivskyy]

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

Refactor sim_spn_read_cb() for CPHS SPN, short-SPN inclusion
---
 src/network.c |   82 +++++++++++++++++++++++++++++++++-----------------------
 1 files changed, 48 insertions(+), 34 deletions(-)

diff --git a/src/network.c b/src/network.c
index e6636bb..677b936 100644
--- a/src/network.c
+++ b/src/network.c
@@ -1656,18 +1656,30 @@ static void sim_spdi_read_cb(int ok, int length, int record,
 	}
 }
 
-static void sim_spn_read_cb(int ok, int length, int record,
-				const unsigned char *data,
-				int record_length, void *user_data)
+static void ofono_netreg_operator_display_name_notify(
+						struct ofono_netreg *netreg)
 {
-	struct ofono_netreg *netreg = user_data;
-	unsigned char dcbyte;
-	char *spn;
+	const char *operator = get_operator_display_name(netreg);
 
-	if (!ok)
-		return;
+	ofono_dbus_signal_property_changed(ofono_dbus_get_connection(),
+					__ofono_atom_get_path(netreg->atom),
+					OFONO_NETWORK_REGISTRATION_INTERFACE,
+					"Name", DBUS_TYPE_STRING, &operator);
+}
 
-	dcbyte = data[0];
+static void sim_spn_display_condition_parse(struct ofono_netreg *netreg,
+						guint8 dcbyte)
+{
+	if (dcbyte & SIM_EFSPN_DC_HOME_PLMN_BIT)
+		netreg->flags |= NETWORK_REGISTRATION_FLAG_HOME_SHOW_PLMN;
+
+	if (!(dcbyte & SIM_EFSPN_DC_ROAMING_SPN_BIT))
+		netreg->flags |= NETWORK_REGISTRATION_FLAG_ROAMING_SHOW_SPN;
+}
+
+static gboolean sim_spn_parse(const void *data, int length, char **dst)
+{
+	char *spn;
 
 	/*
 	 * TS 31.102 says:
@@ -1685,40 +1697,42 @@ static void sim_spn_read_cb(int ok, int length, int record,
 	 * 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 + 1, length - 1);
-	if (spn == NULL) {
-		ofono_error("EFspn read successfully, but couldn't parse");
-		return;
+	spn = sim_string_to_utf8(data, length);
+	if (spn == NULL || strlen(spn) == 0) {
+		if (spn == NULL)
+			ofono_error("EFspn read successfully, "
+					"but couldn't parse");
+		g_free(spn);
+		return FALSE;
 	}
 
-	if (strlen(spn) == 0) {
-		g_free(spn);
+	*dst = spn;
+	return TRUE;
+}
+
+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;
+	unsigned char dcbyte;
+
+	if (!ok)
+		return;
+
+	dcbyte = data[0];
+
+	if (!sim_spn_parse(data + 1, length - 1, &netreg->spname))
 		return;
-	}
 
-	netreg->spname = spn;
 	ofono_sim_read(netreg->sim_context, SIM_EFSPDI_FILEID,
 			OFONO_SIM_FILE_STRUCTURE_TRANSPARENT,
 			sim_spdi_read_cb, netreg);
 
-	if (dcbyte & SIM_EFSPN_DC_HOME_PLMN_BIT)
-		netreg->flags |= NETWORK_REGISTRATION_FLAG_HOME_SHOW_PLMN;
+	sim_spn_display_condition_parse(netreg, dcbyte);
 
-	if (!(dcbyte & SIM_EFSPN_DC_ROAMING_SPN_BIT))
-		netreg->flags |= NETWORK_REGISTRATION_FLAG_ROAMING_SHOW_SPN;
-
-	if (netreg->current_operator) {
-		DBusConnection *conn = ofono_dbus_get_connection();
-		const char *path = __ofono_atom_get_path(netreg->atom);
-		const char *operator;
-
-		operator = get_operator_display_name(netreg);
-
-		ofono_dbus_signal_property_changed(conn, path,
-					OFONO_NETWORK_REGISTRATION_INTERFACE,
-					"Name", DBUS_TYPE_STRING,
-					&operator);
-	}
+	if (netreg->current_operator)
+		ofono_netreg_operator_display_name_notify(netreg);
 }
 
 int ofono_netreg_get_location(struct ofono_netreg *netreg)
-- 
1.7.5.4

[PATCH 3/9] network: Rename spname to spn

[Oleg Zhurakivskyy]

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

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

diff --git a/src/network.c b/src/network.c
index 677b936..fbcd449 100644
--- a/src/network.c
+++ b/src/network.c
@@ -70,7 +70,7 @@ struct ofono_netreg {
 	int flags;
 	DBusMessage *pending;
 	int signal_strength;
-	char *spname;
+	char *spn;
 	struct sim_spdi *spdi;
 	struct sim_eons *eons;
 	struct ofono_sim *sim;
@@ -390,7 +390,7 @@ 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->spname == NULL || strlen(netreg->spname) == 0) {
+	if (netreg->spn == NULL || strlen(netreg->spn) == 0) {
 		g_strlcpy(name, plmn, len);
 		return name;
 	}
@@ -404,14 +404,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->spname, plmn);
+			snprintf(name, len, "%s (%s)", netreg->spn, plmn);
 		else
 			/* Case 2 */
-			snprintf(name, len, "%s", netreg->spname);
+			snprintf(name, len, "%s", netreg->spn);
 	else
 		if (netreg->flags & NETWORK_REGISTRATION_FLAG_ROAMING_SHOW_SPN)
 			/* Case 3 */
-			snprintf(name, len, "%s (%s)", netreg->spname, plmn);
+			snprintf(name, len, "%s (%s)", netreg->spn, plmn);
 		else
 			/* Case 4 */
 			snprintf(name, len, "%s", plmn);
@@ -1722,7 +1722,7 @@ static void sim_spn_read_cb(int ok, int length, int record,
 
 	dcbyte = data[0];
 
-	if (!sim_spn_parse(data + 1, length - 1, &netreg->spname))
+	if (!sim_spn_parse(data + 1, length - 1, &netreg->spn))
 		return;
 
 	ofono_sim_read(netreg->sim_context, SIM_EFSPDI_FILEID,
@@ -1898,7 +1898,7 @@ static void netreg_remove(struct ofono_atom *atom)
 	sim_eons_free(netreg->eons);
 	sim_spdi_free(netreg->spdi);
 
-	g_free(netreg->spname);
+	g_free(netreg->spn);
 	g_free(netreg);
 }
 
@@ -2031,8 +2031,8 @@ static void sim_spn_spdi_changed(int id, void *userdata)
 	netreg->flags &= ~(NETWORK_REGISTRATION_FLAG_HOME_SHOW_PLMN |
 			NETWORK_REGISTRATION_FLAG_ROAMING_SHOW_SPN);
 
-	g_free(netreg->spname);
-	netreg->spname = NULL;
+	g_free(netreg->spn);
+	netreg->spn = NULL;
 
 	sim_spdi_free(netreg->spdi);
 	netreg->spdi = NULL;
-- 
1.7.5.4

[PATCH 4/9] gprs-provision: Fix crash if no SPN present

[Oleg Zhurakivskyy]

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

---
 examples/provision.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/examples/provision.c b/examples/provision.c
index 546a161..3f9d124 100644
--- a/examples/provision.c
+++ b/examples/provision.c
@@ -49,7 +49,7 @@ static int example_provision_get_settings(const char *mcc, const char *mnc,
 			mcc, mnc, spn);
 
 	if (strcmp(mcc, "246") != 0 || strcmp(mnc, "81") != 0 ||
-						strcmp(spn, "oFono") != 0)
+						g_strcmp0(spn, "oFono") != 0)
 		return -ENOENT;
 
 	ofono_debug("Creating example settings for phonesim");
-- 
1.7.5.4

[PATCH 5/9] simutil: Add CPHS SPN and short-SPN IDs

[Oleg Zhurakivskyy]

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

---
 src/simutil.h |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/src/simutil.h b/src/simutil.h
index fde3126..42251d4 100644
--- a/src/simutil.h
+++ b/src/simutil.h
@@ -38,9 +38,11 @@ enum sim_fileid {
 	SIM_EFIMSI_FILEID =			0x6F07,
 	SIM_EF_CPHS_MWIS_FILEID =		0x6F11,
 	SIM_EF_CPHS_CFF_FILEID =		0x6F13,
+	SIM_EF_CPHS_SPN_FILEID =		0x6F14,
 	SIM_EF_CPHS_CSP_FILEID =		0x6F15,
 	SIM_EF_CPHS_INFORMATION_FILEID =	0x6F16,
 	SIM_EF_CPHS_MBDN_FILEID =		0x6F17,
+	SIM_EF_CPHS_SPN_SHORT_FILEID =		0x6F18,
 	SIM_EFUST_FILEID =			0x6F38,
 	SIM_EFSST_FILEID =			0x6F38, /* same as EFust */
 	SIM_EFADN_FILEID =			0x6F3A,
-- 
1.7.5.4

[PATCH 6/9] network: Add CPHS SPN, short-SPN flags

[Oleg Zhurakivskyy]

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

---
 src/network.c |    8 ++++++--
 1 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/src/network.c b/src/network.c
index fbcd449..6e08876 100644
--- a/src/network.c
+++ b/src/network.c
@@ -40,8 +40,12 @@
 #define SETTINGS_STORE "netreg"
 #define SETTINGS_GROUP "Settings"
 
-#define NETWORK_REGISTRATION_FLAG_HOME_SHOW_PLMN 0x1
-#define NETWORK_REGISTRATION_FLAG_ROAMING_SHOW_SPN 0x2
+enum network_registration_flag {
+	NETWORK_REGISTRATION_FLAG_HOME_SHOW_PLMN =	0x1,
+	NETWORK_REGISTRATION_FLAG_ROAMING_SHOW_SPN =	0x2,
+	NETWORK_REGISTRATION_FLAG_CPHS_SPN =		0x4,
+	NETWORK_REGISTRATION_FLAG_CPHS_SPN_SHORT =	0x8,
+};
 
 enum network_registration_mode {
 	NETWORK_REGISTRATION_MODE_AUTO =	0,
-- 
1.7.5.4

[PATCH 7/9] network: Add CPHS SPN fallback

[Oleg Zhurakivskyy]

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

---
 src/network.c |   32 ++++++++++++++++++++++++--------
 1 files changed, 24 insertions(+), 8 deletions(-)

diff --git a/src/network.c b/src/network.c
index 6e08876..1354edf 100644
--- a/src/network.c
+++ b/src/network.c
@@ -1719,21 +1719,37 @@ static void sim_spn_read_cb(int ok, int length, int record,
 				int record_length, void *user_data)
 {
 	struct ofono_netreg *netreg = user_data;
-	unsigned char dcbyte;
+	const unsigned char *dcbyte = NULL;
+	int offset = 0;
 
-	if (!ok)
+	if (!ok) {
+		if (!(netreg->flags & NETWORK_REGISTRATION_FLAG_CPHS_SPN)) {
+
+			netreg->flags |= NETWORK_REGISTRATION_FLAG_CPHS_SPN;
+
+			ofono_sim_read(netreg->sim_context,
+					SIM_EF_CPHS_SPN_FILEID,
+					OFONO_SIM_FILE_STRUCTURE_TRANSPARENT,
+					sim_spn_read_cb, netreg);
+		}
 		return;
+	}
 
-	dcbyte = data[0];
+	if (!(netreg->flags & NETWORK_REGISTRATION_FLAG_CPHS_SPN)) {
+		dcbyte = data;
+		offset = 1;
+	}
 
-	if (!sim_spn_parse(data + 1, length - 1, &netreg->spn))
+	if (!sim_spn_parse(data + offset, length - offset, &netreg->spn))
 		return;
 
-	ofono_sim_read(netreg->sim_context, SIM_EFSPDI_FILEID,
-			OFONO_SIM_FILE_STRUCTURE_TRANSPARENT,
-			sim_spdi_read_cb, netreg);
+	if (!(netreg->flags & NETWORK_REGISTRATION_FLAG_CPHS_SPN))
+		ofono_sim_read(netreg->sim_context, SIM_EFSPDI_FILEID,
+				OFONO_SIM_FILE_STRUCTURE_TRANSPARENT,
+				sim_spdi_read_cb, netreg);
 
-	sim_spn_display_condition_parse(netreg, dcbyte);
+	if (dcbyte != NULL)
+		sim_spn_display_condition_parse(netreg, *dcbyte);
 
 	if (netreg->current_operator)
 		ofono_netreg_operator_display_name_notify(netreg);
-- 
1.7.5.4

[PATCH 8/9] network: Add CPHS short-SPN fallback

[Oleg Zhurakivskyy]

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

---
 src/network.c |   29 ++++++++++++++++++++++++++---
 1 files changed, 26 insertions(+), 3 deletions(-)

diff --git a/src/network.c b/src/network.c
index 1354edf..cfda2d8 100644
--- a/src/network.c
+++ b/src/network.c
@@ -1723,7 +1723,8 @@ static void sim_spn_read_cb(int ok, int length, int record,
 	int offset = 0;
 
 	if (!ok) {
-		if (!(netreg->flags & NETWORK_REGISTRATION_FLAG_CPHS_SPN)) {
+		if (!(netreg->flags & (NETWORK_REGISTRATION_FLAG_CPHS_SPN |
+				NETWORK_REGISTRATION_FLAG_CPHS_SPN_SHORT))) {
 
 			netreg->flags |= NETWORK_REGISTRATION_FLAG_CPHS_SPN;
 
@@ -1731,11 +1732,32 @@ static void sim_spn_read_cb(int ok, int length, int record,
 					SIM_EF_CPHS_SPN_FILEID,
 					OFONO_SIM_FILE_STRUCTURE_TRANSPARENT,
 					sim_spn_read_cb, netreg);
+			return;
 		}
+
+		if (netreg->flags & NETWORK_REGISTRATION_FLAG_CPHS_SPN) {
+
+			netreg->flags |= NETWORK_REGISTRATION_FLAG_CPHS_SPN;
+			netreg->flags |=
+				NETWORK_REGISTRATION_FLAG_CPHS_SPN_SHORT;
+
+			ofono_sim_read(netreg->sim_context,
+					SIM_EF_CPHS_SPN_SHORT_FILEID,
+					OFONO_SIM_FILE_STRUCTURE_TRANSPARENT,
+					sim_spn_read_cb, netreg);
+			return;
+
+		}
+
+		if (netreg->flags & NETWORK_REGISTRATION_FLAG_CPHS_SPN_SHORT)
+			netreg->flags |=
+				NETWORK_REGISTRATION_FLAG_CPHS_SPN_SHORT;
+
 		return;
 	}
 
-	if (!(netreg->flags & NETWORK_REGISTRATION_FLAG_CPHS_SPN)) {
+	if (!(netreg->flags & (NETWORK_REGISTRATION_FLAG_CPHS_SPN |
+			NETWORK_REGISTRATION_FLAG_CPHS_SPN_SHORT))) {
 		dcbyte = data;
 		offset = 1;
 	}
@@ -1743,7 +1765,8 @@ static void sim_spn_read_cb(int ok, int length, int record,
 	if (!sim_spn_parse(data + offset, length - offset, &netreg->spn))
 		return;
 
-	if (!(netreg->flags & NETWORK_REGISTRATION_FLAG_CPHS_SPN))
+	if (!(netreg->flags & (NETWORK_REGISTRATION_FLAG_CPHS_SPN |
+			NETWORK_REGISTRATION_FLAG_CPHS_SPN_SHORT)))
 		ofono_sim_read(netreg->sim_context, SIM_EFSPDI_FILEID,
 				OFONO_SIM_FILE_STRUCTURE_TRANSPARENT,
 				sim_spdi_read_cb, netreg);
-- 
1.7.5.4

[PATCH 9/9] TODO: Remove completed CPHS SPN and short-SPN tasks

[Oleg Zhurakivskyy]

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

---
 TODO |   14 --------------
 1 files changed, 0 insertions(+), 14 deletions(-)

diff --git a/TODO b/TODO
index 159b325..a7dfca8 100644
--- a/TODO
+++ b/TODO
@@ -102,20 +102,6 @@ SIM / SIM File system
   Priority: Low
   Complexity: C4
 
-- CPHS SPN support
-
-  Support reading of the CPHS-defined SPN field from SIMs
-
-  Priority: High
-  Complexity: C1
-
-- CPHS Short SPN support
-
-  Support reading of the CPHS-defined short SPN field from SIMs
-
-  Priority: Medium
-  Complexity: C1
-
 
 Bluetooth HFP
 =============
-- 
1.7.5.4

Re: [PATCH 1/9] network: M9 coding style corrections

[Denis Kenzior]

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

Hi Oleg,

On 12/02/2011 05:10 AM, Oleg Zhurakivskyy wrote:
> ---
>  src/network.c |   10 +++++-----
>  1 files changed, 5 insertions(+), 5 deletions(-)
> 

Patch has been applied, thanks.

Regards,
-Denis

Re: [PATCH 2/9] network: Refactor sim_spn_read_cb()

[Denis Kenzior]

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

Hi Oleg,

On 12/02/2011 05:10 AM, Oleg Zhurakivskyy wrote:
> Refactor sim_spn_read_cb() for CPHS SPN, short-SPN inclusion
> ---
>  src/network.c |   82 +++++++++++++++++++++++++++++++++-----------------------
>  1 files changed, 48 insertions(+), 34 deletions(-)
> 

Patch has been applied, but I did tweak the new naming slightly in a
follow on commit.

Regards,
-Denis

Re: [PATCH 3/9] network: Rename spname to spn

[Denis Kenzior]

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

Hi Oleg,

On 12/02/2011 05:10 AM, Oleg Zhurakivskyy wrote:
> ---
>  src/network.c |   18 +++++++++---------
>  1 files changed, 9 insertions(+), 9 deletions(-)
> 

Patch has been applied, thanks.

Regards,
-Denis

Re: [PATCH 4/9] gprs-provision: Fix crash if no SPN present

[Denis Kenzior]

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

Hi Oleg,

On 12/02/2011 05:10 AM, Oleg Zhurakivskyy wrote:
> ---
>  examples/provision.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 

Patch has been applied, thanks.

Regards,
-Denis

Re: [PATCH 5/9] simutil: Add CPHS SPN and short-SPN IDs

[Denis Kenzior]

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

Hi Oleg,

On 12/02/2011 05:10 AM, Oleg Zhurakivskyy wrote:
> ---
>  src/simutil.h |    2 ++
>  1 files changed, 2 insertions(+), 0 deletions(-)
> 

Patch has been applied, but you did forget to update the EF database for
these entries, which I fixed for you in a follow on commit.

Regards,
-Denis

Re: [PATCH 7/9] network: Add CPHS SPN fallback

[Denis Kenzior]

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

Hi Oleg,

On 12/02/2011 05:10 AM, Oleg Zhurakivskyy wrote:
> ---
>  src/network.c |   32 ++++++++++++++++++++++++--------
>  1 files changed, 24 insertions(+), 8 deletions(-)
> 
> diff --git a/src/network.c b/src/network.c
> index 6e08876..1354edf 100644
> --- a/src/network.c
> +++ b/src/network.c
> @@ -1719,21 +1719,37 @@ static void sim_spn_read_cb(int ok, int length, int record,
>  				int record_length, void *user_data)
>  {
>  	struct ofono_netreg *netreg = user_data;
> -	unsigned char dcbyte;
> +	const unsigned char *dcbyte = NULL;
> +	int offset = 0;
>  
> -	if (!ok)
> +	if (!ok) {
> +		if (!(netreg->flags & NETWORK_REGISTRATION_FLAG_CPHS_SPN)) {
> +
> +			netreg->flags |= NETWORK_REGISTRATION_FLAG_CPHS_SPN;
> +
> +			ofono_sim_read(netreg->sim_context,
> +					SIM_EF_CPHS_SPN_FILEID,
> +					OFONO_SIM_FILE_STRUCTURE_TRANSPARENT,
> +					sim_spn_read_cb, netreg);
> +		}
>  		return;
> +	}
>  
> -	dcbyte = data[0];
> +	if (!(netreg->flags & NETWORK_REGISTRATION_FLAG_CPHS_SPN)) {
> +		dcbyte = data;
> +		offset = 1;
> +	}
>  
> -	if (!sim_spn_parse(data + 1, length - 1, &netreg->spn))
> +	if (!sim_spn_parse(data + offset, length - offset, &netreg->spn))
>  		return;
>  
> -	ofono_sim_read(netreg->sim_context, SIM_EFSPDI_FILEID,
> -			OFONO_SIM_FILE_STRUCTURE_TRANSPARENT,
> -			sim_spdi_read_cb, netreg);
> +	if (!(netreg->flags & NETWORK_REGISTRATION_FLAG_CPHS_SPN))
> +		ofono_sim_read(netreg->sim_context, SIM_EFSPDI_FILEID,
> +				OFONO_SIM_FILE_STRUCTURE_TRANSPARENT,
> +				sim_spdi_read_cb, netreg);
>  
> -	sim_spn_display_condition_parse(netreg, dcbyte);
> +	if (dcbyte != NULL)
> +		sim_spn_display_condition_parse(netreg, *dcbyte);
>  
>  	if (netreg->current_operator)
>  		ofono_netreg_operator_display_name_notify(netreg);

Actually I don't like this approach.  There are some interactions with
the SIM refresh logic triggered by sim_spn_changed and this approach
makes it very hard to puzzle out whether all cases are covered properly.
 Also, the preference is always towards clearer, more readable code,
even if at the expense of being more verbose.  Given how tiny
sim_spn_read_cb is now, I'd rather you handle the CPHS variants separately.

You are also not taking care of the CAT/SAT Refresh cases for the CPHS
variants.

Thinking more about it, the way I'd try to do this is to look up the
necessary bits in the CPHS Information Service Table (CPHS 4.2 Section
B.3.1.1).  Then only read CPHS SPN if the service is active / allocated.
 Only read the short form if CPHS SPN is not available and the short SPN is.

Regards,
-Denis

Re: [PATCH 7/9] network: Add CPHS SPN fallback

[Oleg Zhurakivskyy]

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

Hello Denis,

On 12/03/2011 12:35 AM, Denis Kenzior wrote:
> Actually I don't like this approach.  There are some interactions with
> the SIM refresh logic triggered by sim_spn_changed and this approach
> makes it very hard to puzzle out whether all cases are covered properly.
>   Also, the preference is always towards clearer, more readable code,
> even if at the expense of being more verbose.  Given how tiny
> sim_spn_read_cb is now, I'd rather you handle the CPHS variants separately.
>
> You are also not taking care of the CAT/SAT Refresh cases for the CPHS
> variants.
>
> Thinking more about it, the way I'd try to do this is to look up the
> necessary bits in the CPHS Information Service Table (CPHS 4.2 Section
> B.3.1.1).  Then only read CPHS SPN if the service is active / allocated.
>   Only read the short form if CPHS SPN is not available and the short SPN is.

Thanks for the review and ideas here. Handling CPHS variants separately would be 
cleaner and makes sense. Checking the CPHS Information Service Table will help 
to make the logic simpler too, I will take this approach. Also will check 
CAT/SAT Refresh cases for CPHS variants.

I will rework and prepare another patch.

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

Re: [PATCH 5/9] simutil: Add CPHS SPN and short-SPN IDs

[Oleg Zhurakivskyy]

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

Hello Denis,
On 12/03/2011 12:04 AM, Denis Kenzior wrote:
> Patch has been applied, but you did forget to update the EF database for
> these entries, which I fixed for you in a follow on commit.

Sorry for missing the EF database update and thanks for the fix and help!

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