[PATCH 1/4] Add SIM filesystem watch api.

[PATCH 1/4] Add SIM filesystem watch api.

[Andrzej Zaborowski]

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

This allows code that reads files from the SIM card to also watch for
changes in these files signalled in a Refresh command by the SIM.
---
 include/sim.h |    9 +++++++++
 1 files changed, 9 insertions(+), 0 deletions(-)

diff --git a/include/sim.h b/include/sim.h
index 137e825..deed81b 100644
--- a/include/sim.h
+++ b/include/sim.h
@@ -118,6 +118,8 @@ typedef void (*ofono_sim_lock_unlock_cb_t)(const struct ofono_error *error,
 typedef void (*ofono_sim_locked_cb_t)(const struct ofono_error *error,
 					int locked, void *data);
 
+typedef void (*ofono_sim_file_notify_t)(int id, void *userdata);
+
 struct ofono_sim_driver {
 	const char *name;
 	int (*probe)(struct ofono_sim *sim, unsigned int vendor, void *data);
@@ -218,6 +220,13 @@ int ofono_sim_write(struct ofono_sim_context *context, int id,
 int ofono_sim_read_bytes(struct ofono_sim_context *context, int id,
 			unsigned short offset, unsigned short num_bytes,
 			ofono_sim_file_read_cb_t cb, void *data);
+
+int ofono_sim_add_file_watch(struct ofono_sim_context *context, int id,
+			ofono_sim_file_notify_t notify, void *userdata);
+
+void ofono_sim_remove_file_watch(struct ofono_sim_context *context, int id,
+			ofono_sim_file_notify_t notify, void *userdata);
+
 #ifdef __cplusplus
 }
 #endif
-- 
1.7.1.86.g0e460.dirty

[PATCH 2/4] sim: Implement file watching and basic refresh.

[Andrzej Zaborowski]

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

In this version watches are asssigned to simfs contexts so that
there's less free()ing for users to do.  This patch includes part of
EF cache flushing patch from the previous version.
---
 src/ofono.h |    4 +
 src/sim.c   |  226 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 src/simfs.c |    5 ++
 src/simfs.h |    2 +
 4 files changed, 237 insertions(+), 0 deletions(-)

diff --git a/src/ofono.h b/src/ofono.h
index 6ba0187..b7fed13 100644
--- a/src/ofono.h
+++ b/src/ofono.h
@@ -299,6 +299,10 @@ ofono_bool_t __ofono_sim_service_available(struct ofono_sim *sim,
 						int ust_service,
 						int sst_service);
 
+void __ofono_sim_refresh(struct ofono_sim *sim, GSList *file_list,
+				ofono_bool_t full_file_change,
+				ofono_bool_t naa_init);
+
 #include <ofono/stk.h>
 
 typedef void (*__ofono_sms_sim_download_cb_t)(ofono_bool_t ok,
diff --git a/src/sim.c b/src/sim.c
index 52261c8..55d4d75 100644
--- a/src/sim.c
+++ b/src/sim.c
@@ -97,6 +97,7 @@ struct ofono_sim {
 
 	struct sim_fs *simfs;
 	struct ofono_sim_context *context;
+	GSList *fs_watches;
 
 	unsigned char *iidf_image;
 
@@ -118,6 +119,14 @@ struct service_number {
 	struct ofono_phone_number ph;
 };
 
+struct fs_watch {
+	int id;
+	enum ofono_sim_state reset_state;
+	ofono_sim_file_notify_t notify;
+	void *notify_data;
+	struct ofono_sim_context *context;
+};
+
 static const char *const passwd_name[] = {
 	[OFONO_SIM_PASSWORD_NONE] = "none",
 	[OFONO_SIM_PASSWORD_SIM_PIN] = "pin",
@@ -203,6 +212,51 @@ static enum ofono_sim_password_type puk2pin(enum ofono_sim_password_type type)
 	}
 }
 
+static gint fs_watch_compare_by_id(gconstpointer a, gconstpointer b)
+{
+	const struct fs_watch *w = a;
+	const int *id = b;
+
+	return w->id - *id;
+}
+
+static gint fs_watch_compare_by_context(gconstpointer a, gconstpointer b)
+{
+	const struct fs_watch *w = a;
+
+	return (void *) w->context - b;
+}
+
+static int sim_add_file_watch(struct ofono_sim_context *context, int id,
+				gboolean remove_atoms,
+				ofono_sim_file_notify_t notify, void *userdata)
+{
+	struct ofono_sim *sim = sim_fs_context_get_sim(context);
+	struct fs_watch *item;
+
+	DBG("%p", sim);
+
+	if (sim == NULL)
+		return 0;
+
+	/* Currently there's no need for multiple watches per file */
+	if (g_slist_find_custom(sim->fs_watches, &id, fs_watch_compare_by_id))
+		return -EEXIST;
+
+	item = g_new0(struct fs_watch, 1);
+
+	item->id = id;
+	item->notify = notify;
+	item->notify_data = userdata;
+	item->reset_state = remove_atoms ? OFONO_SIM_STATE_INSERTED :
+		OFONO_SIM_STATE_READY;
+	item->context = context;
+
+	sim->fs_watches = g_slist_prepend(sim->fs_watches, item);
+
+	return id;
+}
+
 static char **get_own_numbers(GSList *own_numbers)
 {
 	int nelem = 0;
@@ -1957,6 +2011,15 @@ struct ofono_sim_context *ofono_sim_context_create(struct ofono_sim *sim)
 
 void ofono_sim_context_free(struct ofono_sim_context *context)
 {
+	struct ofono_sim *sim = sim_fs_context_get_sim(context);
+	GSList *l;
+
+	while ((l = g_slist_find_custom(sim->fs_watches, context,
+					fs_watch_compare_by_context))) {
+		g_free(l->data);
+		sim->fs_watches = g_slist_remove(sim->fs_watches, l->data);
+	}
+
 	return sim_fs_context_free(context);
 }
 
@@ -2278,6 +2341,12 @@ static void sim_remove(struct ofono_atom *atom)
 	sim_fs_free(sim->simfs);
 	sim->simfs = NULL;
 
+	if (sim->fs_watches != NULL) {
+		g_slist_foreach(sim->fs_watches, (GFunc) g_free, NULL);
+		g_slist_free(sim->fs_watches);
+		sim->fs_watches = NULL;
+	}
+
 	g_free(sim);
 }
 
@@ -2364,3 +2433,160 @@ void *ofono_sim_get_data(struct ofono_sim *sim)
 {
 	return sim->driver_data;
 }
+
+static void sim_file_changed_flush(struct ofono_sim *sim, int id)
+{
+	int i, imgid;
+
+	if (id == SIM_EFIMG_FILEID)
+		/* All cached images become invalid */
+		sim_fs_image_cache_flush(sim->simfs);
+	else if (sim->efimg) {
+		/*
+		 * Data and CLUT for image instances stored in the changed
+		 * file need to be re-read.
+		 */
+		for (i = sim->efimg_length / 9 - 1; i >= 0; i--) {
+			imgid = (sim->efimg[i * 9 + 3] << 8) |
+				sim->efimg[i * 9 + 4];
+
+			if (imgid == id)
+				sim_fs_image_cache_flush_file(sim->simfs, i);
+		}
+	}
+
+	sim_fs_cache_flush_file(sim->simfs, id);
+}
+
+int ofono_sim_add_file_watch(struct ofono_sim_context *context, int id,
+			ofono_sim_file_notify_t notify, void *userdata)
+{
+	return sim_add_file_watch(context, id, FALSE, notify, userdata);
+}
+
+void ofono_sim_remove_file_watch(struct ofono_sim_context *context, int id,
+				ofono_sim_file_notify_t notify, void *userdata)
+{
+	struct ofono_sim *sim = sim_fs_context_get_sim(context);
+	GSList *item = g_slist_find_custom(sim->fs_watches,
+						&id, fs_watch_compare_by_id);
+
+	DBG("%p", sim);
+
+	if (!item)
+		return;
+
+	g_free(item->data);
+	sim->fs_watches = g_slist_remove(sim->fs_watches, item->data);
+}
+
+void __ofono_sim_refresh(struct ofono_sim *sim, GSList *file_list,
+			ofono_bool_t full_file_change, ofono_bool_t naa_init)
+{
+	GSList *l, *fl;
+	enum sim_reset {
+		RESET_REMOVE_ATOMS	= OFONO_SIM_STATE_INSERTED,
+		RESET_SIGNAL_READY	= OFONO_SIM_STATE_READY,
+		RESET_NONE,
+	} reset_state = RESET_NONE;
+
+	/* Flush cached content for affected files */
+	if (full_file_change)
+		sim_fs_cache_flush(sim->simfs);
+	else {
+		for (fl = file_list; fl; fl = fl->next) {
+			struct stk_file *file = fl->data;
+			int id = (file->file[file->len - 2] << 8) |
+				(file->file[file->len - 1] << 0);
+
+			sim_file_changed_flush(sim, id);
+		}
+	}
+
+	if (naa_init)
+		reset_state = RESET_REMOVE_ATOMS;
+
+	/*
+	 * Check if we have file change handlers for all of the affected
+	 * files.  If not, we will fall back to re-initialising the
+	 * application which ensures that all files are re-read.
+	 */
+	for (l = sim->fs_watches; l; l = l->next) {
+		struct fs_watch *w = l->data;
+
+		if (full_file_change) {
+			if (w->notify)
+				continue;
+
+			if (w->reset_state < reset_state)
+				reset_state = reset_state;
+
+			continue;
+		}
+
+		for (fl = file_list; fl; fl = fl->next) {
+			struct stk_file *file = fl->data;
+			int id = (file->file[file->len - 2] << 8) |
+				(file->file[file->len - 1] << 0);
+
+			if (id != w->id)
+				continue;
+
+			if (w->notify)
+				break;
+
+			if (w->reset_state < reset_state)
+				reset_state = reset_state;
+
+			break;
+		}
+	}
+
+	/*
+	 * Notify the subscribers of files that have changed unless we
+	 * have determined that a re-initialisation is necessary and
+	 * will trigger re-reading of those files anyway.
+	 */
+	for (l = sim->fs_watches; l; l = l->next) {
+		struct fs_watch *w = l->data;
+
+		if (full_file_change) {
+			if (w->reset_state < reset_state)
+				w->notify(w->id, w->notify_data);
+
+			continue;
+		}
+
+		for (fl = file_list; fl; fl = fl->next) {
+			struct stk_file *file = fl->data;
+			int id = (file->file[file->len - 2] << 8) |
+				(file->file[file->len - 1] << 0);
+
+			if (id != w->id)
+				continue;
+
+			if (w->reset_state < reset_state)
+				w->notify(w->id, w->notify_data);
+
+			break;
+		}
+	}
+
+	switch (reset_state) {
+	case RESET_REMOVE_ATOMS:
+		/*
+		 * REVISIT: There's some concern that on re-insertion the
+		 * atoms will start to talk to the SIM before it becomes
+		 * ready, on certain SIMs.
+		 */
+		ofono_sim_inserted_notify(sim, FALSE);
+		ofono_sim_inserted_notify(sim, TRUE);
+		break;
+	case RESET_SIGNAL_READY:
+		sim->state = OFONO_SIM_STATE_INSERTED;
+		sim_set_ready(sim);
+		break;
+	case RESET_NONE:
+		break;
+	}
+}
diff --git a/src/simfs.c b/src/simfs.c
index 0459447..f829161 100644
--- a/src/simfs.c
+++ b/src/simfs.c
@@ -167,6 +167,11 @@ void sim_fs_context_free(struct ofono_sim_context *context)
 	g_free(context);
 }
 
+struct ofono_sim *sim_fs_context_get_sim(struct ofono_sim_context *context)
+{
+	return context->fs->sim;
+}
+
 static void sim_fs_end_current(struct sim_fs *fs)
 {
 	struct sim_fs_op *op = g_queue_pop_head(fs->op_q);
diff --git a/src/simfs.h b/src/simfs.h
index d93d96e..5623175 100644
--- a/src/simfs.h
+++ b/src/simfs.h
@@ -56,3 +56,5 @@ void sim_fs_image_cache_flush_file(struct sim_fs *fs, int id);
 
 void sim_fs_free(struct sim_fs *fs);
 void sim_fs_context_free(struct ofono_sim_context *context);
+
+struct ofono_sim *sim_fs_context_get_sim(struct ofono_sim_context *context);
-- 
1.7.1.86.g0e460.dirty

Re: [PATCH 2/4] sim: Implement file watching and basic refresh.

[Denis Kenzior]

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

Hi Andrew,

> +void __ofono_sim_refresh(struct ofono_sim *sim, GSList *file_list,
> +			ofono_bool_t full_file_change, ofono_bool_t naa_init)
> +{
> +	GSList *l, *fl;
> +	enum sim_reset {
> +		RESET_REMOVE_ATOMS	= OFONO_SIM_STATE_INSERTED,
> +		RESET_SIGNAL_READY	= OFONO_SIM_STATE_READY,
> +		RESET_NONE,
> +	} reset_state = RESET_NONE;
> +
> +	/* Flush cached content for affected files */
> +	if (full_file_change)
> +		sim_fs_cache_flush(sim->simfs);
> +	else {
> +		for (fl = file_list; fl; fl = fl->next) {
> +			struct stk_file *file = fl->data;
> +			int id = (file->file[file->len - 2] << 8) |
> +				(file->file[file->len - 1] << 0);
> +
> +			sim_file_changed_flush(sim, id);
> +		}
> +	}
> +
> +	if (naa_init)
> +		reset_state = RESET_REMOVE_ATOMS;
> +
> +	/*
> +	 * Check if we have file change handlers for all of the affected
> +	 * files.  If not, we will fall back to re-initialising the
> +	 * application which ensures that all files are re-read.
> +	 */
> +	for (l = sim->fs_watches; l; l = l->next) {
> +		struct fs_watch *w = l->data;
> +
> +		if (full_file_change) {
> +			if (w->notify)
> +				continue;
> +
> +			if (w->reset_state < reset_state)
> +				reset_state = reset_state;
> +
> +			continue;
> +		}

What exactly is this if statement accomplishing? If we have a
full_file_change notification we have to always re-init the SIM.

> +
> +		for (fl = file_list; fl; fl = fl->next) {
> +			struct stk_file *file = fl->data;
> +			int id = (file->file[file->len - 2] << 8) |
> +				(file->file[file->len - 1] << 0);
> +
> +			if (id != w->id)
> +				continue;
> +
> +			if (w->notify)
> +				break;
> +
> +			if (w->reset_state < reset_state)
> +				reset_state = reset_state;
> +
> +			break;
> +		}
> +	}
> +

And I'm pretty sure we can just skip this one as well, a NULL callback
is just that.

> +	/*
> +	 * Notify the subscribers of files that have changed unless we
> +	 * have determined that a re-initialisation is necessary and
> +	 * will trigger re-reading of those files anyway.
> +	 */
> +	for (l = sim->fs_watches; l; l = l->next) {
> +		struct fs_watch *w = l->data;
> +
> +		if (full_file_change) {
> +			if (w->reset_state < reset_state)
> +				w->notify(w->id, w->notify_data);
> +
> +			continue;
> +		}
> +
> +		for (fl = file_list; fl; fl = fl->next) {
> +			struct stk_file *file = fl->data;
> +			int id = (file->file[file->len - 2] << 8) |
> +				(file->file[file->len - 1] << 0);
> +
> +			if (id != w->id)
> +				continue;
> +
> +			if (w->reset_state < reset_state)
> +				w->notify(w->id, w->notify_data);
> +
> +			break;
> +		}
> +	}
> +

So again, this seems entirely way too complicated.  I'd suggest
something like:

if full_file_change or changed ef in (EFbdn, EFfdn, EFest, EFust, EFsst,
EFphase, EFad, EFcphs_info):
	Remove all atoms in post_sim and post_online
	Restart SIM initialization procedure

if full_file_change:
	notify all remaining watches
else
	for each file changed:
		notify matching remaining watches

Incidentally the above list of files to re-read is exactly the same
files we need to re-initialize when we lock ourselves out when trying to
lock/unlock/change the PIN.

> +	switch (reset_state) {
> +	case RESET_REMOVE_ATOMS:
> +		/*
> +		 * REVISIT: There's some concern that on re-insertion the
> +		 * atoms will start to talk to the SIM before it becomes
> +		 * ready, on certain SIMs.
> +		 */
> +		ofono_sim_inserted_notify(sim, FALSE);
> +		ofono_sim_inserted_notify(sim, TRUE);
> +		break;

This is not really a good idea semantics wise.  The SIM hasn't really
been removed and a UICC reset / NAA application reset / NAA session
reset is not being performed either.

> +	case RESET_SIGNAL_READY:
> +		sim->state = OFONO_SIM_STATE_INSERTED;
> +		sim_set_ready(sim);
> +		break;

I still don't see a reason for this one, isn't the file_changed watch
enough?

Regards,
-Denis

Re: [PATCH 2/4] sim: Implement file watching and basic refresh.

[Andrzej Zaborowski]

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

Hi Denis,

On 7 February 2011 20:34, Denis Kenzior <denkenz@gmail.com> wrote:
>> +void __ofono_sim_refresh(struct ofono_sim *sim, GSList *file_list,
>> +                     ofono_bool_t full_file_change, ofono_bool_t naa_init)
>> +{
>> +     GSList *l, *fl;
>> +     enum sim_reset {
>> +             RESET_REMOVE_ATOMS      = OFONO_SIM_STATE_INSERTED,
>> +             RESET_SIGNAL_READY      = OFONO_SIM_STATE_READY,
>> +             RESET_NONE,
>> +     } reset_state = RESET_NONE;
>> +
>> +     /* Flush cached content for affected files */
>> +     if (full_file_change)
>> +             sim_fs_cache_flush(sim->simfs);
>> +     else {
>> +             for (fl = file_list; fl; fl = fl->next) {
>> +                     struct stk_file *file = fl->data;
>> +                     int id = (file->file[file->len - 2] << 8) |
>> +                             (file->file[file->len - 1] << 0);
>> +
>> +                     sim_file_changed_flush(sim, id);
>> +             }
>> +     }
>> +
>> +     if (naa_init)
>> +             reset_state = RESET_REMOVE_ATOMS;
>> +
>> +     /*
>> +      * Check if we have file change handlers for all of the affected
>> +      * files.  If not, we will fall back to re-initialising the
>> +      * application which ensures that all files are re-read.
>> +      */
>> +     for (l = sim->fs_watches; l; l = l->next) {
>> +             struct fs_watch *w = l->data;
>> +
>> +             if (full_file_change) {
>> +                     if (w->notify)
>> +                             continue;
>> +
>> +                     if (w->reset_state < reset_state)
>> +                             reset_state = reset_state;
>> +
>> +                     continue;
>> +             }
>
> What exactly is this if statement accomplishing? If we have a
> full_file_change notification we have to always re-init the SIM.

In practice you're right that we can probably just force SIM
reinitialisation instead.  It seemed logically/semantically easier on
the reader to treat full_file_change == TRUE as if the file_list
contained all of the file IDs, which is what this if statement does:
if full_file_change, then act as if every file was on the changed
files list.

>
>> +
>> +             for (fl = file_list; fl; fl = fl->next) {
>> +                     struct stk_file *file = fl->data;
>> +                     int id = (file->file[file->len - 2] << 8) |
>> +                             (file->file[file->len - 1] << 0);
>> +
>> +                     if (id != w->id)
>> +                             continue;
>> +
>> +                     if (w->notify)
>> +                             break;
>> +
>> +                     if (w->reset_state < reset_state)
>> +                             reset_state = reset_state;
>> +
>> +                     break;
>> +             }
>> +     }
>> +
>
> And I'm pretty sure we can just skip this one as well, a NULL callback
> is just that.

Ok, I think I documented it in the earlier version of this series but
not repeated here.  The idea is that non-null callback means: "notify
me when this file changes, and I'll take care of the changed
contents", while NULL callback means "I haven't implemented a proper
handler yet, so for now you need to reinitialise this atom which will
force it to take the new contents into account".

We go through the list of all changed files.  If any one of them has
no callback, then there's no point calling any notifiers because all
atoms will be reinitialised and will re-read these files anyway.

>
>> +     /*
>> +      * Notify the subscribers of files that have changed unless we
>> +      * have determined that a re-initialisation is necessary and
>> +      * will trigger re-reading of those files anyway.
>> +      */
>> +     for (l = sim->fs_watches; l; l = l->next) {
>> +             struct fs_watch *w = l->data;
>> +
>> +             if (full_file_change) {
>> +                     if (w->reset_state < reset_state)
>> +                             w->notify(w->id, w->notify_data);
>> +
>> +                     continue;
>> +             }
>> +
>> +             for (fl = file_list; fl; fl = fl->next) {
>> +                     struct stk_file *file = fl->data;
>> +                     int id = (file->file[file->len - 2] << 8) |
>> +                             (file->file[file->len - 1] << 0);
>> +
>> +                     if (id != w->id)
>> +                             continue;
>> +
>> +                     if (w->reset_state < reset_state)
>> +                             w->notify(w->id, w->notify_data);
>> +
>> +                     break;
>> +             }
>> +     }
>> +
>
> So again, this seems entirely way too complicated.  I'd suggest
> something like:
>
> if full_file_change or changed ef in (EFbdn, EFfdn, EFest, EFust, EFsst,
> EFphase, EFad, EFcphs_info):
>        Remove all atoms in post_sim and post_online
>        Restart SIM initialization procedure

I guess that is okay, as long as we remember to keep this list updated
(there are various other important files which we may not be using
yet)

>
> if full_file_change:
>        notify all remaining watches

In this case we have already reinitialised the SIM, so I think we
don't need to do that.

> else
>        for each file changed:
>                notify matching remaining watches

What if a file is changed which doesn't have a handler implemented?
Shouldn't we fall back to removing the atom? That was the idea of
those "complicated" loops above.

>
> Incidentally the above list of files to re-read is exactly the same
> files we need to re-initialize when we lock ourselves out when trying to
> lock/unlock/change the PIN.
>
>> +     switch (reset_state) {
>> +     case RESET_REMOVE_ATOMS:
>> +             /*
>> +              * REVISIT: There's some concern that on re-insertion the
>> +              * atoms will start to talk to the SIM before it becomes
>> +              * ready, on certain SIMs.
>> +              */
>> +             ofono_sim_inserted_notify(sim, FALSE);
>> +             ofono_sim_inserted_notify(sim, TRUE);
>> +             break;
>
> This is not really a good idea semantics wise.  The SIM hasn't really
> been removed and a UICC reset / NAA application reset / NAA session
> reset is not being performed either.

Hmm probably.  Although from our point of view the NAA initialisation
looks the same as sim re-insertion so we end up inlining that fuction.

>
>> +     case RESET_SIGNAL_READY:
>> +             sim->state = OFONO_SIM_STATE_INSERTED;
>> +             sim_set_ready(sim);
>> +             break;
>
> I still don't see a reason for this one, isn't the file_changed watch
> enough?

In the long range I guess it should be enough but for the moment we
only have the handler for EFmsisdn and two more files, for other files
we rely on the atom's sim_ready notifier to re-read the files, which
should have the the same effect (right?)

Best regards

Re: [PATCH 2/4] sim: Implement file watching and basic refresh.

[Denis Kenzior]

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

Hi Andrew,

>> if full_file_change or changed ef in (EFbdn, EFfdn, EFest, EFust, EFsst,
>> EFphase, EFad, EFcphs_info):
>>        Remove all atoms in post_sim and post_online
>>        Restart SIM initialization procedure
> 
> I guess that is okay, as long as we remember to keep this list updated
> (there are various other important files which we may not be using
> yet)
> 

That is fine, we fix these during the pre-certification / certification,
etc.

>>
>> if full_file_change:
>>        notify all remaining watches
> 
> In this case we have already reinitialised the SIM, so I think we
> don't need to do that.

Actually we should for files that are read during pre-sim but are not
important enough.  E.g. EFecc, EFli, EFpl, EFiccid, etc.

> 
>> else
>>        for each file changed:
>>                notify matching remaining watches
> 
> What if a file is changed which doesn't have a handler implemented?
> Shouldn't we fall back to removing the atom? That was the idea of
> those "complicated" loops above.

If it should be handled, then its a bug.  Removing all atoms just
because someone forgot to handle a file is bad anyway.

> 
>>
>> Incidentally the above list of files to re-read is exactly the same
>> files we need to re-initialize when we lock ourselves out when trying to
>> lock/unlock/change the PIN.
>>
>>> +     switch (reset_state) {
>>> +     case RESET_REMOVE_ATOMS:
>>> +             /*
>>> +              * REVISIT: There's some concern that on re-insertion the
>>> +              * atoms will start to talk to the SIM before it becomes
>>> +              * ready, on certain SIMs.
>>> +              */
>>> +             ofono_sim_inserted_notify(sim, FALSE);
>>> +             ofono_sim_inserted_notify(sim, TRUE);
>>> +             break;
>>
>> This is not really a good idea semantics wise.  The SIM hasn't really
>> been removed and a UICC reset / NAA application reset / NAA session
>> reset is not being performed either.
> 
> Hmm probably.  Although from our point of view the NAA initialisation
> looks the same as sim re-insertion so we end up inlining that fuction.
>

I'm still fuzzy on what NAA initialization actually does.  Right now my
interpretation is that EFpl/EFli and EFiccid have not changed.  So it is
not quite the same as a reset.

>>
>>> +     case RESET_SIGNAL_READY:
>>> +             sim->state = OFONO_SIM_STATE_INSERTED;
>>> +             sim_set_ready(sim);
>>> +             break;
>>
>> I still don't see a reason for this one, isn't the file_changed watch
>> enough?
> 
> In the long range I guess it should be enough but for the moment we
> only have the handler for EFmsisdn and two more files, for other files
> we rely on the atom's sim_ready notifier to re-read the files, which
> should have the the same effect (right?)

or they can just install a file watch and remove it in case a sim
re-init condition is triggered.  That way if only EFmsisdn is changed,
then the file watch is fired.  If a complete re-init has to be done,
then this watch is removed and the file is re-read during regular
re-initialization procedure.

Regards,
-Denis

Re: [PATCH 2/4] sim: Implement file watching and basic refresh.

[Andrzej Zaborowski]

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

On 7 February 2011 22:13, Denis Kenzior <denkenz@gmail.com> wrote:
>>> if full_file_change or changed ef in (EFbdn, EFfdn, EFest, EFust, EFsst,
>>> EFphase, EFad, EFcphs_info):
>>>        Remove all atoms in post_sim and post_online
>>>        Restart SIM initialization procedure
>>
>> I guess that is okay, as long as we remember to keep this list updated
>> (there are various other important files which we may not be using
>> yet)
>>
>
> That is fine, we fix these during the pre-certification / certification,
> etc.
>
>>>
>>> if full_file_change:
>>>        notify all remaining watches
>>
>> In this case we have already reinitialised the SIM, so I think we
>> don't need to do that.
>
> Actually we should for files that are read during pre-sim but are not
> important enough.  E.g. EFecc, EFli, EFpl, EFiccid, etc.

But if we "restart SIM initialization procedure", we already re-read
those files so there's nothing more we need to do.  The sim
initialization in 51.011 includes reading those files if I remember
correctly.

>
>>
>>> else
>>>        for each file changed:
>>>                notify matching remaining watches
>>
>> What if a file is changed which doesn't have a handler implemented?
>> Shouldn't we fall back to removing the atom? That was the idea of
>> those "complicated" loops above.
>
> If it should be handled, then its a bug.  Removing all atoms just
> because someone forgot to handle a file is bad anyway.

We don't need to remove atoms, in this patch series we simply signal
"sim ready" to atoms in those cases.  Otherwise all those NULL watches
are useless, right?  The reason I introduced the NULL-callback watches
is because there are many files, like EFcfis, which are unlikely to be
changed by the SIM application or operator.  Yet we can't ignore them,
in case we receive a Refresh on those files.  We have two dozens of
such files and it's not practical to add notifiers for all of them,
it's easier to fall back to re-reading all of the post-sim files.

In the patch 3/4 I've added watches for all of the files where changes
need to be somehow handled.  Most of them have NULL callback because
they're not really important and it would be a huge patch if wanted to
have a handler for every single file, although we can add them
gradually later.  The files where changes don't need to be handled,
don't register a watch at all (like EFcbmi, EFcbmir which are only
used for initialising our own topics list)

>
>>
>>>
>>> Incidentally the above list of files to re-read is exactly the same
>>> files we need to re-initialize when we lock ourselves out when trying to
>>> lock/unlock/change the PIN.
>>>
>>>> +     switch (reset_state) {
>>>> +     case RESET_REMOVE_ATOMS:
>>>> +             /*
>>>> +              * REVISIT: There's some concern that on re-insertion the
>>>> +              * atoms will start to talk to the SIM before it becomes
>>>> +              * ready, on certain SIMs.
>>>> +              */
>>>> +             ofono_sim_inserted_notify(sim, FALSE);
>>>> +             ofono_sim_inserted_notify(sim, TRUE);
>>>> +             break;
>>>
>>> This is not really a good idea semantics wise.  The SIM hasn't really
>>> been removed and a UICC reset / NAA application reset / NAA session
>>> reset is not being performed either.
>>
>> Hmm probably.  Although from our point of view the NAA initialisation
>> looks the same as sim re-insertion so we end up inlining that fuction.
>>
>
> I'm still fuzzy on what NAA initialization actually does.  Right now my
> interpretation is that EFpl/EFli and EFiccid have not changed.  So it is
> not quite the same as a reset.

However with Full file change notification they may have changed, so
it's a safe fallback to re-read them.

Best regards

Re: [PATCH 2/4] sim: Implement file watching and basic refresh.

[Denis Kenzior]

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

Hi Andrew,

>>>>
>>>> if full_file_change:
>>>>        notify all remaining watches
>>>
>>> In this case we have already reinitialised the SIM, so I think we
>>> don't need to do that.
>>
>> Actually we should for files that are read during pre-sim but are not
>> important enough.  E.g. EFecc, EFli, EFpl, EFiccid, etc.
> 
> But if we "restart SIM initialization procedure", we already re-read
> those files so there's nothing more we need to do.  The sim
> initialization in 51.011 includes reading those files if I remember
> correctly.
> 

I'm still fuzzy on this one.  EFpl/EFli and EFiccid are in a different
DF than the rest of the files, so they might not need to be initialized
again in practice, unless we're performing a full reset.

In the case of EFecc, the file is on voicecall atom, so the change watch
has to be fired anyway.  So using my approach it works out nicely.

>>
>>>
>>>> else
>>>>        for each file changed:
>>>>                notify matching remaining watches
>>>
>>> What if a file is changed which doesn't have a handler implemented?
>>> Shouldn't we fall back to removing the atom? That was the idea of
>>> those "complicated" loops above.
>>
>> If it should be handled, then its a bug.  Removing all atoms just
>> because someone forgot to handle a file is bad anyway.
> 
> We don't need to remove atoms, in this patch series we simply signal
> "sim ready" to atoms in those cases.  Otherwise all those NULL watches
> are useless, right?  The reason I introduced the NULL-callback watches
> is because there are many files, like EFcfis, which are unlikely to be
> changed by the SIM application or operator.  Yet we can't ignore them,
> in case we receive a Refresh on those files.  We have two dozens of
> such files and it's not practical to add notifiers for all of them,
> it's easier to fall back to re-reading all of the post-sim files.

Honestly I think we should handle all of them, one by one.  It might
take a little while, but we have to anyway in the long run.  We can
concentrate on the important ones first, e.g. EFcsp, EFspn, EFopl,
EFecc, etc.  Sure, in the short term we might not handle each and every
file that could be sent via Refresh.  But then we just acknowledge it as
a bug and fix it later.  Re-initializing everything just because e.g.
EFmbdn or some other unimportant file changed and we're too lazy to fix
it is just not a good idea anyway.

> 
> In the patch 3/4 I've added watches for all of the files where changes
> need to be somehow handled.  Most of them have NULL callback because
> they're not really important and it would be a huge patch if wanted to
> have a handler for every single file, although we can add them
> gradually later.  The files where changes don't need to be handled,
> don't register a watch at all (like EFcbmi, EFcbmir which are only
> used for initialising our own topics list)

I know what you're trying to accomplish, but I don't think we need this
flexibility.  The 'important' files that are likely to change have to be
handled anyway.  For the 2G/3G/CPHS cases we cover most of the EFs that
are required, so we have a good handle on what is needed.  We might be
missing one or two, but certainly not dozens.  So let us keep things simple.

> 
>>
>>>
>>>>
>>>> Incidentally the above list of files to re-read is exactly the same
>>>> files we need to re-initialize when we lock ourselves out when trying to
>>>> lock/unlock/change the PIN.
>>>>
>>>>> +     switch (reset_state) {
>>>>> +     case RESET_REMOVE_ATOMS:
>>>>> +             /*
>>>>> +              * REVISIT: There's some concern that on re-insertion the
>>>>> +              * atoms will start to talk to the SIM before it becomes
>>>>> +              * ready, on certain SIMs.
>>>>> +              */
>>>>> +             ofono_sim_inserted_notify(sim, FALSE);
>>>>> +             ofono_sim_inserted_notify(sim, TRUE);
>>>>> +             break;
>>>>
>>>> This is not really a good idea semantics wise.  The SIM hasn't really
>>>> been removed and a UICC reset / NAA application reset / NAA session
>>>> reset is not being performed either.
>>>
>>> Hmm probably.  Although from our point of view the NAA initialisation
>>> looks the same as sim re-insertion so we end up inlining that fuction.
>>>
>>
>> I'm still fuzzy on what NAA initialization actually does.  Right now my
>> interpretation is that EFpl/EFli and EFiccid have not changed.  So it is
>> not quite the same as a reset.
> 
> However with Full file change notification they may have changed, so
> it's a safe fallback to re-read them.

Your guess is as good as mine right now, however we need this
functionality anyway for the 'SIM PIN locked out after we're in
sim_ready state' case anyway.  Or if lets say we get a File Changed
Notification with EFfdn for example.  There's no sense in re-reading
EFiccid, EFpl/EFli as well.

Regards,
-Denis

Re: [PATCH 2/4] sim: Implement file watching and basic refresh.

[Andrzej Zaborowski]

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

On 8 February 2011 02:25, Denis Kenzior <denkenz@gmail.com> wrote:
>>> Actually we should for files that are read during pre-sim but are not
>>> important enough.  E.g. EFecc, EFli, EFpl, EFiccid, etc.
>>
>> But if we "restart SIM initialization procedure", we already re-read
>> those files so there's nothing more we need to do.  The sim
>> initialization in 51.011 includes reading those files if I remember
>> correctly.
>>
>
> I'm still fuzzy on this one.  EFpl/EFli and EFiccid are in a different
> DF than the rest of the files, so they might not need to be initialized
> again in practice, unless we're performing a full reset.

Maybe, although it's such a rare situation that I don't think we're
gaining anything here by making an assumption that is not documented
in any spec.

I'm also fuzzy on what the differences between the Refresh types are
for us but we can't go wrong by re-reading all files.

>
> In the case of EFecc, the file is on voicecall atom, so the change watch
> has to be fired anyway.  So using my approach it works out nicely.
>
>>>
>>>>
>>>>> else
>>>>>        for each file changed:
>>>>>                notify matching remaining watches
>>>>
>>>> What if a file is changed which doesn't have a handler implemented?
>>>> Shouldn't we fall back to removing the atom? That was the idea of
>>>> those "complicated" loops above.
>>>
>>> If it should be handled, then its a bug.  Removing all atoms just
>>> because someone forgot to handle a file is bad anyway.
>>
>> We don't need to remove atoms, in this patch series we simply signal
>> "sim ready" to atoms in those cases.  Otherwise all those NULL watches
>> are useless, right?  The reason I introduced the NULL-callback watches
>> is because there are many files, like EFcfis, which are unlikely to be
>> changed by the SIM application or operator.  Yet we can't ignore them,
>> in case we receive a Refresh on those files.  We have two dozens of
>> such files and it's not practical to add notifiers for all of them,
>> it's easier to fall back to re-reading all of the post-sim files.
>
> Honestly I think we should handle all of them, one by one.  It might
> take a little while, but we have to anyway in the long run.  We can
> concentrate on the important ones first, e.g. EFcsp, EFspn, EFopl,
> EFecc, etc.  Sure, in the short term we might not handle each and every
> file that could be sent via Refresh.  But then we just acknowledge it as
> a bug and fix it later.  Re-initializing everything just because e.g.
> EFmbdn or some other unimportant file changed and we're too lazy to fix
> it is just not a good idea anyway.

So maybe we should return "Not capable" to a refresh that touches any
of those files?  At the very least we would need to log some kind of
warning.  (Then again these user-writable files are so unlikely to be
changed that I don't think we're losing anything by being safe and
re-reading everything)

>
>>
>> In the patch 3/4 I've added watches for all of the files where changes
>> need to be somehow handled.  Most of them have NULL callback because
>> they're not really important and it would be a huge patch if wanted to
>> have a handler for every single file, although we can add them
>> gradually later.  The files where changes don't need to be handled,
>> don't register a watch at all (like EFcbmi, EFcbmir which are only
>> used for initialising our own topics list)
>
> I know what you're trying to accomplish, but I don't think we need this
> flexibility.  The 'important' files that are likely to change have to be
> handled anyway.  For the 2G/3G/CPHS cases we cover most of the EFs that
> are required, so we have a good handle on what is needed.  We might be
> missing one or two, but certainly not dozens.  So let us keep things simple.
>
>>
>>>
>>>>
>>>>>
>>>>> Incidentally the above list of files to re-read is exactly the same
>>>>> files we need to re-initialize when we lock ourselves out when trying to
>>>>> lock/unlock/change the PIN.
>>>>>
>>>>>> +     switch (reset_state) {
>>>>>> +     case RESET_REMOVE_ATOMS:
>>>>>> +             /*
>>>>>> +              * REVISIT: There's some concern that on re-insertion the
>>>>>> +              * atoms will start to talk to the SIM before it becomes
>>>>>> +              * ready, on certain SIMs.
>>>>>> +              */
>>>>>> +             ofono_sim_inserted_notify(sim, FALSE);
>>>>>> +             ofono_sim_inserted_notify(sim, TRUE);
>>>>>> +             break;
>>>>>
>>>>> This is not really a good idea semantics wise.  The SIM hasn't really
>>>>> been removed and a UICC reset / NAA application reset / NAA session
>>>>> reset is not being performed either.
>>>>
>>>> Hmm probably.  Although from our point of view the NAA initialisation
>>>> looks the same as sim re-insertion so we end up inlining that fuction.
>>>>
>>>
>>> I'm still fuzzy on what NAA initialization actually does.  Right now my
>>> interpretation is that EFpl/EFli and EFiccid have not changed.  So it is
>>> not quite the same as a reset.
>>
>> However with Full file change notification they may have changed, so
>> it's a safe fallback to re-read them.
>
> Your guess is as good as mine right now, however we need this
> functionality anyway for the 'SIM PIN locked out after we're in
> sim_ready state' case anyway.  Or if lets say we get a File Changed
> Notification with EFfdn for example.  There's no sense in re-reading
> EFiccid, EFpl/EFli as well.

In case of EFfdn I think we should eventually have a handler which
will do the right thing and remove or create the affected atoms as
necessary.

But I don't see why EFpl shouldn't change during a Refresh with Full
file change notification.  I wouldn't call it a guess as much as being
ready for anything :)  It's not like this is not allowed to happen, so
there's no reason to think it can't happen.

Any of those files can also be explicitly speciied in the changed
files list.  Maybe the solution is to just use a different reset_level
value when calling sim_add_file_watch for such files.

Best regards

Re: [PATCH 2/4] sim: Implement file watching and basic refresh.

[Denis Kenzior]

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

Hi Andrew,

On 02/07/2011 08:22 PM, Andrzej Zaborowski wrote:
> On 8 February 2011 02:25, Denis Kenzior <denkenz@gmail.com> wrote:
>>>> Actually we should for files that are read during pre-sim but are not
>>>> important enough.  E.g. EFecc, EFli, EFpl, EFiccid, etc.
>>>
>>> But if we "restart SIM initialization procedure", we already re-read
>>> those files so there's nothing more we need to do.  The sim
>>> initialization in 51.011 includes reading those files if I remember
>>> correctly.
>>>
>>
>> I'm still fuzzy on this one.  EFpl/EFli and EFiccid are in a different
>> DF than the rest of the files, so they might not need to be initialized
>> again in practice, unless we're performing a full reset.
> 
> Maybe, although it's such a rare situation that I don't think we're
> gaining anything here by making an assumption that is not documented
> in any spec.
> 
> I'm also fuzzy on what the differences between the Refresh types are
> for us but we can't go wrong by re-reading all files.

And you might be right, but at the very least we still need to handle
simple File Change notifications that affect files critical to system
init.  Today that is e.g. EFfdn, EFbdn, etc.  There's no need to re-read
EFecc, or EFiccid, etc.

>> Honestly I think we should handle all of them, one by one.  It might
>> take a little while, but we have to anyway in the long run.  We can
>> concentrate on the important ones first, e.g. EFcsp, EFspn, EFopl,
>> EFecc, etc.  Sure, in the short term we might not handle each and every
>> file that could be sent via Refresh.  But then we just acknowledge it as
>> a bug and fix it later.  Re-initializing everything just because e.g.
>> EFmbdn or some other unimportant file changed and we're too lazy to fix
>> it is just not a good idea anyway.
> 
> So maybe we should return "Not capable" to a refresh that touches any
> of those files?  At the very least we would need to log some kind of
> warning.  (Then again these user-writable files are so unlikely to be
> changed that I don't think we're losing anything by being safe and
> re-reading everything)

A lot of times we simply don't have a choice to send a 'Not Capable'.
And sure we can add simple dummy handlers for all of the ones we don't
currently handle, so at least we know if something comes up.

>> Your guess is as good as mine right now, however we need this
>> functionality anyway for the 'SIM PIN locked out after we're in
>> sim_ready state' case anyway.  Or if lets say we get a File Changed
>> Notification with EFfdn for example.  There's no sense in re-reading
>> EFiccid, EFpl/EFli as well.
> 
> In case of EFfdn I think we should eventually have a handler which
> will do the right thing and remove or create the affected atoms as
> necessary.

We do not allow EFfdn enabled SIMs, so if this file is being changed
then likely we're going from FDN disabled to FDN enabled.

> 
> But I don't see why EFpl shouldn't change during a Refresh with Full
> file change notification.  I wouldn't call it a guess as much as being
> ready for anything :)  It's not like this is not allowed to happen, so
> there's no reason to think it can't happen.

You are probably right, but see the simple File Change notification case
above.

> 
> Any of those files can also be explicitly speciied in the changed
> files list.  Maybe the solution is to just use a different reset_level
> value when calling sim_add_file_watch for such files.
> 

Why complicate these things with reset level? Only the SIM atom has to
care, and we can take care of these things inside...

Regards,
-Denis

[PATCH 3/4] Watch files that ofono keeps in memory.

[Andrzej Zaborowski]

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

Add mostly dummy watches on files who's state is kept in memory,
just to ensure that on a refresh affecting any of those files ofono
re-reads them from SIM.  Only EFmsisdn, EFsdn and EFecc changes are
handled without changing sim state right now.
---
 src/call-forwarding.c |   10 ++++-
 src/cbs.c             |    2 +
 src/message-waiting.c |   13 +++++++
 src/network.c         |    9 +++++
 src/sim.c             |   95 ++++++++++++++++++++++++++++++++++++++++++++-----
 src/voicecall.c       |   24 +++++++++----
 6 files changed, 135 insertions(+), 18 deletions(-)

diff --git a/src/call-forwarding.c b/src/call-forwarding.c
index ca6dad9..0e15563 100644
--- a/src/call-forwarding.c
+++ b/src/call-forwarding.c
@@ -1362,14 +1362,20 @@ static void sim_read_cf_indicator(struct ofono_call_forwarding *cf)
 {
 	if (__ofono_sim_service_available(cf->sim,
 			SIM_UST_SERVICE_CFIS,
-			SIM_SST_SERVICE_CFIS) == TRUE)
+			SIM_SST_SERVICE_CFIS) == TRUE) {
 		ofono_sim_read(cf->sim_context, SIM_EFCFIS_FILEID,
 				OFONO_SIM_FILE_STRUCTURE_FIXED,
 				sim_cfis_read_cb, cf);
-	else
+		ofono_sim_add_file_watch(cf->sim_context, SIM_EFCFIS_FILEID,
+						NULL, cf);
+	} else {
 		ofono_sim_read(cf->sim_context, SIM_EF_CPHS_CFF_FILEID,
 				OFONO_SIM_FILE_STRUCTURE_TRANSPARENT,
 				sim_cphs_cff_read_cb, cf);
+		ofono_sim_add_file_watch(cf->sim_context,
+						SIM_EF_CPHS_CFF_FILEID,
+						NULL, cf);
+	}
 }
 
 int ofono_call_forwarding_driver_register(const struct ofono_call_forwarding_driver *d)
diff --git a/src/cbs.c b/src/cbs.c
index 1220d0b..e50e0d0 100644
--- a/src/cbs.c
+++ b/src/cbs.c
@@ -928,6 +928,8 @@ static void cbs_got_imsi(struct ofono_cbs *cbs)
 	ofono_sim_read(cbs->sim_context, SIM_EFCBMID_FILEID,
 			OFONO_SIM_FILE_STRUCTURE_TRANSPARENT,
 			sim_cbmid_read_cb, cbs);
+	ofono_sim_add_file_watch(cbs->sim_context, SIM_EFCBMID_FILEID,
+					NULL, cbs);
 }
 
 static gboolean reset_base_station_name(gpointer user)
diff --git a/src/message-waiting.c b/src/message-waiting.c
index c0293e7..6a959ec 100644
--- a/src/message-waiting.c
+++ b/src/message-waiting.c
@@ -981,6 +981,19 @@ void ofono_message_waiting_register(struct ofono_message_waiting *mw)
 		ofono_sim_read(mw->sim_context, SIM_EF_CPHS_MWIS_FILEID,
 				OFONO_SIM_FILE_STRUCTURE_TRANSPARENT,
 				mw_cphs_mwis_read_cb, mw);
+
+		ofono_sim_add_file_watch(mw->sim_context, SIM_EFMWIS_FILEID,
+						NULL, mw);
+		ofono_sim_add_file_watch(mw->sim_context,
+						SIM_EF_CPHS_MWIS_FILEID,
+						NULL, mw);
+		ofono_sim_add_file_watch(mw->sim_context, SIM_EFMBI_FILEID,
+						NULL, mw);
+		ofono_sim_add_file_watch(mw->sim_context, SIM_EFMBDN_FILEID,
+						NULL, mw);
+		ofono_sim_add_file_watch(mw->sim_context,
+						SIM_EF_CPHS_MBDN_FILEID,
+						NULL, mw);
 	}
 
 	__ofono_atom_register(mw->atom, message_waiting_unregister);
diff --git a/src/network.c b/src/network.c
index d94f696..8190142 100644
--- a/src/network.c
+++ b/src/network.c
@@ -1826,6 +1826,15 @@ void ofono_netreg_register(struct ofono_netreg *netreg)
 		ofono_sim_read(netreg->sim_context, SIM_EFSPN_FILEID,
 				OFONO_SIM_FILE_STRUCTURE_TRANSPARENT,
 				sim_spn_read_cb, netreg);
+
+		ofono_sim_add_file_watch(netreg->sim_context, SIM_EFPNN_FILEID,
+						NULL, netreg);
+		ofono_sim_add_file_watch(netreg->sim_context, SIM_EFSPN_FILEID,
+						NULL, netreg);
+		ofono_sim_add_file_watch(netreg->sim_context, SIM_EFSPDI_FILEID,
+						NULL, netreg);
+		ofono_sim_add_file_watch(netreg->sim_context, SIM_EFOPL_FILEID,
+						NULL, netreg);
 	}
 
 	__ofono_atom_register(netreg->atom, netreg_unregister);
diff --git a/src/sim.c b/src/sim.c
index 55d4d75..dd1efce 100644
--- a/src/sim.c
+++ b/src/sim.c
@@ -980,6 +980,7 @@ static void sim_iidf_read_cb(int ok, int length, int record,
 	/* read the clut data */
 	ofono_sim_read_bytes(sim->context, iidf_id, offset, clut_len,
 					sim_iidf_read_clut_cb, sim);
+	ofono_sim_add_file_watch(sim->context, iidf_id, NULL, sim);
 }
 
 static void sim_get_image(struct ofono_sim *sim, unsigned char id,
@@ -1249,10 +1250,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);
 
@@ -1265,6 +1268,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,
@@ -1272,6 +1290,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)
@@ -1316,16 +1341,46 @@ static void sim_efimg_read_cb(int ok, int length, int record,
 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,
+						sim_own_numbers_changed, sim);
+		ofono_sim_remove_file_watch(sim->context, SIM_EFSDN_FILEID,
+						sim_service_numbers_changed,
+						sim);
+		ofono_sim_remove_file_watch(sim->context, SIM_EFIMG_FILEID,
+						NULL, sim);
+
+		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,
+							NULL, sim);
+		}
 
