From: Denis Kenzior <denkenz@gmail.com>
To: ofono@ofono.org
Subject: Re: [PATCH 3/9] sim: Watch for changes to relevant SIM files.
Date: Thu, 17 Feb 2011 15:08:20 -0600 [thread overview]
Message-ID: <4D5D8E44.5050906@gmail.com> (raw)
In-Reply-To: <1297756739-2958-3-git-send-email-andrew.zaborowski@intel.com>
[-- Attachment #1: Type: text/plain, Size: 10192 bytes --]
Hi Andrew,
On 02/15/2011 01:58 AM, Andrzej Zaborowski wrote:
> The watch callbacks are notified when a watched file is changed
> during a Refresh. Currently in most of the callbacks we free all
> the information read from the file, and schedule a re-read. I
> wonder if we need some sort of check if a re-read is already in
> progress. We may also need to send PropertyChanged indicating
> that the value is invalid until the file read finishes at which
> point we send another PropertyChanged. Otherwise the value of
> the property is for a short while inconsistent with what
> GetProperties would return (but I note that D-bus is already racy
> so maybe it doesn't matter).
> ---
> src/sim.c | 153 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++----
> 1 files changed, 143 insertions(+), 10 deletions(-)
>
> diff --git a/src/sim.c b/src/sim.c
> index 7c2e151..63c1ce9 100644
> --- a/src/sim.c
> +++ b/src/sim.c
> @@ -59,6 +59,7 @@ struct ofono_sim {
> char **language_prefs;
> unsigned char *efli;
> unsigned char efli_length;
> + gboolean language_prefs_update;
>
> enum ofono_sim_password_type pin_type;
> gboolean locked_pins[OFONO_SIM_PASSWORD_SIM_PUK]; /* Number of PINs */
> @@ -929,6 +930,11 @@ static void sim_iidf_read_cb(int ok, int length, int record,
> sim_iidf_read_clut_cb, sim);
> }
>
> +static void sim_image_data_changed(int id, void *userdata)
> +{
> + /* TODO: notify D-bus clients */
> +}
> +
> static void sim_get_image(struct ofono_sim *sim, unsigned char id,
> gpointer user_data)
> {
> @@ -959,6 +965,8 @@ static void sim_get_image(struct ofono_sim *sim, unsigned char id,
> /* read the image data */
> ofono_sim_read_bytes(sim->context, iidf_id, iidf_offset, iidf_len,
> sim_iidf_read_cb, sim);
> + ofono_sim_add_file_watch(sim->context, iidf_id,
> + sim_image_data_changed, sim, NULL);
> }
>
> static DBusMessage *sim_get_icon(DBusConnection *conn,
> @@ -1198,10 +1206,12 @@ out:
> check:
> /* All records retrieved */
> if (sim->service_numbers) {
> - char **service_numbers;
> -
> sim->service_numbers = g_slist_reverse(sim->service_numbers);
> sim->sdn_ready = TRUE;
> + }
> +
> + if (sim->sdn_ready) {
> + char **service_numbers;
>
> service_numbers = get_service_numbers(sim->service_numbers);
>
> @@ -1214,6 +1224,21 @@ check:
> }
> }
>
> +static void sim_service_numbers_changed(int id, void *userdata)
> +{
> + struct ofono_sim *sim = userdata;
> +
> + if (sim->service_numbers) {
> + g_slist_foreach(sim->service_numbers,
> + (GFunc)service_number_free, NULL);
> + g_slist_free(sim->service_numbers);
> + sim->service_numbers = NULL;
> + }
> +
> + ofono_sim_read(sim->context, SIM_EFSDN_FILEID,
> + OFONO_SIM_FILE_STRUCTURE_FIXED, sim_sdn_read_cb, sim);
> +}
> +
> static void sim_own_numbers_update(struct ofono_sim *sim)
> {
> ofono_sim_read(sim->context, SIM_EFMSISDN_FILEID,
> @@ -1221,6 +1246,13 @@ static void sim_own_numbers_update(struct ofono_sim *sim)
> sim);
> }
>
> +static void sim_own_numbers_changed(int id, void *userdata)
> +{
> + struct ofono_sim *sim = userdata;
> +
> + sim_own_numbers_update(sim);
> +}
> +
> static void sim_efimg_read_cb(int ok, int length, int record,
> const unsigned char *data,
> int record_length, void *userdata)
> @@ -1262,19 +1294,69 @@ static void sim_efimg_read_cb(int ok, int length, int record,
> memcpy(efimg, &data[1], 9);
> }
>
> +static void sim_efimg_changed(int id, void *userdata)
> +{
> + struct ofono_sim *sim = userdata;
> + int i, iidf_id;
> +
> + if (sim->efimg != NULL) {
> + for (i = sim->efimg_length / 9 - 1; i >= 0; i--) {
> + iidf_id = (sim->efimg[i * 9 + 3] << 8) |
> + sim->efimg[i * 9 + 4];
> +
> + ofono_sim_remove_file_watch(sim->context, iidf_id);
> + }
> +
> + g_free(sim->efimg);
> + sim->efimg = NULL;
> + sim->efimg_length = 0;
> + }
> +
> + ofono_sim_read(sim->context, SIM_EFIMG_FILEID,
> + OFONO_SIM_FILE_STRUCTURE_FIXED, sim_efimg_read_cb, sim);
> +
> + /* TODO: notify D-bus clients */
> +}
> +
> static void sim_ready(enum ofono_sim_state new_state, void *user)
> {
> struct ofono_sim *sim = user;
> + int i, iidf_id;
> +
> + if (new_state != OFONO_SIM_STATE_READY) {
> + if (sim->context == NULL)
> + return;
> +
> + ofono_sim_remove_file_watch(sim->context, SIM_EFMSISDN_FILEID);
> + ofono_sim_remove_file_watch(sim->context, SIM_EFSDN_FILEID);
> + ofono_sim_remove_file_watch(sim->context, SIM_EFIMG_FILEID);
> +
So two problems here. The file_watch id is not related to the file EF.
So this won't really work anyway. It also sounds like we should be
canceling the entire context (and thus the related file watches) instead
of trying to cancel them one by one.
What do you think of using two separate sim contexts here? Also, this
might really belong in sim_free_main_state instead of this function.
> + if (sim->efimg == NULL)
> + return;
> +
> + for (i = sim->efimg_length / 9 - 1; i >= 0; i--) {
> + iidf_id = (sim->efimg[i * 9 + 3] << 8) |
> + sim->efimg[i * 9 + 4];
> +
> + ofono_sim_remove_file_watch(sim->context, iidf_id);
> + }
>
> - if (new_state != OFONO_SIM_STATE_READY)
> return;
> + }
>
> sim_own_numbers_update(sim);
> + ofono_sim_add_file_watch(sim->context, SIM_EFMSISDN_FILEID,
> + sim_own_numbers_changed, sim, NULL);
>
> ofono_sim_read(sim->context, SIM_EFSDN_FILEID,
> OFONO_SIM_FILE_STRUCTURE_FIXED, sim_sdn_read_cb, sim);
> + ofono_sim_add_file_watch(sim->context, SIM_EFSDN_FILEID,
> + sim_service_numbers_changed, sim, NULL);
> +
> ofono_sim_read(sim->context, SIM_EFIMG_FILEID,
> OFONO_SIM_FILE_STRUCTURE_FIXED, sim_efimg_read_cb, sim);
> + ofono_sim_add_file_watch(sim->context, SIM_EFIMG_FILEID,
> + sim_efimg_changed, sim, NULL);
> }
>
> static void sim_imsi_cb(const struct ofono_error *error, const char *imsi,
> @@ -1873,7 +1955,11 @@ skip_efpl:
> DBUS_TYPE_STRING,
> &sim->language_prefs);
>
> - sim_pin_check(sim);
> + /* Proceed with sim initialization if we're not merely updating */
> + if (!sim->language_prefs_update)
> + sim_pin_check(sim);
> +
> + sim->language_prefs_update = FALSE;
> }
>
> static void sim_iccid_read_cb(int ok, int length, int record,
> @@ -1899,6 +1985,43 @@ static void sim_iccid_read_cb(int ok, int length, int record,
> &sim->iccid);
> }
>
> +static void sim_iccid_changed(int id, void *userdata)
> +{
> + struct ofono_sim *sim = userdata;
> +
> + if (sim->iccid) {
> + g_free(sim->iccid);
> + sim->iccid = NULL;
> + }
> +
> + ofono_sim_read(sim->context, SIM_EF_ICCID_FILEID,
> + OFONO_SIM_FILE_STRUCTURE_TRANSPARENT,
> + sim_iccid_read_cb, sim);
> +}
> +
> +static void sim_efli_efpl_changed(int id, void *userdata)
> +{
> + struct ofono_sim *sim = userdata;
> +
> + if (sim->efli != NULL) /* This shouldn't happen */
> + return;
> +
> + if (sim->language_prefs) {
> + g_strfreev(sim->language_prefs);
> + sim->language_prefs = NULL;
> + }
> +
> + sim->language_prefs_update = TRUE;
> +
> + ofono_sim_read(sim->context, SIM_EFLI_FILEID,
> + OFONO_SIM_FILE_STRUCTURE_TRANSPARENT,
> + sim_efli_read_cb, sim);
> +
> + ofono_sim_read(sim->context, SIM_EFPL_FILEID,
> + OFONO_SIM_FILE_STRUCTURE_TRANSPARENT,
> + sim_efpl_read_cb, sim);
> +}
> +
> static void sim_initialize(struct ofono_sim *sim)
> {
> /*
> @@ -1927,10 +2050,15 @@ static void sim_initialize(struct ofono_sim *sim)
> * in the EFust
> */
>
> + if (sim->context == NULL)
> + sim->context = ofono_sim_context_create(sim);
> +
> /* Grab the EFiccid which is always available */
> ofono_sim_read(sim->context, SIM_EF_ICCID_FILEID,
> OFONO_SIM_FILE_STRUCTURE_TRANSPARENT,
> sim_iccid_read_cb, sim);
> + ofono_sim_add_file_watch(sim->context, SIM_EF_ICCID_FILEID,
> + sim_iccid_changed, sim, NULL);
>
> /* EFecc is read by the voicecall atom */
>
> @@ -1945,9 +2073,14 @@ static void sim_initialize(struct ofono_sim *sim)
> ofono_sim_read(sim->context, SIM_EFLI_FILEID,
> OFONO_SIM_FILE_STRUCTURE_TRANSPARENT,
> sim_efli_read_cb, sim);
> + ofono_sim_add_file_watch(sim->context, SIM_EFLI_FILEID,
> + sim_efli_efpl_changed, sim, NULL);
> +
> ofono_sim_read(sim->context, SIM_EFPL_FILEID,
> OFONO_SIM_FILE_STRUCTURE_TRANSPARENT,
> sim_efpl_read_cb, sim);
> + ofono_sim_add_file_watch(sim->context, SIM_EFPL_FILEID,
> + sim_efli_efpl_changed, sim, NULL);
> }
>
> struct ofono_sim_context *ofono_sim_context_create(struct ofono_sim *sim)
> @@ -2096,6 +2229,11 @@ static void sim_free_early_state(struct ofono_sim *sim)
> g_strfreev(sim->language_prefs);
> sim->language_prefs = NULL;
> }
> +
> + if (sim->context) {
> + ofono_sim_context_free(sim->context);
> + sim->context = NULL;
> + }
> }
>
> static void sim_free_main_state(struct ofono_sim *sim)
> @@ -2119,6 +2257,7 @@ static void sim_free_main_state(struct ofono_sim *sim)
> (GFunc)service_number_free, NULL);
> g_slist_free(sim->service_numbers);
> sim->service_numbers = NULL;
> + sim->sdn_ready = FALSE;
> }
>
> if (sim->efust) {
> @@ -2296,11 +2435,6 @@ static void sim_remove(struct ofono_atom *atom)
>
> sim_free_state(sim);
>
> - if (sim->context) {
> - ofono_sim_context_free(sim->context);
> - sim->context = NULL;
> - }
> -
Out of curiosity, why are you moving this one? It seems to have no
effect. Either way, this belongs in a separate patch ;)
> sim_fs_free(sim->simfs);
> sim->simfs = NULL;
>
> @@ -2366,7 +2500,6 @@ void ofono_sim_register(struct ofono_sim *sim)
> ofono_modem_add_interface(modem, OFONO_SIM_MANAGER_INTERFACE);
> sim->state_watches = __ofono_watchlist_new(g_free);
> sim->simfs = sim_fs_new(sim, sim->driver);
> - sim->context = ofono_sim_context_create(sim);
Same comment as above.
>
> __ofono_atom_register(sim->atom, sim_unregister);
>
Regards,
-Denis
next prev parent reply other threads:[~2011-02-17 21:08 UTC|newest]
Thread overview: 20+ messages / expand[flat|nested] mbox.gz Atom feed top
2011-02-15 7:58 [PATCH 1/9] modem.c: Go to PRE_SIM modem state on OFONO_SIM_STATE_INSERTED Andrzej Zaborowski
2011-02-15 7:58 ` [PATCH 2/9] sim: Implement basic Refresh Andrzej Zaborowski
2011-02-17 21:03 ` Denis Kenzior
2011-02-19 4:18 ` Andrzej Zaborowski
2011-02-15 7:58 ` [PATCH 3/9] sim: Watch for changes to relevant SIM files Andrzej Zaborowski
2011-02-17 21:08 ` Denis Kenzior [this message]
2011-02-19 4:24 ` Andrzej Zaborowski
2011-02-15 7:58 ` [PATCH 4/9] voicecall: " Andrzej Zaborowski
2011-02-17 21:08 ` Denis Kenzior
2011-02-15 7:58 ` [PATCH 5/9] network: " Andrzej Zaborowski
2011-02-17 21:09 ` Denis Kenzior
2011-02-15 7:58 ` [PATCH 6/9] cbs: " Andrzej Zaborowski
2011-02-17 21:10 ` Denis Kenzior
2011-02-15 7:58 ` [PATCH 7/9] message-waiting: " Andrzej Zaborowski
2011-02-17 20:54 ` Denis Kenzior
2011-02-15 7:58 ` [PATCH 8/9] call-forwarding: " Andrzej Zaborowski
2011-02-17 20:57 ` Denis Kenzior
2011-02-15 7:58 ` [PATCH 9/9] stk: Partially handle Refresh command Andrzej Zaborowski
2011-02-17 21:17 ` Denis Kenzior
2011-02-17 21:00 ` [PATCH 1/9] modem.c: Go to PRE_SIM modem state on OFONO_SIM_STATE_INSERTED Denis Kenzior
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=4D5D8E44.5050906@gmail.com \
--to=denkenz@gmail.com \
--cc=ofono@ofono.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.