[PATCH] Fix gprs provisioning for some SIM (non-USIM) cards

[PATCH] Fix gprs provisioning for some SIM (non-USIM) cards

[Alfonso Sanchez-Beato]

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

In some cases the field MNC length in EFad is not present. MNC can
have either 2 or 3 digits depending on the MCC: we will try with
those 2 lengths when searching in the database for this corner case
(this situation can happen for SIMs (non-USIM), as MNC lenght field
is not mandatory for them - see 3gpp TS 51.011)
---
 include/sim.h |  1 +
 src/gprs.c    | 26 +++++++++++++++++++++----
 src/sim.c     | 61 ++++++++++++++++++++++++++++++++++++++++-------------------
 3 files changed, 65 insertions(+), 23 deletions(-)

diff --git a/include/sim.h b/include/sim.h
index ed850f9..f63324a 100644
--- a/include/sim.h
+++ b/include/sim.h
@@ -191,6 +191,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);
+unsigned ofono_sim_get_mnc_length(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);
 
diff --git a/src/gprs.c b/src/gprs.c
index e379f7b..0218696 100644
--- a/src/gprs.c
+++ b/src/gprs.c
@@ -2967,7 +2967,7 @@ static void provision_context(const struct ofono_gprs_provision_data *ap,
 	gprs->contexts = g_slist_append(gprs->contexts, context);
 }
 