-	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);
 
 	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);
+
 	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, NULL, sim);
 }
 
 static void sim_imsi_cb(const struct ofono_error *error, const char *imsi,
@@ -1444,6 +1499,9 @@ static gboolean check_bdn_status(struct ofono_sim *sim)
 		sim_fs_read_info(sim->context, SIM_EFBDN_FILEID,
 				OFONO_SIM_FILE_STRUCTURE_FIXED,
 				sim_efbdn_info_read_cb, sim);
+		sim_add_file_watch(sim->context, SIM_EFBDN_FILEID,
+					TRUE, NULL, sim);
+
 		return TRUE;
 	}
 
@@ -1498,6 +1556,9 @@ static void sim_efsst_read_cb(int ok, int length, int record,
 		sim_fs_read_info(sim->context, SIM_EFADN_FILEID,
 					OFONO_SIM_FILE_STRUCTURE_FIXED,
 					sim_efadn_info_read_cb, sim);
+		sim_add_file_watch(sim->context, SIM_EFADN_FILEID,
+					TRUE, NULL, sim);
+
 		return;
 	}
 
@@ -1588,6 +1649,8 @@ static void sim_efust_read_cb(int ok, int length, int record,
 		ofono_sim_read(sim->context, SIM_EFEST_FILEID,
 				OFONO_SIM_FILE_STRUCTURE_TRANSPARENT,
 				sim_efest_read_cb, sim);
+		sim_add_file_watch(sim->context, SIM_EFEST_FILEID,
+					TRUE, NULL, sim);
 
 		return;
 	}
