[PATCH 1/5] sim: Cache flushing functions.

[PATCH 1/5] sim: Cache flushing functions.

[Andrzej Zaborowski]

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

---
 src/sim.c   |   23 ++++++++++++++++++++
 src/simfs.c |   68 ++++++++++++++++++++++++++++++++++++++++++++--------------
 src/simfs.h |    5 ++++
 3 files changed, 79 insertions(+), 17 deletions(-)

diff --git a/src/sim.c b/src/sim.c
index 335f611..27ff572 100644
--- a/src/sim.c
+++ b/src/sim.c
@@ -2255,3 +2255,26 @@ 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);
+}
diff --git a/src/simfs.c b/src/simfs.c
index 617af14..a99a2c6 100644
--- a/src/simfs.c
+++ b/src/simfs.c
@@ -907,9 +907,6 @@ void sim_fs_check_version(struct sim_fs *fs)
 	const char *imsi = ofono_sim_get_imsi(fs->sim);
 	enum ofono_sim_phase phase = ofono_sim_get_phase(fs->sim);
 	unsigned char version;
-	struct dirent **entries;
-	int len;
-	char *path;
 
 	if (imsi == NULL || phase == OFONO_SIM_PHASE_UNKNOWN)
 		return;
@@ -918,10 +915,20 @@ void sim_fs_check_version(struct sim_fs *fs)
 		if (version == SIM_FS_VERSION)
 			return;
 
-	path = g_strdup_printf(SIM_CACHE_BASEPATH, imsi, phase);
+	sim_fs_cache_flush(fs);
+
+	version = SIM_FS_VERSION;
+	write_file(&version, 1, SIM_CACHE_MODE, SIM_CACHE_VERSION, imsi, phase);
+}
+
+void sim_fs_cache_flush(struct sim_fs *fs)
+{
+	const char *imsi = ofono_sim_get_imsi(fs->sim);
+	enum ofono_sim_phase phase = ofono_sim_get_phase(fs->sim);
+	char *path = g_strdup_printf(SIM_CACHE_BASEPATH, imsi, phase);
+	struct dirent **entries;
+	int len = scandir(path, &entries, NULL, alphasort);
 
-	ofono_info("Detected old simfs version in %s, removing", path);
-	len = scandir(path, &entries, NULL, alphasort);
 	g_free(path);
 
 	if (len > 0) {
@@ -934,20 +941,47 @@ void sim_fs_check_version(struct sim_fs *fs)
 		g_free(entries);
 	}
 
-	path = g_strdup_printf(SIM_IMAGE_CACHE_BASEPATH, imsi, phase);
-	len = scandir(path, &entries, NULL, alphasort);
+	sim_fs_image_cache_flush(fs);
+}
+
+void sim_fs_cache_flush_file(struct sim_fs *fs, int id)
+{
+	const char *imsi = ofono_sim_get_imsi(fs->sim);
+	enum ofono_sim_phase phase = ofono_sim_get_phase(fs->sim);
+	char *path = g_strdup_printf(SIM_CACHE_PATH, imsi, phase, id);
+
+	remove(path);
 	g_free(path);
+}
 
-	if (len > 0) {
-		/* Remove everything */
-		while (len--) {
-			remove_imagefile(imsi, phase, entries[len]);
-			g_free(entries[len]);
-		}
+void sim_fs_image_cache_flush(struct sim_fs *fs)
+{
+	const char *imsi = ofono_sim_get_imsi(fs->sim);
+	enum ofono_sim_phase phase = ofono_sim_get_phase(fs->sim);
+	char *path = g_strdup_printf(SIM_IMAGE_CACHE_BASEPATH, imsi, phase);
+	struct dirent **entries;
+	int len = scandir(path, &entries, NULL, alphasort);
 
-		g_free(entries);
+	g_free(path);
+
+	if (len == 0)
+		return;
+
+	/* Remove everything */
+	while (len--) {
+		remove_imagefile(imsi, phase, entries[len]);
+		g_free(entries[len]);
 	}
 
-	version = SIM_FS_VERSION;
-	write_file(&version, 1, SIM_CACHE_MODE, SIM_CACHE_VERSION, imsi, phase);
+	g_free(entries);
+}
+
+void sim_fs_image_cache_flush_file(struct sim_fs *fs, int id)
+{
+	const char *imsi = ofono_sim_get_imsi(fs->sim);
+	enum ofono_sim_phase phase = ofono_sim_get_phase(fs->sim);
+	char *path = g_strdup_printf(SIM_IMAGE_CACHE_PATH, imsi, phase, id);
+
+	remove(path);
+	g_free(path);
 }
diff --git a/src/simfs.h b/src/simfs.h
index ef962db..8c6f761 100644
--- a/src/simfs.h
+++ b/src/simfs.h
@@ -47,4 +47,9 @@ char *sim_fs_get_cached_image(struct sim_fs *fs, int id);
 
 void sim_fs_cache_image(struct sim_fs *fs, const char *image, int id);
 
+void sim_fs_cache_flush(struct sim_fs *fs);
+void sim_fs_cache_flush_file(struct sim_fs *fs, int id);
+void sim_fs_image_cache_flush(struct sim_fs *fs);
+void sim_fs_image_cache_flush_file(struct sim_fs *fs, int id);
+
 void sim_fs_free(struct sim_fs *fs);
-- 
1.7.1.86.g0e460.dirty

[PATCH 1/5] sim: Cache flushing functions.

[Andrzej Zaborowski]

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

---
 src/sim.c   |   23 ++++++++++++++++++++
 src/simfs.c |   68 ++++++++++++++++++++++++++++++++++++++++++++--------------
 src/simfs.h |    5 ++++
 3 files changed, 79 insertions(+), 17 deletions(-)

diff --git a/src/sim.c b/src/sim.c
index 335f611..27ff572 100644
--- a/src/sim.c
+++ b/src/sim.c
@@ -2255,3 +2255,26 @@ 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);
+}
diff --git a/src/simfs.c b/src/simfs.c
index 617af14..b77e1dc 100644
--- a/src/simfs.c
+++ b/src/simfs.c
@@ -907,9 +907,6 @@ void sim_fs_check_version(struct sim_fs *fs)
 	const char *imsi = ofono_sim_get_imsi(fs->sim);
 	enum ofono_sim_phase phase = ofono_sim_get_phase(fs->sim);
 	unsigned char version;
-	struct dirent **entries;
-	int len;
-	char *path;
 
 	if (imsi == NULL || phase == OFONO_SIM_PHASE_UNKNOWN)
 		return;
