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

[Denis Kenzior <denkenz@gmail.com>]

[-- 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