@@ -1649,6 +1712,8 @@ static void sim_efphase_read_cb(int ok, int length, int record,
 		ofono_sim_read(sim->context, SIM_EFUST_FILEID,
 				OFONO_SIM_FILE_STRUCTURE_TRANSPARENT,
 				sim_efust_read_cb, sim);
+		sim_add_file_watch(sim->context, SIM_EFUST_FILEID,
+					TRUE, NULL, sim);
 
 		return;
 	}
@@ -1671,6 +1736,7 @@ static void sim_efphase_read_cb(int ok, int length, int record,
 	ofono_sim_read(sim->context, SIM_EFSST_FILEID,
 			OFONO_SIM_FILE_STRUCTURE_TRANSPARENT,
 			sim_efsst_read_cb, sim);
+	sim_add_file_watch(sim->context, SIM_EFSST_FILEID, TRUE, NULL, sim);
 }
 
 static void sim_initialize_after_pin(struct ofono_sim *sim)
@@ -1678,10 +1744,12 @@ static void sim_initialize_after_pin(struct ofono_sim *sim)
 	ofono_sim_read(sim->context, SIM_EFPHASE_FILEID,
 			OFONO_SIM_FILE_STRUCTURE_TRANSPARENT,
 			sim_efphase_read_cb, sim);
