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
_______________________________________________
ofono mailing list
[email protected]
http://lists.ofono.org/listinfo/ofono
