Hi Denis,
On 7 February 2011 20:34, Denis Kenzior <[email protected]> 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
_______________________________________________
ofono mailing list
[email protected]
http://lists.ofono.org/listinfo/ofono