+	sim_add_file_watch(sim->context, SIM_EFPHASE_FILEID, TRUE, NULL, sim);
 
 	ofono_sim_read(sim->context, SIM_EFAD_FILEID,
 			OFONO_SIM_FILE_STRUCTURE_TRANSPARENT,
 			sim_ad_read_cb, sim);
+	sim_add_file_watch(sim->context, SIM_EFAD_FILEID, TRUE, NULL, sim);
 
 	/*
 	 * Read CPHS-support bits, this is still part of the SIM
@@ -1690,6 +1758,8 @@ static void sim_initialize_after_pin(struct ofono_sim *sim)
 	ofono_sim_read(sim->context, SIM_EF_CPHS_INFORMATION_FILEID,
 			OFONO_SIM_FILE_STRUCTURE_TRANSPARENT,
 			sim_cphs_information_read_cb, sim);
+	sim_add_file_watch(sim->context, SIM_EF_CPHS_INFORMATION_FILEID,
+				TRUE, NULL, sim);
 }
 
 static void sim_pin_query_cb(const struct ofono_error *error,
@@ -1978,10 +2048,14 @@ 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);
+	sim_add_file_watch(sim->context, SIM_EF_ICCID_FILEID, TRUE, NULL, sim);
 
 	/* EFecc is read by the voicecall atom */
 