@@ -918,10 +915,20 @@ void sim_fs_check_version(struct sim_fs *fs)
 		if (version == SIM_FS_VERSION)
 			return;
 
-	path = g_strdup_printf(SIM_CACHE_BASEPATH, imsi, phase);
+	sim_fs_cache_flush(fs);
+
+	version = SIM_FS_VERSION;
+	write_file(&version, 1, SIM_CACHE_MODE, SIM_CACHE_VERSION, imsi, phase);
+}
+
+void sim_fs_cache_flush(struct sim_fs *fs)
+{
+	const char *imsi = ofono_sim_get_imsi(fs->sim);
+	enum ofono_sim_phase phase = ofono_sim_get_phase(fs->sim);
+	char *path = g_strdup_printf(SIM_CACHE_BASEPATH, imsi, phase);
+	struct dirent **entries;
+	int len = scandir(path, &entries, NULL, alphasort);
 
-	ofono_info("Detected old simfs version in %s, removing", path);
-	len = scandir(path, &entries, NULL, alphasort);
 	g_free(path);
 
 	if (len > 0) {
@@ -934,20 +941,47 @@ void sim_fs_check_version(struct sim_fs *fs)
 		g_free(entries);
 	}
 
-	path = g_strdup_printf(SIM_IMAGE_CACHE_BASEPATH, imsi, phase);
-	len = scandir(path, &entries, NULL, alphasort);
+	sim_fs_image_cache_flush(fs);
+}
+
+void sim_fs_cache_flush_file(struct sim_fs *fs, int id)
+{
+	const char *imsi = ofono_sim_get_imsi(fs->sim);
+	enum ofono_sim_phase phase = ofono_sim_get_phase(fs->sim);
+	char *path = g_strdup_printf(SIM_CACHE_PATH, imsi, phase, id);
+
+	remove(path);
 	g_free(path);
+}
 
-	if (len > 0) {
-		/* Remove everything */
-		while (len--) {
-			remove_imagefile(imsi, phase, entries[len]);
-			g_free(entries[len]);
-		}
+void sim_fs_image_cache_flush(struct sim_fs *fs)
+{
+	const char *imsi = ofono_sim_get_imsi(fs->sim);
+	enum ofono_sim_phase phase = ofono_sim_get_phase(fs->sim);
+	char *path = g_strdup_printf(SIM_IMAGE_CACHE_BASEPATH, imsi, phase);
+	struct dirent **entries;
+	int len = scandir(path, &entries, NULL, alphasort);
 
-		g_free(entries);
+	g_free(path);
+
+	if (len <= 0)
+		return;
+
+	/* Remove everything */
+	while (len--) {
+		remove_imagefile(imsi, phase, entries[len]);
+		g_free(entries[len]);
 	}
 
-	version = SIM_FS_VERSION;
-	write_file(&version, 1, SIM_CACHE_MODE, SIM_CACHE_VERSION, imsi, phase);
+	g_free(entries);
+}
+
+void sim_fs_image_cache_flush_file(struct sim_fs *fs, int id)
+{
+	const char *imsi = ofono_sim_get_imsi(fs->sim);
+	enum ofono_sim_phase phase = ofono_sim_get_phase(fs->sim);
+	char *path = g_strdup_printf(SIM_IMAGE_CACHE_PATH, imsi, phase, id);
+
+	remove(path);
+	g_free(path);
 }
diff --git a/src/simfs.h b/src/simfs.h
index ef962db..8c6f761 100644
--- a/src/simfs.h
+++ b/src/simfs.h
@@ -47,4 +47,9 @@ char *sim_fs_get_cached_image(struct sim_fs *fs, int id);
 
 void sim_fs_cache_image(struct sim_fs *fs, const char *image, int id);
 
+void sim_fs_cache_flush(struct sim_fs *fs);
+void sim_fs_cache_flush_file(struct sim_fs *fs, int id);
+void sim_fs_image_cache_flush(struct sim_fs *fs);
+void sim_fs_image_cache_flush_file(struct sim_fs *fs, int id);
+
 void sim_fs_free(struct sim_fs *fs);
-- 
1.7.1.86.g0e460.dirty

[PATCH 2/5] Add SIM filesystem watch api.

[Andrzej Zaborowski]

[-- Attachment #1: Type: text/plain, Size: 1325 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 |   10 ++++++++++
 1 files changed, 10 insertions(+), 0 deletions(-)

diff --git a/include/sim.h b/include/sim.h
index 7860e24..aab981c 100644
--- a/include/sim.h
+++ b/include/sim.h
@@ -114,6 +114,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);
@@ -207,6 +209,14 @@ int ofono_sim_write(struct ofono_sim *sim, int id,
 int ofono_sim_read_bytes(struct ofono_sim *sim, 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 *sim, int id,
+			enum ofono_sim_state reset_state,
+			ofono_sim_file_notify_t notify, void *userdata);
+
+void ofono_sim_remove_file_watch(struct ofono_sim *sim, int id,
+			ofono_sim_file_notify_t notify, void *userdata);
+
 #ifdef __cplusplus
 }
 #endif
-- 
1.7.1.86.g0e460.dirty

[PATCH 3/5] sim: Implement file watching and basic refresh.

[Andrzej Zaborowski]

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

---
 src/ofono.h |    4 +
 src/sim.c   |  185 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 189 insertions(+), 0 deletions(-)

diff --git a/src/ofono.h b/src/ofono.h
index cab70cd..4c0a8e1 100644
--- a/src/ofono.h
+++ b/src/ofono.h
@@ -297,6 +297,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 27ff572..7923302 100644
--- a/src/sim.c
+++ b/src/sim.c
@@ -92,6 +92,7 @@ struct ofono_sim {
 	struct ofono_watchlist *state_watches;
 
 	struct sim_fs *simfs;
+	GSList *fs_watches;
 
 	unsigned char *iidf_image;
 
@@ -113,6 +114,13 @@ 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;
+};
+
 static const char *const passwd_name[] = {
 	[OFONO_SIM_PASSWORD_NONE] = "none",
 	[OFONO_SIM_PASSWORD_SIM_PIN] = "pin",
@@ -2174,6 +2182,12 @@ static void sim_remove(struct ofono_atom *atom)
 		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);
 }
 
@@ -2278,3 +2292,174 @@ static void sim_file_changed_flush(struct ofono_sim *sim, int id)
 
 	sim_fs_cache_flush_file(sim->simfs, id);
 }
