RE: [PATCH] ofono: Bring device attached to modem down and up if IMSI has really changed

[Pasi Sjöholm]

>>   if (g_str_equal(key, "SubscriberIdentity")) {
>> - sim_update_imsi(modem, &value);
>> + dbus_message_iter_get_basic(&value, &new_imsi);
>> +
>> + if (g_strcmp0(modem->imsi,new_imsi) != 0) {
>What is the benefit of checking the existing imsi against the new one?
>Isn't it known already that the sim is tied to this particular modem, as
>the modem is looked using the sending D-Bus path?

>> + sim_update_imsi(modem, &value);
>> +
>> + if (modem->device)
>> + destroy_device(modem);
>> + }
>Shouldn't this then be run unconditionally?

For some reason (didn't dig it further) the sim_changed is called multiple 
times with the SubscriberIdentity as key when hot-swapping the sim card and the 
connman already has the new IMSI so we should not destroy the device in that 
case to get service identier corrected as it is already correct. It can lead to 
connman missing the cellular context if it's done (yes, I tried.. and it seems 
to occur when the sim has a pin set but not when it does not).

>BTW, the sim_* functions are in need of better checking that the
>messages contain the desired properties.

Noticed that also when discussing with Hannu Mallat. :)
___
connman mailing list
connman@connman.net
https://lists.connman.net/mailman/listinfo/connman

Re: [PATCH] ofono: Bring device attached to modem down and up if IMSI has really changed

[Patrik Flykt]

Hi,

On Mon, 2014-09-29 at 11:20 +0300, pasi.sjoh...@jolla.com wrote:
> From: Pasi Sjöholm 
> 
> Bringing the device down when the IMSI is really changed will force the
> service have correct identifier and not to use the old one (eg. when SIM-
> card is changed).
> ---
>  plugins/ofono.c | 10 +-
>  1 file changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/plugins/ofono.c b/plugins/ofono.c
> index 7af551b..0d296d7 100644
> --- a/plugins/ofono.c
> +++ b/plugins/ofono.c
> @@ -1908,6 +1908,7 @@ static gboolean sim_changed(DBusConnection *conn, 
> DBusMessage *message,
>   struct modem_data *modem;
>   DBusMessageIter iter, value;
>   const char *key;
> + char *new_imsi;
>  
>   modem = g_hash_table_lookup(modem_hash, path);
>   if (!modem)
> @@ -1925,7 +1926,14 @@ static gboolean sim_changed(DBusConnection *conn, 
> DBusMessage *message,
>   dbus_message_iter_recurse(&iter, &value);
>  
>   if (g_str_equal(key, "SubscriberIdentity")) {
> - sim_update_imsi(modem, &value);
> + dbus_message_iter_get_basic(&value, &new_imsi);
> +
> + if (g_strcmp0(modem->imsi,new_imsi) != 0) {

What is the benefit of checking the existing imsi against the new one?
Isn't it known already that the sim is tied to this particular modem, as
the modem is looked using the sending D-Bus path?

> + sim_update_imsi(modem, &value); 
> +
> + if (modem->device)
> + destroy_device(modem);
> + }

Shouldn't this then be run unconditionally?

>  
>   if (!ready_to_create_device(modem))
>   return TRUE;

BTW, the sim_* functions are in need of better checking that the
messages contain the desired properties.

Cheers,

Patrik

___
connman mailing list
connman@connman.net
https://lists.connman.net/mailman/listinfo/connman