@@ -1996,9 +2070,12 @@ 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);
+	sim_add_file_watch(sim->context, SIM_EFLI_FILEID, TRUE, NULL, sim);
+
 	ofono_sim_read(sim->context, SIM_EFPL_FILEID,
 			OFONO_SIM_FILE_STRUCTURE_TRANSPARENT,
 			sim_efpl_read_cb, sim);
+	sim_add_file_watch(sim->context, SIM_EFPL_FILEID, TRUE, NULL, sim);
 }
 
 struct ofono_sim_context *ofono_sim_context_create(struct ofono_sim *sim)
@@ -2151,6 +2228,7 @@ static void sim_free_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->efli) {
@@ -2190,6 +2268,11 @@ static void sim_free_state(struct ofono_sim *sim)
 		sim->efimg_length = 0;
 	}
 
+	if (sim->context) {
+		ofono_sim_context_free(sim->context);
+		sim->context = NULL;
+	}
+
 	g_free(sim->iidf_image);
 	sim->iidf_image = NULL;
 
@@ -2333,11 +2416,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;
-	}
-
 	sim_fs_free(sim->simfs);
 	sim->simfs = NULL;
 
@@ -2409,7 +2487,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);
 
 	__ofono_atom_register(sim->atom, sim_unregister);
 