+
+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;
+}
+
+int ofono_sim_add_file_watch(struct ofono_sim *sim, int id,
+			enum ofono_sim_state reset_state,
+			ofono_sim_file_notify_t notify, void *userdata)
+{
+	struct fs_watch *item;
+
+	DBG("%p", sim);
+
+	if (sim == NULL)
+		return 0;
+
+	if (notify == 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 = reset_state;
+
+	sim->fs_watches = g_slist_prepend(sim->fs_watches, item);
+
+	return id;
+}
+
+void ofono_sim_remove_file_watch(struct ofono_sim *sim, int id,
+			ofono_sim_file_notify_t notify, void *userdata)
+{
+	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, *filel;
+	enum sim_reset_state {
+		RESET_STATE_NOT_PRESENT	= OFONO_SIM_STATE_NOT_PRESENT,
+		RESET_STATE_INSERTED	= OFONO_SIM_STATE_INSERTED,
+		RESET_STATE_READY	= OFONO_SIM_STATE_READY,
+		RESET_STATE_NONE,
+	} reset_state = RESET_STATE_NONE;
+
+	/* Flush cached content for affected files */
+	if (full_file_change)
+		sim_fs_cache_flush(sim->simfs);
+	else
+		for (filel = file_list; filel; filel = filel->next) {
+			struct stk_file *file = filel->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_STATE_INSERTED;
+
+	/*
+	 * 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 (filel = file_list; filel; filel = filel->next) {
+			struct stk_file *file = filel->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 (filel = file_list; filel; filel = filel->next) {
+			struct stk_file *file = filel->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_STATE_NOT_PRESENT:
+		ofono_sim_inserted_notify(sim, FALSE);
+		ofono_sim_inserted_notify(sim, TRUE);
+		break;
+	case RESET_STATE_INSERTED:
+		sim_free_state(sim);
+		sim->state = OFONO_SIM_STATE_INSERTED;
+		sim_initialize(sim);
+		break;
+	case RESET_STATE_READY:
+		sim->state = OFONO_SIM_STATE_INSERTED;
+		for (l = sim->state_watches->items; l; l = l->next) {
+			struct ofono_watchlist_item *item = l->data;
+			ofono_sim_state_event_cb_t notify = item->notify;
+
+			notify(sim->state, item->notify_data);
+		}
+
+		sim_set_ready(sim);
+		break;
+	case RESET_STATE_NONE:
+		break;
+	}
+}
-- 
1.7.1.86.g0e460.dirty

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

[Andrzej Zaborowski]

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

These are (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 |   18 ++++++++-
 src/cbs.c             |    6 +++
 src/message-waiting.c |   30 ++++++++++++++++
 src/network.c         |   24 +++++++++++++
 src/sim.c             |   92 +++++++++++++++++++++++++++++++++++++++++++++++--
 src/voicecall.c       |   28 +++++++++++----
 6 files changed, 186 insertions(+), 12 deletions(-)

diff --git a/src/call-forwarding.c b/src/call-forwarding.c
index 512f223..981f4a6 100644
--- a/src/call-forwarding.c
+++ b/src/call-forwarding.c
@@ -1362,14 +1362,21 @@ 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, SIM_EFCFIS_FILEID,
 				OFONO_SIM_FILE_STRUCTURE_FIXED,
 				sim_cfis_read_cb, cf);
-	else
+		ofono_sim_add_file_watch(cf->sim, SIM_EFCFIS_FILEID,
+						OFONO_SIM_STATE_READY,
+						NULL, cf);
+	} else {
 		ofono_sim_read(cf->sim, SIM_EF_CPHS_CFF_FILEID,
 				OFONO_SIM_FILE_STRUCTURE_TRANSPARENT,
 				sim_cphs_cff_read_cb, cf);
+		ofono_sim_add_file_watch(cf->sim, SIM_EF_CPHS_CFF_FILEID,
+						OFONO_SIM_STATE_READY,
+						NULL, cf);
+	}
 }
 
 int ofono_call_forwarding_driver_register(const struct ofono_call_forwarding_driver *d)
@@ -1407,6 +1414,13 @@ static void call_forwarding_unregister(struct ofono_atom *atom)
 
 	if (cf->ussd_watch)
 		__ofono_modem_remove_atom_watch(modem, cf->ussd_watch);
+
+	if (cf->sim) {
+		ofono_sim_remove_file_watch(cf->sim, SIM_EFCFIS_FILEID,
+						NULL, cf);
+		ofono_sim_remove_file_watch(cf->sim, SIM_EF_CPHS_CFF_FILEID,
+						NULL, cf);
+	}
 }
 
 static void call_forwarding_remove(struct ofono_atom *atom)
diff --git a/src/cbs.c b/src/cbs.c
index 8e4afc1..4efa7b2 100644
--- a/src/cbs.c
+++ b/src/cbs.c
@@ -605,6 +605,10 @@ static void cbs_unregister(struct ofono_atom *atom)
 		cbs->efcbmid_contents = NULL;
 	}
 
+	if (cbs->sim)
+		ofono_sim_remove_file_watch(cbs->sim, SIM_EFCBMID_FILEID,
+						NULL, cbs);
+
 	cbs->sim = NULL;
 	cbs->stk = NULL;
 
@@ -922,6 +926,8 @@ static void cbs_got_imsi(struct ofono_cbs *cbs)
 	ofono_sim_read(cbs->sim, SIM_EFCBMID_FILEID,
 			OFONO_SIM_FILE_STRUCTURE_TRANSPARENT,
 			sim_cbmid_read_cb, cbs);
+	ofono_sim_add_file_watch(cbs->sim, SIM_EFCBMID_FILEID,
+					OFONO_SIM_STATE_READY, NULL, cbs);
 }
 
 static gboolean reset_base_station_name(gpointer user)
diff --git a/src/message-waiting.c b/src/message-waiting.c
index d8bfe34..0ff2fb4 100644
--- a/src/message-waiting.c
+++ b/src/message-waiting.c
@@ -915,10 +915,24 @@ void __ofono_message_waiting_mwi(struct ofono_message_waiting *mw,
 
 static void message_waiting_unregister(struct ofono_atom *atom)
 {
+	struct ofono_message_waiting *mw = __ofono_atom_get_data(atom);
 	DBusConnection *conn = ofono_dbus_get_connection();
 	struct ofono_modem *modem = __ofono_atom_get_modem(atom);
 	const char *path = __ofono_atom_get_path(atom);
 
+	if (mw->sim) {
+		ofono_sim_remove_file_watch(mw->sim, SIM_EFMWIS_FILEID,
+						NULL, mw);
+		ofono_sim_remove_file_watch(mw->sim, SIM_EF_CPHS_MWIS_FILEID,
+						NULL, mw);
+		ofono_sim_remove_file_watch(mw->sim, SIM_EFMBI_FILEID,
+						NULL, mw);
+		ofono_sim_remove_file_watch(mw->sim, SIM_EFMBDN_FILEID,
+						NULL, mw);
+		ofono_sim_remove_file_watch(mw->sim, SIM_EF_CPHS_MBDN_FILEID,
+						NULL, mw);
+	}
+
 	g_dbus_unregister_interface(conn, path,
 					OFONO_MESSAGE_WAITING_INTERFACE);
 	ofono_modem_remove_interface(modem, OFONO_MESSAGE_WAITING_INTERFACE);
@@ -961,6 +975,22 @@ void ofono_message_waiting_register(struct ofono_message_waiting *mw)
 		ofono_sim_read(mw->sim, SIM_EF_CPHS_MWIS_FILEID,
 				OFONO_SIM_FILE_STRUCTURE_TRANSPARENT,
 				mw_cphs_mwis_read_cb, mw);
+
+		ofono_sim_add_file_watch(mw->sim, SIM_EFMWIS_FILEID,
+						OFONO_SIM_STATE_READY,
+						NULL, mw);
+		ofono_sim_add_file_watch(mw->sim, SIM_EF_CPHS_MWIS_FILEID,
+						OFONO_SIM_STATE_READY,
+						NULL, mw);
+		ofono_sim_add_file_watch(mw->sim, SIM_EFMBI_FILEID,
+						OFONO_SIM_STATE_READY,
+						NULL, mw);
+		ofono_sim_add_file_watch(mw->sim, SIM_EFMBDN_FILEID,
+						OFONO_SIM_STATE_READY,
+						NULL, mw);
+		ofono_sim_add_file_watch(mw->sim, SIM_EF_CPHS_MBDN_FILEID,
+						OFONO_SIM_STATE_READY,
+						NULL, mw);
 	}
 
 	__ofono_atom_register(mw->atom, message_waiting_unregister);
diff --git a/src/network.c b/src/network.c
index eb0f797..ba3e962 100644
--- a/src/network.c
+++ b/src/network.c
@@ -1708,6 +1708,17 @@ static void netreg_unregister(struct ofono_atom *atom)
 		netreg->settings = NULL;
 	}
 
+	if (netreg->sim) {
+		ofono_sim_remove_file_watch(netreg->sim, SIM_EFPNN_FILEID,
+						NULL, netreg);
+		ofono_sim_remove_file_watch(netreg->sim, SIM_EFSPN_FILEID,
+						NULL, netreg);
+		ofono_sim_remove_file_watch(netreg->sim, SIM_EFSPDI_FILEID,
+						NULL, netreg);
+		ofono_sim_remove_file_watch(netreg->sim, SIM_EFOPL_FILEID,
+						NULL, netreg);
+	}
+
 	g_dbus_unregister_interface(conn, path,
 					OFONO_NETWORK_REGISTRATION_INTERFACE);
 	ofono_modem_remove_interface(modem,
@@ -1845,6 +1856,19 @@ void ofono_netreg_register(struct ofono_netreg *netreg)
 		ofono_sim_read(netreg->sim, SIM_EFSPN_FILEID,
 				OFONO_SIM_FILE_STRUCTURE_TRANSPARENT,
 				sim_spn_read_cb, netreg);
+
+		ofono_sim_add_file_watch(netreg->sim, SIM_EFPNN_FILEID,
+						OFONO_SIM_STATE_READY,
+						NULL, netreg);
+		ofono_sim_add_file_watch(netreg->sim, SIM_EFSPN_FILEID,
+						OFONO_SIM_STATE_READY,
+						NULL, netreg);
+		ofono_sim_add_file_watch(netreg->sim, SIM_EFSPDI_FILEID,
+						OFONO_SIM_STATE_READY,
+						NULL, netreg);
+		ofono_sim_add_file_watch(netreg->sim, SIM_EFOPL_FILEID,
+						OFONO_SIM_STATE_READY,
+						NULL, netreg);
 	}
 
 	__ofono_atom_register(netreg->atom, netreg_unregister);
diff --git a/src/sim.c b/src/sim.c
index 7923302..89f7d58 100644
--- a/src/sim.c
+++ b/src/sim.c
@@ -890,6 +890,8 @@ static void sim_get_image(struct ofono_sim *sim, unsigned char id,
 	/* read the image data */
 	ofono_sim_read_bytes(sim, iidf_id, iidf_offset, iidf_len,
 				sim_iidf_read_cb, sim);
+	ofono_sim_add_file_watch(sim, iidf_id,
+					OFONO_SIM_STATE_READY, NULL, sim);
 }
 
 static DBusMessage *sim_get_icon(DBusConnection *conn,
@@ -1127,11 +1129,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);
 
 		ofono_dbus_signal_dict_property_changed(conn, path,
@@ -1143,12 +1146,34 @@ 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, 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, SIM_EFMSISDN_FILEID, OFONO_SIM_FILE_STRUCTURE_FIXED,
 			sim_msisdn_read_cb, 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)
@@ -1193,16 +1218,44 @@ 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) {
+		ofono_sim_remove_file_watch(sim, SIM_EFMSISDN_FILEID,
+						sim_own_numbers_changed, sim);
+		ofono_sim_remove_file_watch(sim, SIM_EFSDN_FILEID,
+						sim_service_numbers_changed,
+						sim);
+		ofono_sim_remove_file_watch(sim, 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, iidf_id, NULL, sim);
+		}
 
-	if (new_state != OFONO_SIM_STATE_READY)
 		return;
+	}
 
 	sim_own_numbers_update(sim);
