Re: [PATCH 1/2] netreg: Add CPHS CSP implementation
Hi Denis,
2011/2/3 Denis Kenzior <denkenz(a)gmail.com>:
> + if (netreg->mode ==
NETWORK_REGISTRATION_MODE_AUTO_ONLY)
> + return NULL;
> +
So we might just return access_denied here as well.
There is a reason for allowing this, namely that sometimes you want to
re-run the logic for automatically operator selection. Granted, this
is not your most common use case, but since it also does no harm to
allowing it, I didn't.
> set_registration_mode(netreg,
NETWORK_REGISTRATION_MODE_AUTO);
>
> return NULL;
<snip>
> @@ -1769,7 +1835,7 @@ static void netreg_load_settings(struct ofono_netreg *netreg)
> mode = g_key_file_get_integer(netreg->settings, SETTINGS_GROUP,
> "Mode", NULL);
>
> - if (mode >= 0 && mode <= 1)
> + if (mode != NETWORK_REGISTRATION_MODE_AUTO_ONLY)
> netreg->mode = mode;
>
This check is unnecessary, you already loaded the settings before you
managed to load the sim file. You're also removing a sanity check for
no real reason.
That's just a bug. The intention was to write:
if (mode == NEtWORK_REGISTRATION_MODE_AUTO ||
mode == NETWORK_REGISTRATION_MODE_MANUAL)
netreg->mode = mode;
The patch has a seemingly shorter way of describing the same, but as
you point out, misses the sanity check. Must have been lack of coffee
or something. ;)
This brings up another point, since the CPHS CSP file is actually
user-writeable (for some unknown reason) the user can remove the SIM,
put it in another phone and modify the forced-auto settings. If you
store the forced-auto mode in the settings file, then there's no way to
ever go back to non-forced auto, even though he should be able to.
I think the above way will in fact default to auto even if the
settings store has auto-only. And yes, EFcsp can definitely change
even at run-time, as manual mode really should be allowed when
roaming, at least in countries that the operator for instance has
multiple suitable roaming partners with varying coverage.
I suggest that when forced-auto is detected, that we only write
NETWORK_REGISTRATION_MODE_AUTO to the settings store.
That sounds reasonable, too.
Cheers,
Aki