-static void provision_contexts(struct ofono_gprs *gprs, const char *mcc,
+static int provision_contexts(struct ofono_gprs *gprs, const char *mcc,
 				const char *mnc, const char *spn)
 {
 	struct ofono_gprs_provision_data *settings;
@@ -2977,13 +2977,15 @@ static void provision_contexts(struct ofono_gprs *gprs, const char *mcc,
 	if (__ofono_gprs_provision_get_settings(mcc, mnc, spn,
 						&settings, &count) == FALSE) {
 		ofono_warn("Provisioning failed");
-		return;
+		return -EINVAL;
 	}
 
 	for (i = 0; i < count; i++)
 		provision_context(&settings[i], gprs);
 
 	__ofono_gprs_provision_free_settings(settings, count);
+
+	return 0;
 }
 
 static void ofono_gprs_finish_register(struct ofono_gprs *gprs)
@@ -3021,9 +3023,25 @@ static void spn_read_cb(const char *spn, const char *dc, void *data)
 	struct ofono_gprs *gprs	= data;
 	struct ofono_modem *modem = __ofono_atom_get_modem(gprs->atom);
 	struct ofono_sim *sim = __ofono_atom_find(OFONO_ATOM_TYPE_SIM, modem);
+	const char *mnc = ofono_sim_get_mnc(sim);
+	const char *mcc = ofono_sim_get_mcc(sim);
+
+	if (ofono_sim_get_mnc_length(sim) == 0 && mnc != NULL) {
+		/* MNC length not found in EFad: we try with length 3 and 2 */
+		char aux_mnc[OFONO_MAX_MNC_LENGTH + 1];
 
-	provision_contexts(gprs, ofono_sim_get_mcc(sim),
-					ofono_sim_get_mnc(sim), spn);
+		strncpy(aux_mnc, mnc, OFONO_MAX_MNC_LENGTH);
+		aux_mnc[OFONO_MAX_MNC_LENGTH] = '\0';
+
+		if (provision_contexts(gprs, mcc, aux_mnc, spn) != 0) {
+			ofono_info("MNC length is not 3: trying length 2");
+			aux_mnc[OFONO_MAX_MNC_LENGTH - 1] = '\0';
+			provision_contexts(gprs, mcc, aux_mnc, spn);
+		}
+
+	} else {
+		provision_contexts(gprs, mcc, mnc, spn);
+	}
 
 	ofono_sim_remove_spn_watch(sim, &gprs->spn_watch);
 
diff --git a/src/sim.c b/src/sim.c
index edae5eb..3d31386 100644
--- a/src/sim.c
+++ b/src/sim.c
@@ -1427,6 +1427,8 @@ static void sim_imsi_obtained(struct ofono_sim *sim, const char *imsi)
 {
 	DBusConnection *conn = ofono_dbus_get_connection();
 	const char *path = __ofono_atom_get_path(sim->atom);
+	unsigned char aux_mnc_length;
+	const char *str;
 
 	sim->imsi = g_strdup(imsi);
 
@@ -1435,27 +1437,36 @@ static void sim_imsi_obtained(struct ofono_sim *sim, const char *imsi)
 						"SubscriberIdentity",
 						DBUS_TYPE_STRING, &sim->imsi);
 
-	if (sim->mnc_length) {
-		const char *str;
+	/*
+	 * sim->mnc_length = 0 means that EFad did not contain the MNC length
+	 * field. So we will copy 3 digits from the IMSI in the MNC. MNC can
+	 * have either 2 or 3 digits depending on the MCC: we will try with
+	 * those 2 lengths when searching in the database for this corner case
+	 * (this situation can happen for SIMs (non-USIM), as MNC lenght field
+	 * is not mandatory for them - see TS 51.011)
+	 */
+	if (sim->mnc_length)
+		aux_mnc_length = sim->mnc_length;
+	else
+		aux_mnc_length = OFONO_MAX_MNC_LENGTH;
 
-		strncpy(sim->mcc, sim->imsi, OFONO_MAX_MCC_LENGTH);
-		sim->mcc[OFONO_MAX_MCC_LENGTH] = '\0';
-		strncpy(sim->mnc, sim->imsi + OFONO_MAX_MCC_LENGTH,
-			sim->mnc_length);
-		sim->mnc[sim->mnc_length] = '\0';
+	strncpy(sim->mcc, sim->imsi, OFONO_MAX_MCC_LENGTH);
+	sim->mcc[OFONO_MAX_MCC_LENGTH] = '\0';
+	strncpy(sim->mnc, sim->imsi + OFONO_MAX_MCC_LENGTH,
+		aux_mnc_length);
+	sim->mnc[aux_mnc_length] = '\0';
 
-		str = sim->mcc;
-		ofono_dbus_signal_property_changed(conn, path,
-						OFONO_SIM_MANAGER_INTERFACE,
-						"MobileCountryCode",
-						DBUS_TYPE_STRING, &str);
+	str = sim->mcc;
+	ofono_dbus_signal_property_changed(conn, path,
+					OFONO_SIM_MANAGER_INTERFACE,
+					"MobileCountryCode",
+					DBUS_TYPE_STRING, &str);
 
-		str = sim->mnc;
-		ofono_dbus_signal_property_changed(conn, path,
-						OFONO_SIM_MANAGER_INTERFACE,
-						"MobileNetworkCode",
-						DBUS_TYPE_STRING, &str);
-	}
+	str = sim->mnc;
+	ofono_dbus_signal_property_changed(conn, path,
+					OFONO_SIM_MANAGER_INTERFACE,
+					"MobileNetworkCode",
+					DBUS_TYPE_STRING, &str);
 
 	sim_set_ready(sim);
 
@@ -1772,8 +1783,12 @@ static void sim_ad_read_cb(int ok, int length, int record,
 	if (!ok)
 		return;
 
+	if (length < 3) {
+		ofono_error("EFad should contain at least three bytes");
+		return;
+	}
 	if (length < 4) {
-		ofono_error("EFad should contain@least four bytes");
+		ofono_info("EFad does not contain MNC length");
 		return;
 	}
 
@@ -2234,6 +2249,14 @@ const char *ofono_sim_get_mnc(struct ofono_sim *sim)
 	return sim->mnc;
 }
 
+unsigned ofono_sim_get_mnc_length(struct ofono_sim *sim)
+{
+	if (sim == NULL)
+		return 0;
+
+	return sim->mnc_length;
+}
+
 const char *ofono_sim_get_spn(struct ofono_sim *sim)
 {
 	if (sim == NULL)
-- 
1.8.1.2

Re: [PATCH] Fix gprs provisioning for some SIM (non-USIM) cards

[Jonas Bonn]

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

On 10/16/2013 12:55 PM, Alfonso Sanchez-Beato wrote:
> In some cases the field MNC length in EFad is not present. MNC can
> have either 2 or 3 digits depending on the MCC: we will try with
> those 2 lengths when searching in the database for this corner case
> (this situation can happen for SIMs (non-USIM), as MNC lenght field
> is not mandatory for them - see 3gpp TS 51.011)

We've been here before...

http://comments.gmane.org/gmane.comp.handhelds.ofono/13552

Something like this is definitely needed... SIM's from at least one 
carrier in Sweden fail to provide the MNC length.

/Jonas


> ---
>   include/sim.h |  1 +
>   src/gprs.c    | 26 +++++++++++++++++++++----
>   src/sim.c     | 61 ++++++++++++++++++++++++++++++++++++++++-------------------
>   3 files changed, 65 insertions(+), 23 deletions(-)
>
> diff --git a/include/sim.h b/include/sim.h
> index ed850f9..f63324a 100644
> --- a/include/sim.h
> +++ b/include/sim.h
> @@ -191,6 +191,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);
> +unsigned ofono_sim_get_mnc_length(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);
>
> diff --git a/src/gprs.c b/src/gprs.c
> index e379f7b..0218696 100644
> --- a/src/gprs.c
> +++ b/src/gprs.c
> @@ -2967,7 +2967,7 @@ static void provision_context(const struct ofono_gprs_provision_data *ap,
>   	gprs->contexts = g_slist_append(gprs->contexts, context);
>   }
>
> -static void provision_contexts(struct ofono_gprs *gprs, const char *mcc,
> +static int provision_contexts(struct ofono_gprs *gprs, const char *mcc,
>   				const char *mnc, const char *spn)
>   {
>   	struct ofono_gprs_provision_data *settings;
> @@ -2977,13 +2977,15 @@ static void provision_contexts(struct ofono_gprs *gprs, const char *mcc,
>   	if (__ofono_gprs_provision_get_settings(mcc, mnc, spn,
>   						&settings, &count) == FALSE) {
>   		ofono_warn("Provisioning failed");
> -		return;
> +		return -EINVAL;
>   	}
>
>   	for (i = 0; i < count; i++)
>   		provision_context(&settings[i], gprs);
>
>   	__ofono_gprs_provision_free_settings(settings, count);
> +
> +	return 0;
>   }
>
>   static void ofono_gprs_finish_register(struct ofono_gprs *gprs)
> @@ -3021,9 +3023,25 @@ static void spn_read_cb(const char *spn, const char *dc, void *data)
>   	struct ofono_gprs *gprs	= data;
>   	struct ofono_modem *modem = __ofono_atom_get_modem(gprs->atom);
>   	struct ofono_sim *sim = __ofono_atom_find(OFONO_ATOM_TYPE_SIM, modem);
> +	const char *mnc = ofono_sim_get_mnc(sim);
> +	const char *mcc = ofono_sim_get_mcc(sim);
> +
> +	if (ofono_sim_get_mnc_length(sim) == 0 && mnc != NULL) {
> +		/* MNC length not found in EFad: we try with length 3 and 2 */
> +		char aux_mnc[OFONO_MAX_MNC_LENGTH + 1];
>
> -	provision_contexts(gprs, ofono_sim_get_mcc(sim),
> -					ofono_sim_get_mnc(sim), spn);
> +		strncpy(aux_mnc, mnc, OFONO_MAX_MNC_LENGTH);
> +		aux_mnc[OFONO_MAX_MNC_LENGTH] = '\0';
> +
> +		if (provision_contexts(gprs, mcc, aux_mnc, spn) != 0) {
> +			ofono_info("MNC length is not 3: trying length 2");
> +			aux_mnc[OFONO_MAX_MNC_LENGTH - 1] = '\0';
> +			provision_contexts(gprs, mcc, aux_mnc, spn);
> +		}
> +
> +	} else {
> +		provision_contexts(gprs, mcc, mnc, spn);
> +	}
>
>   	ofono_sim_remove_spn_watch(sim, &gprs->spn_watch);
>
> diff --git a/src/sim.c b/src/sim.c
> index edae5eb..3d31386 100644
> --- a/src/sim.c
> +++ b/src/sim.c
> @@ -1427,6 +1427,8 @@ static void sim_imsi_obtained(struct ofono_sim *sim, const char *imsi)
>   {
>   	DBusConnection *conn = ofono_dbus_get_connection();
>   	const char *path = __ofono_atom_get_path(sim->atom);
> +	unsigned char aux_mnc_length;
> +	const char *str;
>
>   	sim->imsi = g_strdup(imsi);
>
> @@ -1435,27 +1437,36 @@ static void sim_imsi_obtained(struct ofono_sim *sim, const char *imsi)
>   						"SubscriberIdentity",
>   						DBUS_TYPE_STRING, &sim->imsi);
>
> -	if (sim->mnc_length) {
> -		const char *str;
> +	/*
> +	 * sim->mnc_length = 0 means that EFad did not contain the MNC length
> +	 * field. So we will copy 3 digits from the IMSI in the MNC. MNC can
> +	 * have either 2 or 3 digits depending on the MCC: we will try with
> +	 * those 2 lengths when searching in the database for this corner case
> +	 * (this situation can happen for SIMs (non-USIM), as MNC lenght field
> +	 * is not mandatory for them - see TS 51.011)
> +	 */
> +	if (sim->mnc_length)
> +		aux_mnc_length = sim->mnc_length;
> +	else
> +		aux_mnc_length = OFONO_MAX_MNC_LENGTH;
>
> -		strncpy(sim->mcc, sim->imsi, OFONO_MAX_MCC_LENGTH);
> -		sim->mcc[OFONO_MAX_MCC_LENGTH] = '\0';
> -		strncpy(sim->mnc, sim->imsi + OFONO_MAX_MCC_LENGTH,
> -			sim->mnc_length);
> -		sim->mnc[sim->mnc_length] = '\0';
> +	strncpy(sim->mcc, sim->imsi, OFONO_MAX_MCC_LENGTH);
> +	sim->mcc[OFONO_MAX_MCC_LENGTH] = '\0';
> +	strncpy(sim->mnc, sim->imsi + OFONO_MAX_MCC_LENGTH,
> +		aux_mnc_length);
> +	sim->mnc[aux_mnc_length] = '\0';
>
> -		str = sim->mcc;
> -		ofono_dbus_signal_property_changed(conn, path,
> -						OFONO_SIM_MANAGER_INTERFACE,
> -						"MobileCountryCode",
> -						DBUS_TYPE_STRING, &str);
> +	str = sim->mcc;
> +	ofono_dbus_signal_property_changed(conn, path,
> +					OFONO_SIM_MANAGER_INTERFACE,
> +					"MobileCountryCode",
> +					DBUS_TYPE_STRING, &str);
>
> -		str = sim->mnc;
> -		ofono_dbus_signal_property_changed(conn, path,
> -						OFONO_SIM_MANAGER_INTERFACE,
> -						"MobileNetworkCode",
> -						DBUS_TYPE_STRING, &str);
> -	}
> +	str = sim->mnc;
> +	ofono_dbus_signal_property_changed(conn, path,
> +					OFONO_SIM_MANAGER_INTERFACE,
> +					"MobileNetworkCode",
> +					DBUS_TYPE_STRING, &str);
>
>   	sim_set_ready(sim);
>
> @@ -1772,8 +1783,12 @@ static void sim_ad_read_cb(int ok, int length, int record,
>   	if (!ok)
>   		return;
>
> +	if (length < 3) {
> +		ofono_error("EFad should contain at least three bytes");
> +		return;
> +	}
>   	if (length < 4) {
> -		ofono_error("EFad should contain at least four bytes");
> +		ofono_info("EFad does not contain MNC length");
>   		return;
>   	}
>
> @@ -2234,6 +2249,14 @@ const char *ofono_sim_get_mnc(struct ofono_sim *sim)
>   	return sim->mnc;
>   }
>
> +unsigned ofono_sim_get_mnc_length(struct ofono_sim *sim)
> +{
> +	if (sim == NULL)
> +		return 0;
> +
> +	return sim->mnc_length;
> +}
> +
>   const char *ofono_sim_get_spn(struct ofono_sim *sim)
>   {
>   	if (sim == NULL)
>

Re: [PATCH] Fix gprs provisioning for some SIM (non-USIM) cards

[Denis Kenzior]

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

Hi Alfonso,

On 10/16/2013 05:55 AM, Alfonso Sanchez-Beato wrote:
> In some cases the field MNC length in EFad is not present. MNC can
> have either 2 or 3 digits depending on the MCC: we will try with
> those 2 lengths when searching in the database for this corner case
> (this situation can happen for SIMs (non-USIM), as MNC lenght field
> is not mandatory for them - see 3gpp TS 51.011)

Where are you finding SIMs these days?  Are you raiding a museum? :)

> ---
>   include/sim.h |  1 +
>   src/gprs.c    | 26 +++++++++++++++++++++----
>   src/sim.c     | 61 ++++++++++++++++++++++++++++++++++++++++-------------------
>   3 files changed, 65 insertions(+), 23 deletions(-)
>

On a serious note, please review the 'Submitting Patches' section in the 
oFono HACKING document.

> diff --git a/include/sim.h b/include/sim.h
> index ed850f9..f63324a 100644
> --- a/include/sim.h
> +++ b/include/sim.h
> @@ -191,6 +191,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);
> +unsigned ofono_sim_get_mnc_length(struct ofono_sim *sim);

This is a definite 'no way are we exposing this'

>   const char *ofono_sim_get_spn(struct ofono_sim *sim);
>   enum ofono_sim_phase ofono_sim_get_phase(struct ofono_sim *sim);
>
> diff --git a/src/gprs.c b/src/gprs.c
> index e379f7b..0218696 100644
> --- a/src/gprs.c
> +++ b/src/gprs.c
> @@ -2967,7 +2967,7 @@ static void provision_context(const struct ofono_gprs_provision_data *ap,
>   	gprs->contexts = g_slist_append(gprs->contexts, context);
>   }
>
> -static void provision_contexts(struct ofono_gprs *gprs, const char *mcc,
> +static int provision_contexts(struct ofono_gprs *gprs, const char *mcc,
>   				const char *mnc, const char *spn)
>   {
>   	struct ofono_gprs_provision_data *settings;
> @@ -2977,13 +2977,15 @@ static void provision_contexts(struct ofono_gprs *gprs, const char *mcc,
>   	if (__ofono_gprs_provision_get_settings(mcc, mnc, spn,
>   						&settings, &count) == FALSE) {
>   		ofono_warn("Provisioning failed");
> -		return;
> +		return -EINVAL;
>   	}
>
>   	for (i = 0; i < count; i++)
>   		provision_context(&settings[i], gprs);
>
>   	__ofono_gprs_provision_free_settings(settings, count);
> +
> +	return 0;
>   }
>
>   static void ofono_gprs_finish_register(struct ofono_gprs *gprs)
> @@ -3021,9 +3023,25 @@ static void spn_read_cb(const char *spn, const char *dc, void *data)
>   	struct ofono_gprs *gprs	= data;
>   	struct ofono_modem *modem = __ofono_atom_get_modem(gprs->atom);
>   	struct ofono_sim *sim = __ofono_atom_find(OFONO_ATOM_TYPE_SIM, modem);
> +	const char *mnc = ofono_sim_get_mnc(sim);
> +	const char *mcc = ofono_sim_get_mcc(sim);
> +
> +	if (ofono_sim_get_mnc_length(sim) == 0 && mnc != NULL) {
> +		/* MNC length not found in EFad: we try with length 3 and 2 */
> +		char aux_mnc[OFONO_MAX_MNC_LENGTH + 1];
>
> -	provision_contexts(gprs, ofono_sim_get_mcc(sim),
> -					ofono_sim_get_mnc(sim), spn);
> +		strncpy(aux_mnc, mnc, OFONO_MAX_MNC_LENGTH);
> +		aux_mnc[OFONO_MAX_MNC_LENGTH] = '\0';
> +
> +		if (provision_contexts(gprs, mcc, aux_mnc, spn) != 0) {
> +			ofono_info("MNC length is not 3: trying length 2");
> +			aux_mnc[OFONO_MAX_MNC_LENGTH - 1] = '\0';
> +			provision_contexts(gprs, mcc, aux_mnc, spn);
> +		}
> +
> +	} else {
> +		provision_contexts(gprs, mcc, mnc, spn);
> +	}

This is a NAK.  You can't just try things randomly and see what sticks. 
  It is not a good strategy in a critical subsystem.  If you cannot make 
a good decision, it is far safer to let the user decide, which will 
happen in the first place.

The MNC can only be one value and that value can even change depending 
on whether you're operating the same SIM in a 2G device or 3G device. 
So trying length 3 when in reality it is 2 can and most likely will get 
you into trouble.

Moreover, the core cannot assume that SIM reading functionality has been 
enabled in the driver, so you're just making assumptions that things 
failed because you're running on a 2G SIM.

>
>   	ofono_sim_remove_spn_watch(sim, &gprs->spn_watch);
>
> diff --git a/src/sim.c b/src/sim.c
> index edae5eb..3d31386 100644
> --- a/src/sim.c
> +++ b/src/sim.c
> @@ -1427,6 +1427,8 @@ static void sim_imsi_obtained(struct ofono_sim *sim, const char *imsi)
>   {
>   	DBusConnection *conn = ofono_dbus_get_connection();
>   	const char *path = __ofono_atom_get_path(sim->atom);
> +	unsigned char aux_mnc_length;
> +	const char *str;
>
>   	sim->imsi = g_strdup(imsi);
>
> @@ -1435,27 +1437,36 @@ static void sim_imsi_obtained(struct ofono_sim *sim, const char *imsi)
>   						"SubscriberIdentity",
>   						DBUS_TYPE_STRING, &sim->imsi);
>
> -	if (sim->mnc_length) {
> -		const char *str;
> +	/*
> +	 * sim->mnc_length = 0 means that EFad did not contain the MNC length
> +	 * field. So we will copy 3 digits from the IMSI in the MNC. MNC can
> +	 * have either 2 or 3 digits depending on the MCC: we will try with
> +	 * those 2 lengths when searching in the database for this corner case
> +	 * (this situation can happen for SIMs (non-USIM), as MNC lenght field
> +	 * is not mandatory for them - see TS 51.011)
> +	 */
> +	if (sim->mnc_length)
> +		aux_mnc_length = sim->mnc_length;
> +	else
> +		aux_mnc_length = OFONO_MAX_MNC_LENGTH;
>
> -		strncpy(sim->mcc, sim->imsi, OFONO_MAX_MCC_LENGTH);
> -		sim->mcc[OFONO_MAX_MCC_LENGTH] = '\0';
> -		strncpy(sim->mnc, sim->imsi + OFONO_MAX_MCC_LENGTH,
> -			sim->mnc_length);
> -		sim->mnc[sim->mnc_length] = '\0';
> +	strncpy(sim->mcc, sim->imsi, OFONO_MAX_MCC_LENGTH);
> +	sim->mcc[OFONO_MAX_MCC_LENGTH] = '\0';
> +	strncpy(sim->mnc, sim->imsi + OFONO_MAX_MCC_LENGTH,
> +		aux_mnc_length);
> +	sim->mnc[aux_mnc_length] = '\0';
>
> -		str = sim->mcc;
> -		ofono_dbus_signal_property_changed(conn, path,
> -						OFONO_SIM_MANAGER_INTERFACE,
> -						"MobileCountryCode",
> -						DBUS_TYPE_STRING, &str);
> +	str = sim->mcc;
> +	ofono_dbus_signal_property_changed(conn, path,
> +					OFONO_SIM_MANAGER_INTERFACE,
> +					"MobileCountryCode",
> +					DBUS_TYPE_STRING, &str);
>
> -		str = sim->mnc;
> -		ofono_dbus_signal_property_changed(conn, path,
> -						OFONO_SIM_MANAGER_INTERFACE,
> -						"MobileNetworkCode",
> -						DBUS_TYPE_STRING, &str);
> -	}
> +	str = sim->mnc;
> +	ofono_dbus_signal_property_changed(conn, path,
> +					OFONO_SIM_MANAGER_INTERFACE,
> +					"MobileNetworkCode",
> +					DBUS_TYPE_STRING, &str);

Again, you cannot make the assumption that MNC length is not known 
because EFad was on a 2G sim.  It may be that the driver just does not 
implement SIM read operations, or the hardware does not allow them.

So you're just making stuff up here.

>
>   	sim_set_ready(sim);
>
> @@ -1772,8 +1783,12 @@ static void sim_ad_read_cb(int ok, int length, int record,
>   	if (!ok)
>   		return;
>
> +	if (length < 3) {
> +		ofono_error("EFad should contain at least three bytes");
> +		return;
> +	}
>   	if (length < 4) {
> -		ofono_error("EFad should contain at least four bytes");
> +		ofono_info("EFad does not contain MNC length");
>   		return;
>   	}

I'm fine with this chunk since in 2G sims the length is indeed only 3.

>
> @@ -2234,6 +2249,14 @@ const char *ofono_sim_get_mnc(struct ofono_sim *sim)
>   	return sim->mnc;
>   }
>
> +unsigned ofono_sim_get_mnc_length(struct ofono_sim *sim)
> +{
> +	if (sim == NULL)
> +		return 0;
> +
> +	return sim->mnc_length;
> +}
> +
>   const char *ofono_sim_get_spn(struct ofono_sim *sim)
>   {
>   	if (sim == NULL)
>

Regards,
-Denis

Re: [PATCH] Fix gprs provisioning for some SIM (non-USIM) cards

[Denis Kenzior]

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

Hi Jonas,

On 10/16/2013 06:19 AM, Jonas Bonn wrote:
> On 10/16/2013 12:55 PM, Alfonso Sanchez-Beato wrote:
>> In some cases the field MNC length in EFad is not present. MNC can
>> have either 2 or 3 digits depending on the MCC: we will try with
>> those 2 lengths when searching in the database for this corner case
>> (this situation can happen for SIMs (non-USIM), as MNC lenght field
>> is not mandatory for them - see 3gpp TS 51.011)
>
> We've been here before...
>
> http://comments.gmane.org/gmane.comp.handhelds.ofono/13552
>
> Something like this is definitely needed... SIM's from at least one
> carrier in Sweden fail to provide the MNC length.
>

Feel free to improve your patch to be a bit more pedantic:
- checking for length == 3
- making sure that EFphase was set to something sane.

I might be convinced to default to MNC length 2 in that case

Regards,
-Denis

Re: [PATCH] Fix gprs provisioning for some SIM (non-USIM) cards

[Alfonso Sanchez-Beato]

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

Hi,


> We've been here before...
>
> http://comments.gmane.org/gmane.comp.handhelds.ofono/13552
>
> Something like this is definitely needed... SIM's from at least one
> carrier in Sweden fail to provide the MNC length.

The SIM that showed the bug in our case was from a guy in Pakistan. So
it seems that these SIMs are used by more operators than one would
expect.

> The MNC can only be one value and that value can even change depending on whether you're
> operating the same SIM in a 2G device or 3G device. So trying length 3 when in reality it is 2
> can and most likely will get you into trouble.

I do not think this can happen. For a given MCC, the length of the MNC
is always the same, or at least that is the recommendation of the ITU.

> I might be convinced to default to MNC length 2 in that case
>
> Regards,
> -Denis


 I do not think this is a good idea. Taking a look the ITU database,
countries like USA, Canada, or Colombia have MNC length == 3.

As the relationship between MCC and MNC is deterministic, when we know
the MCC we can directly find out the MNC length. I agree in that
trying first length 2, then 3 is not very elegant. But if we do not do
that I see two possible solutions:

1.- Store a table statically or in a file with the correspondence
between MCC and MNC length. I foresee maintenance problems with this
approach.

2.- Add a function in the provision driver so we can ask for the MNC
length for a given MCC, which would be invoked when MNC length is not
found in EFad. The implementation would search in the provisioning DB:
it could return the length of the first MNC found for that MCC (it
must be the same for all networks for that MCC).

Opinions on this?

Thanks,
Alfonso

Re: [PATCH] Fix gprs provisioning for some SIM (non-USIM) cards

[Denis Kenzior]

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

Hi Alfonso,

 >> The MNC can only be one value and that value can even change 
depending on whether you're
>> operating the same SIM in a 2G device or 3G device. So trying length 3 when in reality it is 2
>> can and most likely will get you into trouble.
>
> I do not think this can happen. For a given MCC, the length of the MNC
> is always the same, or at least that is the recommendation of the ITU.
>

The MNC in the IMSI remains the same, but the network operator MNC can 
change.  For example, T-Mobile used to report its 2G network MCC/MNC as 
310 26 even though its MNC is 260.  Then there are countries that run 
mixed 2 and 3 length MNCs...

Regardless, my main point here is that without properly coded EFad data 
we cannot reliably tell what MNC length to use.

>> I might be convinced to default to MNC length 2 in that case
>>
>
>   I do not think this is a good idea. Taking a look the ITU database,
> countries like USA, Canada, or Colombia have MNC length == 3.
>

I said I might be convinced.  If the phase is set and the EFad length is 
3.  The SIMs from NA operators tend to omit EFad completely or populate 
it properly.  Still, the safest approach is that we should not report 
the MNC if the SIM doesn't tell us explicitly.

> As the relationship between MCC and MNC is deterministic, when we know
> the MCC we can directly find out the MNC length. I agree in that
> trying first length 2, then 3 is not very elegant. But if we do not do
> that I see two possible solutions:

It really isn't that clear cut.  For example, check out MCC 405.

>
> 1.- Store a table statically or in a file with the correspondence
> between MCC and MNC length. I foresee maintenance problems with this
> approach.
>
> 2.- Add a function in the provision driver so we can ask for the MNC
> length for a given MCC, which would be invoked when MNC length is not
> found in EFad. The implementation would search in the provisioning DB:
> it could return the length of the first MNC found for that MCC (it
> must be the same for all networks for that MCC).
>
> Opinions on this?

I think you're trying to solve a problem that doesn't need to be solved. 
  Its 2013, I've not seen 2G sims available for ages now.  3G sims 
mandate EFad data to be properly populated.  I really fail to see the 
point, especially breaking the core to do so.

Fallback to manual user provisioning and give the user a list to choose 
from.

Regards,
-Denis

Re: [PATCH] Fix gprs provisioning for some SIM (non-USIM) cards

[Alfonso Sanchez-Beato]

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

Hi Denis,

>
> I think you're trying to solve a problem that doesn't need to be solved.
> Its 2013, I've not seen 2G sims available for ages now.  3G sims mandate
> EFad data to be properly populated.  I really fail to see the point,
> especially breaking the core to do so.

I can see that your opinion is that it is not an important issue
because it is supposed not to affect too many people.

But the fact is that at least two operators, one from Sweden and
another one from Pakistan provide (compliant) SIMs with non-fully
populated EFad.

One new SIM from a MVNO that came to my hands a few weeks ago was SIM
and not USIM (although EFad was fully populated). 3G phones do work
with SIMs, and if the operator can buy slightly cheaper cards, it
might use them. And there are many areas in the world where you just
have 2G+EDGE coverage, so the operator does not really need to provide
USIM. So they will provide SIMs, and some of them might no have full
EFad.

So I see your point, but it is not backed by numbers, and I have the
feeling that this *might* affect more users than expected, especially
if we think of Asia or Africa.

> Fallback to manual user provisioning and give the user a list to choose
> from.

I think we can make life easier for many users if we provide a
fail-safe mechanism that will be triggered only for
non-fully-populated EFad and that in that case will work in most, if
not all, the cases. I do not say that the patch I sent first would be
the solution, we can think a different way of solving this.

Regards,
Alfonso

Re: [PATCH] Fix gprs provisioning for some SIM (non-USIM) cards

[Denis Kenzior]

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

Hi Alfonso,

On 10/17/2013 11:20 AM, Alfonso Sanchez-Beato wrote:
> Hi Denis,
>
>>
>> I think you're trying to solve a problem that doesn't need to be solved.
>> Its 2013, I've not seen 2G sims available for ages now.  3G sims mandate
>> EFad data to be properly populated.  I really fail to see the point,
>> especially breaking the core to do so.
>
> I can see that your opinion is that it is not an important issue
> because it is supposed not to affect too many people.
>
> But the fact is that at least two operators, one from Sweden and
> another one from Pakistan provide (compliant) SIMs with non-fully
> populated EFad.

The real question is what exactly is happening with these SIMs.  For 
example, if a SIM is an old 2G SIM that has a length 3 EFad, then likely 
it is simply a European SIM with a 2 character MNC.

Whereas if EFad is not populated at all, then you're really in trouble.

For example, I have 2x T-Mobile SIMs on my desk and 2x AT&T SIMs.  Both 
AT&T SIMs have fully populated EFad when used in a 3G device.  When you 
insert these into a Freerunner, they both report EFad as non-existent. 
One T-Mobile 3G SIM has everything populated properly both times.  The 
other SIM reports EFad as length 3 (e.g. no MNC length is included) on a 
3G device and has no EFad whatsoever on the Freerunner.

The last SIM is an older SIM, but as you can see even well-established 
operators can get things wrong.  Being as pedantic as possible is the 
safest approach.  The core is written to be as conservative as possible 
and not try to pick things randomly.  If the SIM is configured wrongly, 
well then there's nothing we can do.

So again, you can try to guess, but you need to do this outside of the 
core.  If you want to run heuristics against IMSI based on some 
database, then just extend the provisioning API to do so.

>
> One new SIM from a MVNO that came to my hands a few weeks ago was SIM
> and not USIM (although EFad was fully populated). 3G phones do work
> with SIMs, and if the operator can buy slightly cheaper cards, it
> might use them. And there are many areas in the world where you just
> have 2G+EDGE coverage, so the operator does not really need to provide
> USIM. So they will provide SIMs, and some of them might no have full
> EFad.
>

Again, I think you're confusing the issue.  The issue is not a 2G sim vs 
3G sim.  The issue is whether EFad was populated properly.  If the 
operator bothers to include it, then likely they will put proper data in 
there, but even then you don't really know for sure.

If EFad is missing completely, well that is a different story.

> So I see your point, but it is not backed by numbers, and I have the
> feeling that this *might* affect more users than expected, especially
> if we think of Asia or Africa.
>

You have to pick your battles.  There's no way you can make this work in 
a foolproof way for every single operator out there.  Too many of them 
just do not follow standards or have not followed them in the past.

>> Fallback to manual user provisioning and give the user a list to choose
>> from.
>
> I think we can make life easier for many users if we provide a
> fail-safe mechanism that will be triggered only for
> non-fully-populated EFad and that in that case will work in most, if
> not all, the cases. I do not say that the patch I sent first would be
> the solution, we can think a different way of solving this.
>

Patches are always welcome, but please keep in mind the core policies I 
outlined.  Plugins have much less restrictions and you can get a bit 
more creative there.  If you want to maintain a MCC -> MNC length 
database based on ITU E.212, then I definitely encourage you to do so.

Regards,
-Denis

Re: [PATCH] Fix gprs provisioning for some SIM (non-USIM) cards

[Alfonso Sanchez-Beato]

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

Hi Denis,

<snip>

> So again, you can try to guess, but you need to do this outside of the core.
> If you want to run heuristics against IMSI based on some database, then just
> extend the provisioning API to do so.

<snip>

> Patches are always welcome, but please keep in mind the core policies I
> outlined.  Plugins have much less restrictions and you can get a bit more
> creative there.  If you want to maintain a MCC -> MNC length database based
> on ITU E.212, then I definitely encourage you to do so.
>
> Regards,
> -Denis

After thinking about different options, my proposition is:

* Write a new plugin called mnclenght that will find the length of the
MNC code based on the IMSI. It will use a DB of MCC/MNC
correspondences and check some special cases for some operators (this
is the approach taken in Android, btw).

* This plugin will be called by the sim core after reading the IMSI in
case the MNC length has not been set before. If a valid MNC length is
found, the MNC will be read from the IMSI and MobileNetworkCode
property will be updated in dBus.

Would this approach be okay for going upstream?

Regards,
Alfonso

Re: [PATCH] Fix gprs provisioning for some SIM (non-USIM) cards

[Denis Kenzior]

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

Hi Alfonso,

 > After thinking about different options, my proposition is:
>
> * Write a new plugin called mnclenght that will find the length of the
> MNC code based on the IMSI. It will use a DB of MCC/MNC
> correspondences and check some special cases for some operators (this
> is the approach taken in Android, btw).

Sounds good to me.

>
> * This plugin will be called by the sim core after reading the IMSI in
> case the MNC length has not been set before. If a valid MNC length is
> found, the MNC will be read from the IMSI and MobileNetworkCode
> property will be updated in dBus.
>

We shall have to see.  The only reason for exposing MCC/MNC to external 
applications is to streamline the manual user provisioning a bit.  E.g. 
if this information is provided, then some UI steps can be skipped by 
the user.  However, this information is optional.

> Would this approach be okay for going upstream?
>

In general I'd rather keep the information coming from plugins to the 
core to a minimum.  We shall decide this second point once I see the 
code.  The MCC->MNC length database would be useful regardless however, 
and can be used by the existing provisioning infrastructure.  So this 
part would definitely be welcome.

Regards,
-Denis