+	ofono_sim_add_file_watch(sim, SIM_EFMSISDN_FILEID,
+					OFONO_SIM_STATE_READY,
+					sim_own_numbers_changed, sim);
 
 	ofono_sim_read(sim, SIM_EFSDN_FILEID, OFONO_SIM_FILE_STRUCTURE_FIXED,
 			sim_sdn_read_cb, sim);
+	ofono_sim_add_file_watch(sim, SIM_EFSDN_FILEID,
+					OFONO_SIM_STATE_READY,
+					sim_service_numbers_changed, sim);
+
 	ofono_sim_read(sim, SIM_EFIMG_FILEID, OFONO_SIM_FILE_STRUCTURE_FIXED,
 			sim_efimg_read_cb, sim);
+	ofono_sim_add_file_watch(sim, SIM_EFIMG_FILEID,
+					OFONO_SIM_STATE_READY, NULL, sim);
 }
 
 static void sim_imsi_cb(const struct ofono_error *error, const char *imsi,
@@ -1466,6 +1519,9 @@ static void sim_efust_read_cb(int ok, int length, int record,
 		ofono_sim_read(sim, SIM_EFEST_FILEID,
 				OFONO_SIM_FILE_STRUCTURE_TRANSPARENT,
 				sim_efest_read_cb, sim);
+		ofono_sim_add_file_watch(sim, SIM_EFEST_FILEID,
+						OFONO_SIM_STATE_INSERTED,
+						NULL, sim);
 
 		return;
 	}
@@ -1527,6 +1583,9 @@ static void sim_efphase_read_cb(int ok, int length, int record,
 		ofono_sim_read(sim, SIM_EFUST_FILEID,
 				OFONO_SIM_FILE_STRUCTURE_TRANSPARENT,
 				sim_efust_read_cb, sim);
+		ofono_sim_add_file_watch(sim, SIM_EFUST_FILEID,
+						OFONO_SIM_STATE_INSERTED,
+						NULL, sim);
 
 		return;
 	}
@@ -1549,6 +1608,8 @@ static void sim_efphase_read_cb(int ok, int length, int record,
 	ofono_sim_read(sim, SIM_EFSST_FILEID,
 			OFONO_SIM_FILE_STRUCTURE_TRANSPARENT,
 			sim_efsst_read_cb, sim);
+	ofono_sim_add_file_watch(sim, SIM_EFSST_FILEID,
+					OFONO_SIM_STATE_INSERTED, NULL, sim);
 }
 
 static void sim_initialize_after_pin(struct ofono_sim *sim)
@@ -1556,10 +1617,14 @@ static void sim_initialize_after_pin(struct ofono_sim *sim)
 	ofono_sim_read(sim, SIM_EFPHASE_FILEID,
 			OFONO_SIM_FILE_STRUCTURE_TRANSPARENT,
 			sim_efphase_read_cb, sim);
+	ofono_sim_add_file_watch(sim, SIM_EFPHASE_FILEID,
+					OFONO_SIM_STATE_INSERTED, NULL, sim);
 
 	ofono_sim_read(sim, SIM_EFAD_FILEID,
 			OFONO_SIM_FILE_STRUCTURE_TRANSPARENT,
 			sim_ad_read_cb, sim);
+	ofono_sim_add_file_watch(sim, SIM_EFAD_FILEID,
+					OFONO_SIM_STATE_INSERTED, NULL, sim);
 
 	/*
 	 * Read CPHS-support bits, this is still part of the SIM
@@ -1568,6 +1633,8 @@ static void sim_initialize_after_pin(struct ofono_sim *sim)
 	ofono_sim_read(sim, SIM_EF_CPHS_INFORMATION_FILEID,
 			OFONO_SIM_FILE_STRUCTURE_TRANSPARENT,
 			sim_cphs_information_read_cb, sim);
+	ofono_sim_add_file_watch(sim, SIM_EF_CPHS_INFORMATION_FILEID,
+					OFONO_SIM_STATE_INSERTED, NULL, sim);
 }
 
 static void sim_pin_query_cb(const struct ofono_error *error,
@@ -1858,6 +1925,8 @@ static void sim_initialize(struct ofono_sim *sim)
 	ofono_sim_read(sim, SIM_EF_ICCID_FILEID,
 			OFONO_SIM_FILE_STRUCTURE_TRANSPARENT,
 			sim_iccid_read_cb, sim);
+	ofono_sim_add_file_watch(sim, SIM_EF_ICCID_FILEID,
+					OFONO_SIM_STATE_INSERTED, NULL, sim);
 
 	/* EFecc is read by the voicecall atom */
 
@@ -1872,9 +1941,14 @@ static void sim_initialize(struct ofono_sim *sim)
 	ofono_sim_read(sim, SIM_EFLI_FILEID,
 			OFONO_SIM_FILE_STRUCTURE_TRANSPARENT,
 			sim_efli_read_cb, sim);
+	ofono_sim_add_file_watch(sim, SIM_EFLI_FILEID,
+					OFONO_SIM_STATE_INSERTED, NULL, sim);
+
 	ofono_sim_read(sim, SIM_EFPL_FILEID,
 			OFONO_SIM_FILE_STRUCTURE_TRANSPARENT,
 			sim_efpl_read_cb, sim);
+	ofono_sim_add_file_watch(sim, SIM_EFPL_FILEID,
+					OFONO_SIM_STATE_INSERTED, NULL, sim);
 }
 
 int ofono_sim_read_bytes(struct ofono_sim *sim, int id,
@@ -1995,6 +2069,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) {
@@ -2039,6 +2114,17 @@ static void sim_free_state(struct ofono_sim *sim)
 
 	sim->fixed_dialing = FALSE;
 	sim->barred_dialing = FALSE;
+
+	ofono_sim_remove_file_watch(sim, SIM_EFUST_FILEID, NULL, sim);
+	ofono_sim_remove_file_watch(sim, SIM_EFEST_FILEID, NULL, sim);
+	ofono_sim_remove_file_watch(sim, SIM_EFSST_FILEID, NULL, sim);
+	ofono_sim_remove_file_watch(sim, SIM_EFPHASE_FILEID, NULL, sim);
+	ofono_sim_remove_file_watch(sim, SIM_EFAD_FILEID, NULL, sim);
+	ofono_sim_remove_file_watch(sim, SIM_EF_CPHS_INFORMATION_FILEID,
+					NULL, sim);
+	ofono_sim_remove_file_watch(sim, SIM_EF_ICCID_FILEID, NULL, sim);
+	ofono_sim_remove_file_watch(sim, SIM_EFLI_FILEID, NULL, sim);
+	ofono_sim_remove_file_watch(sim, SIM_EFPL_FILEID, NULL, sim);
 }
 
 void ofono_sim_inserted_notify(struct ofono_sim *sim, ofono_bool_t inserted)
