Hi Denis and Andras,
2010/12/3 Denis Kenzior <denk...@gmail.com>:
> <snip>
>> +static void modem_change_online(struct ofono_modem *modem,
>> + enum modem_state new_state)
>> +{
>> + DBusConnection *conn = ofono_dbus_get_connection();
>> + enum modem_state old_state = modem->modem_state;
>> + ofono_bool_t new_online = new_state == MODEM_STATE_ONLINE;
I'd define a new enum modem_state for emergency mode, MODEM_STATE_EMERGENCY.
>> + DBG("old state: %d, new state: %d", old_state, new_state);
>> +
>> + if (new_online == modem->online)
>> + return;
>
> You check for online not being equal here
>
>> +
>> + modem->modem_state = new_state;
>> + modem->online = new_online;
>> +
>> + ofono_dbus_signal_property_changed(conn, modem->path,
>> + OFONO_MODEM_INTERFACE, "Online",
>> + DBUS_TYPE_BOOLEAN, &modem->online);
>> +
>> + notify_online_watches(modem);
>> +}
>> +
>> +static void emergency_online_cb(const struct ofono_error *error, void *data)
>> +{
>> + struct ofono_modem *modem = data;
>> +
>> + DBG("modem: %p", modem);
>> +
>> + if (error->type == OFONO_ERROR_TYPE_NO_ERROR &&
>> + modem->modem_state != MODEM_STATE_ONLINE)
>
> And perform essentially the same check here... Seems wasteful
>
>> + modem_change_online(modem, MODEM_STATE_ONLINE);
>> +}
>> +
>> +static void emergency_offline_cb(const struct ofono_error *error, void
>> *data)
>> +{
>> + struct ofono_modem *modem = data;
>> + DBusConnection *conn = ofono_dbus_get_connection();
>> + const char *path = ofono_modem_get_path(modem);
>> + gboolean state = FALSE;
>> +
>> + DBG("modem: %p", modem);
>> +
>> + if (error->type == OFONO_ERROR_TYPE_NO_ERROR &&
>> + modem->modem_state == MODEM_STATE_ONLINE)
>> + modem_change_online(modem, modem->modem_state_pre_emergency);
>
> Same comment as above
>
>> +
>> + modem->emergency--;
>> +
>> + ofono_dbus_signal_property_changed(conn, path,
>> + OFONO_MODEM_INTERFACE,
>> + "Emergency",
>> + DBUS_TYPE_BOOLEAN,
>> + &state);
>> +}
>> +
>> +void ofono_modem_inc_emergency(struct ofono_modem *modem)
>> +{
>> + DBusConnection *conn = ofono_dbus_get_connection();
>> + const char *path = ofono_modem_get_path(modem);
>> + gboolean state = TRUE;
>> +
>> + DBG("modem: %p", modem);
>> +
>> + modem->emergency++;
>> + if (modem->emergency > 1)
>> + return;
>> +
>> + ofono_dbus_signal_property_changed(conn, path,
>> + OFONO_MODEM_INTERFACE,
>> + "Emergency",
>> + DBUS_TYPE_BOOLEAN,
>> + &state);
>> +
>> + modem->modem_state_pre_emergency = modem->modem_state;
>> +
>> + if (modem->modem_state == MODEM_STATE_ONLINE)
>> + return;
>
> So my only major concern left is here actually. If we activate an
> emergency call and a set_property(Online, blah) is active, we get into
> some funny race conditions. I mentioned these to Pekka on IRC. But
> basically the worst one is if we have an Online=False operation pending.
> In this case your call proceeds, but then gets terminated shortly
> thereafter by the offline procedure ;)
That is a tricky problem. I'd say the most straightforward way is to
return an error if there is a pending operation and push these
problems up the stack. In any case dialer has to retry ecall if user
tries to make the emergency call just after or before moving device to
offline / removing SIM card / etc. However, I'm not sure if we can
propagate the error in all cases where ofono_modem_inc_emergency()
gets called.
In order to manage the worst case in best possible manner, I'd check
for set_online in progress here. Return FALSE from
ofono_modem_get_online() immediately after the set_online(FALSE) call
is made. Also, check the emergency state again in after set_online
callback response. The online/offline watches should be called before
the set_online() call. (I have no idea what we will do should
set_online(FALSE) fail)?
--
Pekka.Pessi mail at nokia.com
_______________________________________________
ofono mailing list
ofono@ofono.org
http://lists.ofono.org/listinfo/ofono