diff --git a/src/voicecall.c b/src/voicecall.c
index 7632c0d..1a85f91 100644
--- a/src/voicecall.c
+++ b/src/voicecall.c
@@ -2279,6 +2279,19 @@ struct ofono_voicecall *ofono_voicecall_create(struct ofono_modem *modem,
 	return vc;
 }
 
+static void read_ecc_numbers(int id, void *userdata)
+{
+	struct ofono_voicecall *vc = userdata;
+
+	/* Try both formats, only one or none will work */
+	ofono_sim_read(vc->sim_context, SIM_EFECC_FILEID,
+			OFONO_SIM_FILE_STRUCTURE_TRANSPARENT,
+			ecc_g2_read_cb, vc);
+	ofono_sim_read(vc->sim_context, SIM_EFECC_FILEID,
+			OFONO_SIM_FILE_STRUCTURE_FIXED,
+			ecc_g3_read_cb, vc);
+}
+
 static void sim_state_watch(enum ofono_sim_state new_state, void *user)
 {
 	struct ofono_voicecall *vc = user;
@@ -2288,13 +2301,10 @@ static void sim_state_watch(enum ofono_sim_state new_state, void *user)
 		if (vc->sim_context == NULL)
 			vc->sim_context = ofono_sim_context_create(vc->sim);
 
-		/* Try both formats, only one or none will work */
-		ofono_sim_read(vc->sim_context, SIM_EFECC_FILEID,
-				OFONO_SIM_FILE_STRUCTURE_TRANSPARENT,
-				ecc_g2_read_cb, vc);
-		ofono_sim_read(vc->sim_context, SIM_EFECC_FILEID,
-				OFONO_SIM_FILE_STRUCTURE_FIXED,
-				ecc_g3_read_cb, vc);
+		read_ecc_numbers(SIM_EFECC_FILEID, vc);
+
+		ofono_sim_add_file_watch(vc->sim_context, SIM_EFECC_FILEID,
+						read_ecc_numbers, vc);
 		break;
 	case OFONO_SIM_STATE_NOT_PRESENT:
 		/* TODO: Must release all non-emergency calls */
-- 
1.7.1.86.g0e460.dirty

[PATCH 4/4] stk: Partially handle Refresh command.

[Andrzej Zaborowski]

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

Only the four "NAA initialisation" modes are handled at the moment.
---
 src/stk.c |   65 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 65 insertions(+), 0 deletions(-)

diff --git a/src/stk.c b/src/stk.c
index 199e02b..bc46b2f 100644
--- a/src/stk.c
+++ b/src/stk.c
@@ -1973,10 +1973,22 @@ static gboolean handle_command_send_ussd(const struct stk_command *cmd,
 	return FALSE;
 }
 
+static void free_idle_mode_text(struct ofono_stk *stk)
+{
+	g_free(stk->idle_mode_text);
+	stk->idle_mode_text = NULL;
+
+	memset(&stk->idle_mode_icon, 0, sizeof(stk->idle_mode_icon));
+}
+
 static gboolean handle_command_refresh(const struct stk_command *cmd,
 					struct stk_response *rsp,
 					struct ofono_stk *stk)
 {
+	struct ofono_error failure = { .type = OFONO_ERROR_TYPE_FAILURE };
+	struct ofono_sim *sim = NULL;
+	struct ofono_atom *sim_atom;
+	int err;
 	GSList *l;
 
 	DBG("");
@@ -2030,6 +2042,59 @@ static gboolean handle_command_refresh(const struct stk_command *cmd,
 					cmd->refresh.icon_id.qualifier);
 	DBG("Alpha ID: %s", cmd->refresh.alpha_id);
 
+	sim_atom = __ofono_modem_find_atom(__ofono_atom_get_modem(stk->atom),
+						OFONO_ATOM_TYPE_SIM);
+	if (sim_atom)
+		sim = __ofono_atom_get_data(sim_atom);
+
+	if (sim == NULL) {
+		rsp->result.type = STK_RESULT_TYPE_NOT_CAPABLE;
+		return TRUE;
+	}
+
+	if (cmd->qualifier < 4) {
+		int qualifier = stk->pending_cmd->qualifier;
+		GSList *file_list = stk->pending_cmd->refresh.file_list;
+
+		/* Don't free the list yet */
+		stk->pending_cmd->refresh.file_list = NULL;
+
+		/*
+		 * Queue the TERMINAL RESPONSE before triggering potential
+		 * file accesses.
+		 */
+		err = stk_respond(stk, rsp, stk_command_cb);
+		if (err)
+			stk_command_cb(&failure, stk);
+
+		/* TODO: use the alphaId / icon */
+		/* TODO: if AID is supplied, check its value */
+		/* TODO: possibly check if a D-bus call is pending or
+		 * an STK session ongoing. */
+
+		/* TODO: free some elements of the atom state */
+
+		switch (qualifier) {
+		case 0:
+			free_idle_mode_text(stk);
+			__ofono_sim_refresh(sim, file_list, TRUE, TRUE);
+			break;
+		case 1:
+			__ofono_sim_refresh(sim, file_list, FALSE, FALSE);
+			break;
+		case 2:
+		case 3:
+			free_idle_mode_text(stk);
+			__ofono_sim_refresh(sim, file_list, FALSE, TRUE);
+			break;
+		}
+
+		g_slist_foreach(file_list, (GFunc) g_free, NULL);
+		g_slist_free(file_list);
+
+		return FALSE;
+	}
+
 	rsp->result.type = STK_RESULT_TYPE_NOT_CAPABLE;
 	return TRUE;
 }
-- 
1.7.1.86.g0e460.dirty

Re: [PATCH 4/4] stk: Partially handle Refresh command.

[Denis Kenzior]

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

Hi Andrew,

> @@ -2030,6 +2042,59 @@ static gboolean handle_command_refresh(const struct stk_command *cmd,
>  					cmd->refresh.icon_id.qualifier);
>  	DBG("Alpha ID: %s", cmd->refresh.alpha_id);
>  
> +	sim_atom = __ofono_modem_find_atom(__ofono_atom_get_modem(stk->atom),
> +						OFONO_ATOM_TYPE_SIM);
> +	if (sim_atom)
> +		sim = __ofono_atom_get_data(sim_atom);
> +
> +	if (sim == NULL) {
> +		rsp->result.type = STK_RESULT_TYPE_NOT_CAPABLE;
> +		return TRUE;
> +	}
> +
> +	if (cmd->qualifier < 4) {
> +		int qualifier = stk->pending_cmd->qualifier;
> +		GSList *file_list = stk->pending_cmd->refresh.file_list;
> +
> +		/* Don't free the list yet */
> +		stk->pending_cmd->refresh.file_list = NULL;
> +
> +		/*
> +		 * Queue the TERMINAL RESPONSE before triggering potential
> +		 * file accesses.
> +		 */
> +		err = stk_respond(stk, rsp, stk_command_cb);
> +		if (err)
> +			stk_command_cb(&failure, stk);

So my question here is should we delay calling refresh until after our
terminal response has been accepted?

> +
> +		/* TODO: use the alphaId / icon */
> +		/* TODO: if AID is supplied, check its value */
> +		/* TODO: possibly check if a D-bus call is pending or
> +		 * an STK session ongoing. */
> +
> +		/* TODO: free some elements of the atom state */
> +
> +		switch (qualifier) {
> +		case 0:
> +			free_idle_mode_text(stk);
> +			__ofono_sim_refresh(sim, file_list, TRUE, TRUE);
> +			break;
> +		case 1:
> +			__ofono_sim_refresh(sim, file_list, FALSE, FALSE);
> +			break;
> +		case 2:
> +		case 3:
> +			free_idle_mode_text(stk);
> +			__ofono_sim_refresh(sim, file_list, FALSE, TRUE);
> +			break;
> +		}
> +
> +		g_slist_foreach(file_list, (GFunc) g_free, NULL);
> +		g_slist_free(file_list);
> +
> +		return FALSE;
> +	}
> +
>  	rsp->result.type = STK_RESULT_TYPE_NOT_CAPABLE;
>  	return TRUE;
>  }

Should we be handling the case where refresh is notified via
ofono_stk_proactive_command_handled_notify as well?

Regards,
-Denis

Re: [PATCH 1/4] Add SIM filesystem watch api.

[Denis Kenzior]

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

Hi Andrew,

On 02/03/2011 07:20 PM, Andrzej Zaborowski wrote:
> This allows code that reads files from the SIM card to also watch for
> changes in these files signalled in a Refresh command by the SIM.
> ---
>  include/sim.h |    9 +++++++++
>  1 files changed, 9 insertions(+), 0 deletions(-)
> 

So I pushed my own implementation of these based on ofono_watchlist.  I
think it was a bit cleaner than your proposal.  Can you check that these
patches make sense to you?

Regards,
-Denis