diff --git a/src/voicecall.c b/src/voicecall.c
index 97fc36b..5df3314 100644
--- a/src/voicecall.c
+++ b/src/voicecall.c
@@ -2246,19 +2246,30 @@ 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, SIM_EFECC_FILEID,
+			OFONO_SIM_FILE_STRUCTURE_TRANSPARENT,
+			ecc_g2_read_cb, vc);
+	ofono_sim_read(vc->sim, 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;
 
 	switch (new_state) {
 	case OFONO_SIM_STATE_INSERTED:
-		/* Try both formats, only one or none will work */
-		ofono_sim_read(vc->sim, SIM_EFECC_FILEID,
-				OFONO_SIM_FILE_STRUCTURE_TRANSPARENT,
-				ecc_g2_read_cb, vc);
-		ofono_sim_read(vc->sim, 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, SIM_EFECC_FILEID,
+						OFONO_SIM_STATE_INSERTED,
+						read_ecc_numbers, vc);
 		break;
 	case OFONO_SIM_STATE_NOT_PRESENT:
 		/* TODO: Must release all non-emergency calls */
@@ -2275,6 +2286,9 @@ static void sim_state_watch(enum ofono_sim_state new_state, void *user)
 
 		add_to_en_list(&vc->new_en_list, default_en_list_no_sim);
 		set_new_ecc(vc);
+
+		ofono_sim_remove_file_watch(vc->sim, SIM_EFECC_FILEID,
+						read_ecc_numbers, vc);
 	default:
 		break;
 	}
-- 
1.7.1.86.g0e460.dirty

[PATCH 5/5] 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 932e49a..ca01cf3 100644
--- a/src/stk.c
+++ b/src/stk.c
@@ -1951,10 +1951,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("");
@@ -2008,6 +2020,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 1/5] sim: Cache flushing functions.

