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

[Denis Kenzior <denkenz@gmail.com>]

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