Re: [RFC PATCH 2/4] modem: add EmergencyMode property

[Denis Kenzior <denkenz@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 4185 bytes --]

Hi Andras,

<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;
> +
> +	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 ;)

> +
> +	modem->driver->set_online(modem, TRUE, emergency_online_cb, modem);
> +}
> +

<snip>

> diff --git a/src/ofono.h b/src/ofono.h
> index d1a4bdc..11d939d 100644
> --- a/src/ofono.h
> +++ b/src/ofono.h
> @@ -58,6 +58,7 @@ DBusMessage *__ofono_error_not_attached(DBusMessage *msg);
>  DBusMessage *__ofono_error_attach_in_progress(DBusMessage *msg);
>  DBusMessage *__ofono_error_canceled(DBusMessage *msg);
>  DBusMessage *__ofono_error_access_denied(DBusMessage *msg);
> +DBusMessage *__ofono_error_emergency_active(DBusMessage *msg);

Can you put this into a separate patch?

>  
>  void __ofono_dbus_pending_reply(DBusMessage **msg, DBusMessage *reply);
>  
> @@ -185,6 +186,10 @@ unsigned int __ofono_modem_add_online_watch(struct ofono_modem *modem,
>  void __ofono_modem_remove_online_watch(struct ofono_modem *modem,
>  					unsigned int id);
>  
> +ofono_bool_t ofono_modem_get_emergency(struct ofono_modem *modem);
> +void ofono_modem_inc_emergency(struct ofono_modem *modem);
> +void ofono_modem_dec_emergency(struct ofono_modem *modem);
> +
>  #include <ofono/call-barring.h>
>  
>  gboolean __ofono_call_barring_is_busy(struct ofono_call_barring *cb);

And please resubmit the entire series, seeing only patch 2/4 and patch
4/4 is quite confusing.

Regards,
-Denis