[Denis Kenzior]

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

Hi Andrew,

On 01/10/2011 05:01 AM, Andrzej Zaborowski wrote:
> ---
>  src/sim.c   |   23 ++++++++++++++++++++
>  src/simfs.c |   68 ++++++++++++++++++++++++++++++++++++++++++++--------------
>  src/simfs.h |    5 ++++
>  3 files changed, 79 insertions(+), 17 deletions(-)
> 

So I applied this patch but...:

> +
> +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);
> +}

This function seems extraneous to this patch.  I assumed that it was
meant to be included in patch 4 and I left it out.

Regards,
-Denis

Re: [PATCH 3/5] sim: Implement file watching and basic refresh.

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 4558 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, *filel;
> +	enum sim_reset_state {
> +		RESET_STATE_NOT_PRESENT	= OFONO_SIM_STATE_NOT_PRESENT,
> +		RESET_STATE_INSERTED	= OFONO_SIM_STATE_INSERTED,
> +		RESET_STATE_READY	= OFONO_SIM_STATE_READY,
> +		RESET_STATE_NONE,
> +	} reset_state = RESET_STATE_NONE;
> +
> +	/* Flush cached content for affected files */
> +	if (full_file_change)
> +		sim_fs_cache_flush(sim->simfs);
> +	else
> +		for (filel = file_list; filel; filel = filel->next) {
> +			struct stk_file *file = filel->data;
> +			int id = (file->file[file->len - 2] << 8) |
> +				(file->file[file->len - 1] << 0);
> +
> +			sim_file_changed_flush(sim, id);
> +		}

Just a nitpick, but when you have such compound statements inside
if/while/for, could you enclose them in parentheses?  I think that makes
the code a bit more readable.

e.g.
if (foo)
else {
	for () {

	}
}

> +
> +	if (naa_init)
> +		reset_state = RESET_STATE_INSERTED;
> +
> +	/*
> +	 * 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 (filel = file_list; filel; filel = filel->next) {
> +			struct stk_file *file = filel->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 (filel = file_list; filel; filel = filel->next) {
> +			struct stk_file *file = filel->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_STATE_NOT_PRESENT:
> +		ofono_sim_inserted_notify(sim, FALSE);
> +		ofono_sim_inserted_notify(sim, TRUE);

Are you sure we can do it this way?  I would assume that the modem might
require some time to perform its own SIM initialization and would have
to signal the sim_inserted itself...

> +		break;
> +	case RESET_STATE_INSERTED:
> +		sim_free_state(sim);
> +		sim->state = OFONO_SIM_STATE_INSERTED;
> +		sim_initialize(sim);

What about removing all the post_sim / online atoms?  In theory
something like EFfdn could have been changed and now we're stuck in
fixed dialing mode.  We should not keep the atoms around in this case.

> +		break;
> +	case RESET_STATE_READY:
> +		sim->state = OFONO_SIM_STATE_INSERTED;
> +		for (l = sim->state_watches->items; l; l = l->next) {
> +			struct ofono_watchlist_item *item = l->data;
> +			ofono_sim_state_event_cb_t notify = item->notify;
> +
> +			notify(sim->state, item->notify_data);

I'm having trouble understanding the need for this for loop.  Isn't the
SIM already inserted?

> +		}
> +
> +		sim_set_ready(sim);
> +		break;
> +	case RESET_STATE_NONE:
> +		break;
> +	}
> +}

Overall it seems like there are two categories of files that we should
worry about:

- Those handled inside sim.c that affect sim initialization:
	- EFust, EFest, EFsst
	- EFbdn, EFfdn
	- EFphase, EFcphs_info (these ones are really unlikely outside of a
full sim reset)
	- EFad (This one is unlikely as it would affect the IMSI)
- The ones that don't

To me it sounds like we should just special case the ones affecting sim
initialization inside this function and use a simplified API for the rest.

Regards,
-Denis

Re: [PATCH 3/5] sim: Implement file watching and basic refresh.

[Andrzej Zaborowski]

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

Hi Denis,

I took me a while to get back to this, but we need to work it out...

On 12 January 2011 22:55, 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, *filel;
>> +     enum sim_reset_state {
>> +             RESET_STATE_NOT_PRESENT = OFONO_SIM_STATE_NOT_PRESENT,
>> +             RESET_STATE_INSERTED    = OFONO_SIM_STATE_INSERTED,
>> +             RESET_STATE_READY       = OFONO_SIM_STATE_READY,
>> +             RESET_STATE_NONE,
>> +     } reset_state = RESET_STATE_NONE;
>> +
>> +     /* Flush cached content for affected files */
>> +     if (full_file_change)
>> +             sim_fs_cache_flush(sim->simfs);
>> +     else
>> +             for (filel = file_list; filel; filel = filel->next) {
>> +                     struct stk_file *file = filel->data;
>> +                     int id = (file->file[file->len - 2] << 8) |
>> +                             (file->file[file->len - 1] << 0);
>> +
>> +                     sim_file_changed_flush(sim, id);
>> +             }
>
> Just a nitpick, but when you have such compound statements inside
> if/while/for, could you enclose them in parentheses?  I think that makes
> the code a bit more readable.
>
> e.g.
> if (foo)
> else {
>        for () {
>
>        }
> }

