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