Ok, good point.

>
>> +
>> +     if (naa_init)
>> +             reset_state = RESET_STATE_INSERTED;
>> +
>> +     /*
>> +      * 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 (filel = file_list; filel; filel = filel->next) {
>> +                     struct stk_file *file = filel->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 (filel = file_list; filel; filel = filel->next) {
>> +                     struct stk_file *file = filel->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_STATE_NOT_PRESENT:
>> +             ofono_sim_inserted_notify(sim, FALSE);
>> +             ofono_sim_inserted_notify(sim, TRUE);
>
> Are you sure we can do it this way?  I would assume that the modem might
> require some time to perform its own SIM initialization and would have
> to signal the sim_inserted itself...

I assume that the modem would send the refresh command only once it
has done all its internal housekeeping and is ready for us to re-read
the changed files.  If not then probably the AT interface will block
for as much time as needed?

>
>> +             break;
>> +     case RESET_STATE_INSERTED:
>> +             sim_free_state(sim);
>> +             sim->state = OFONO_SIM_STATE_INSERTED;
>> +             sim_initialize(sim);
>
> What about removing all the post_sim / online atoms?  In theory
> something like EFfdn could have been changed and now we're stuck in
> fixed dialing mode.  We should not keep the atoms around in this case.

So in the next patch I add a "watch" for every file in the SIM that we
care about.  Every watch has a sort of "reset level", which tells
sim.c how much of our state needs to be reinitialised, so that we
don't reset the atoms if that is not necessary.  For example for EFust
/ EFest we set the level to RESET_STATE_INSERTED.

This is the case for FDN, because will be re-read EFust/EFest as a
result of sim_initialize() and later at some point we will signal
OFONO_SIM_STATE_READY so all the concerned atoms will take note.  It's
not necessary to remove the atoms in this case.

>
>> +             break;
>> +     case RESET_STATE_READY:
>> +             sim->state = OFONO_SIM_STATE_INSERTED;
>> +             for (l = sim->state_watches->items; l; l = l->next) {
>> +                     struct ofono_watchlist_item *item = l->data;
>> +                     ofono_sim_state_event_cb_t notify = item->notify;
>> +
>> +                     notify(sim->state, item->notify_data);
>
> I'm having trouble understanding the need for this for loop.  Isn't the
> SIM already inserted?

I initially didn't have this loop, but I thought it might be better to
signal "INSERTED" just before "READY", so that atoms see a *change* if
they though that the sim was READY before.  But looking at the current
users of the sim state watch, it makes no difference.

>
>> +             }
>> +
>> +             sim_set_ready(sim);
>> +             break;
>> +     case RESET_STATE_NONE:
>> +             break;
>> +     }
>> +}
>
> Overall it seems like there are two categories of files that we should
> worry about:
>
> - Those handled inside sim.c that affect sim initialization:
>        - EFust, EFest, EFsst
>        - EFbdn, EFfdn
>        - EFphase, EFcphs_info (these ones are really unlikely outside of a
> full sim reset)
>        - EFad (This one is unlikely as it would affect the IMSI)
> - The ones that don't
>
> To me it sounds like we should just special case the ones affecting sim
> initialization inside this function and use a simplified API for the rest.

So I think this wouldn't make the api much better because:

* These files you listed are the majority of all files we read, so the
"normal" users are in minority.

* The only simplification I see would be removal of one parameter in
add_file_watch_call

int ofono_sim_add_file_watch(struct ofono_sim *sim, int id,
                       enum ofono_sim_state reset_state,
                       ofono_sim_file_notify_t notify, void *userdata);

or are you thinking about a different simplifaction?

Best regards

Re: [PATCH 3/5] sim: Implement file watching and basic refresh.

[Denis Kenzior]

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

Hi Andrew,

>>> +     switch (reset_state) {
>>> +     case RESET_STATE_NOT_PRESENT:
>>> +             ofono_sim_inserted_notify(sim, FALSE);
>>> +             ofono_sim_inserted_notify(sim, TRUE);
>>
>> Are you sure we can do it this way?  I would assume that the modem might
>> require some time to perform its own SIM initialization and would have
>> to signal the sim_inserted itself...
> 
> I assume that the modem would send the refresh command only once it
> has done all its internal housekeeping and is ready for us to re-read
> the changed files.  If not then probably the AT interface will block
> for as much time as needed?
> 

So I think we have to double check this part.  There are definitely STK
implementations which send the Refresh and then look at the terminal
response to see whether the refresh should succeed or not.

>>
>>> +             break;
>>> +     case RESET_STATE_INSERTED:
>>> +             sim_free_state(sim);
>>> +             sim->state = OFONO_SIM_STATE_INSERTED;
>>> +             sim_initialize(sim);
>>
>> What about removing all the post_sim / online atoms?  In theory
>> something like EFfdn could have been changed and now we're stuck in
>> fixed dialing mode.  We should not keep the atoms around in this case.
> 
> So in the next patch I add a "watch" for every file in the SIM that we
> care about.  Every watch has a sort of "reset level", which tells
> sim.c how much of our state needs to be reinitialised, so that we
> don't reset the atoms if that is not necessary.  For example for EFust
> / EFest we set the level to RESET_STATE_INSERTED.
> 
> This is the case for FDN, because will be re-read EFust/EFest as a
> result of sim_initialize() and later at some point we will signal
> OFONO_SIM_STATE_READY so all the concerned atoms will take note.  It's
> not necessary to remove the atoms in this case.
> 

Still not sure this works.  Lets say you have a SIM that can change FDN
mode based on STK refresh.  If it sends us such a command and FDN is now
enabled, we should remove all atoms except pre-sim, as they are no
longer relevant.  If we start oFono up and FDN is enabled, the atoms
won't be present, why is the case via refresh different?

>>
>>> +             break;
>>> +     case RESET_STATE_READY:
>>> +             sim->state = OFONO_SIM_STATE_INSERTED;
>>> +             for (l = sim->state_watches->items; l; l = l->next) {
>>> +                     struct ofono_watchlist_item *item = l->data;
>>> +                     ofono_sim_state_event_cb_t notify = item->notify;
>>> +
>>> +                     notify(sim->state, item->notify_data);
>>
>> I'm having trouble understanding the need for this for loop.  Isn't the
>> SIM already inserted?
> 
> I initially didn't have this loop, but I thought it might be better to
> signal "INSERTED" just before "READY", so that atoms see a *change* if
> they though that the sim was READY before.  But looking at the current
> users of the sim state watch, it makes no difference.
> 

Then lets drop it.

>> Overall it seems like there are two categories of files that we should
>> worry about:
>>
>> - Those handled inside sim.c that affect sim initialization:
>>        - EFust, EFest, EFsst
>>        - EFbdn, EFfdn
>>        - EFphase, EFcphs_info (these ones are really unlikely outside of a
>> full sim reset)
>>        - EFad (This one is unlikely as it would affect the IMSI)
>> - The ones that don't
>>
>> To me it sounds like we should just special case the ones affecting sim
>> initialization inside this function and use a simplified API for the rest.
> 
> So I think this wouldn't make the api much better because:
> 
> * These files you listed are the majority of all files we read, so the
> "normal" users are in minority.

I'm not so sure, I counted at least 15 instances of 'normal' files.

> 
> * The only simplification I see would be removal of one parameter in
> add_file_watch_call
> 
> int ofono_sim_add_file_watch(struct ofono_sim *sim, int id,
>                        enum ofono_sim_state reset_state,
>                        ofono_sim_file_notify_t notify, void *userdata);
> 
> or are you thinking about a different simplifaction?

Yes, I would yang the reset_state parameter.  That one is really
confusing and not needed outside the sim atom anyway.  So my proposal is
to make this as complicated as you need inside sim atom, but expose a
specialized API to the rest of the system.

Regards,
-Denis

Re: [PATCH 3/5] sim: Implement file watching and basic refresh.

[Andrzej Zaborowski]

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

On 31 January 2011 18:45, Denis Kenzior <denkenz@gmail.com> wrote:
>>>> +     switch (reset_state) {
>>>> +     case RESET_STATE_NOT_PRESENT:
>>>> +             ofono_sim_inserted_notify(sim, FALSE);
>>>> +             ofono_sim_inserted_notify(sim, TRUE);
>>>
>>> Are you sure we can do it this way?  I would assume that the modem might
>>> require some time to perform its own SIM initialization and would have
>>> to signal the sim_inserted itself...
>>
>> I assume that the modem would send the refresh command only once it
>> has done all its internal housekeeping and is ready for us to re-read
>> the changed files.  If not then probably the AT interface will block
>> for as much time as needed?
>>
>
> So I think we have to double check this part.  There are definitely STK
> implementations which send the Refresh and then look at the terminal
> response to see whether the refresh should succeed or not.

Yes, it would be good to confirm, at this point I have no data.
However I can't see a good way to find out when the SIM is ready to
accept new commands (except sending the commands away and waiting
until we get called back).

>
>>>
>>>> +             break;
>>>> +     case RESET_STATE_INSERTED:
>>>> +             sim_free_state(sim);
>>>> +             sim->state = OFONO_SIM_STATE_INSERTED;
>>>> +             sim_initialize(sim);
>>>
>>> What about removing all the post_sim / online atoms?  In theory
>>> something like EFfdn could have been changed and now we're stuck in
>>> fixed dialing mode.  We should not keep the atoms around in this case.
>>
>> So in the next patch I add a "watch" for every file in the SIM that we
>> care about.  Every watch has a sort of "reset level", which tells
>> sim.c how much of our state needs to be reinitialised, so that we
>> don't reset the atoms if that is not necessary.  For example for EFust
>> / EFest we set the level to RESET_STATE_INSERTED.
>>
>> This is the case for FDN, because will be re-read EFust/EFest as a
>> result of sim_initialize() and later at some point we will signal
>> OFONO_SIM_STATE_READY so all the concerned atoms will take note.  It's
>> not necessary to remove the atoms in this case.
>>
>
> Still not sure this works.  Lets say you have a SIM that can change FDN
> mode based on STK refresh.  If it sends us such a command and FDN is now
> enabled, we should remove all atoms except pre-sim, as they are no
> longer relevant.  If we start oFono up and FDN is enabled, the atoms
> won't be present, why is the case via refresh different?

You're right, my logic has failed somewhere.. I see that the
"RESET_STATE_INSERTED" state is actually unneeded, it should just be
same as NOT_PRESENT and drop all post-sim atoms and then potentially
re-create.

>
>>>
>>>> +             break;
>>>> +     case RESET_STATE_READY:
>>>> +             sim->state = OFONO_SIM_STATE_INSERTED;
>>>> +             for (l = sim->state_watches->items; l; l = l->next) {
>>>> +                     struct ofono_watchlist_item *item = l->data;
>>>> +                     ofono_sim_state_event_cb_t notify = item->notify;
>>>> +
>>>> +                     notify(sim->state, item->notify_data);
>>>
>>> I'm having trouble understanding the need for this for loop.  Isn't the
>>> SIM already inserted?
>>
>> I initially didn't have this loop, but I thought it might be better to
>> signal "INSERTED" just before "READY", so that atoms see a *change* if
>> they though that the sim was READY before.  But looking at the current
>> users of the sim state watch, it makes no difference.
>>
>
> Then lets drop it.
>
>>> Overall it seems like there are two categories of files that we should
>>> worry about:
>>>
>>> - Those handled inside sim.c that affect sim initialization:
>>>        - EFust, EFest, EFsst
>>>        - EFbdn, EFfdn
>>>        - EFphase, EFcphs_info (these ones are really unlikely outside of a
>>> full sim reset)
>>>        - EFad (This one is unlikely as it would affect the IMSI)
>>> - The ones that don't
>>>
>>> To me it sounds like we should just special case the ones affecting sim
>>> initialization inside this function and use a simplified API for the rest.
>>
>> So I think this wouldn't make the api much better because:
>>
>> * These files you listed are the majority of all files we read, so the
>> "normal" users are in minority.
>
> I'm not so sure, I counted at least 15 instances of 'normal' files.
>
>>
>> * The only simplification I see would be removal of one parameter in
>> add_file_watch_call
>>
>> int ofono_sim_add_file_watch(struct ofono_sim *sim, int id,
>>                        enum ofono_sim_state reset_state,
>>                        ofono_sim_file_notify_t notify, void *userdata);
>>
>> or are you thinking about a different simplifaction?
>
> Yes, I would yang the reset_state parameter.  That one is really
> confusing and not needed outside the sim atom anyway.  So my proposal is
> to make this as complicated as you need inside sim atom, but expose a
> specialized API to the rest of the system.

Ok, sounds good.

Best regards