[PATCH 0/4] Emergency Calls (4th round)

[PATCH 0/4] Emergency Calls (4th round)

[Andras Domokos]

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

Here is a new proposal for emergency calls handling.

Steps in handling emergency calls:
- subscribe to modem online notifications (add modem online watcher)
- an emergency call detected (phone number is emergency number)
- increment emergency mode
  - advertise "EmergencyMode" property change on D-Bus (first call)
  - set modem online if it's in offline mode (minimal setup)
    - adevertise "Online" property change on D-Bus
- if modem is not online postpone making the call, otherwise make
  the emergency call
- when modem online notification comes and there is postponed call request
  make the emergency call
- when an emergency call ends decrement emergency mode
  - set modem offline if it was set online due to the emergency call (last call)
    - advertise "Online" property change on D-Bus
  - advertise "EmergencyMode" property change on D-Bus (last call)
 
Note: emergency calls with SIM card (network registered or unregistered) and
without SIM card are supported.

Andras Domokos (4):
  modem: add modem online-offline watch
  modem: add EmergencyMode property
  modem: move dial_request_cb function
  voicecall: add emergency call handling

 src/modem.c     |  180 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 src/ofono.h     |   12 ++++
 src/voicecall.c |  175 +++++++++++++++++++++++++++++++++++++++++++----------
 3 files changed, 334 insertions(+), 33 deletions(-)

[RFC PATCH 1/4] modem: add modem online-offline watch

[Andras Domokos]

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

From: Andras Domokos <andras.domokos@nokia.com>

---
 src/modem.c |   46 ++++++++++++++++++++++++++++++++++++++++++++++
 src/ofono.h |    8 ++++++++
 2 files changed, 54 insertions(+), 0 deletions(-)

diff --git a/src/modem.c b/src/modem.c
index 3776461..f73cc1d 100644
--- a/src/modem.c
+++ b/src/modem.c
@@ -72,6 +72,7 @@ struct ofono_modem {
 	ofono_bool_t		powered_pending;
 	guint			timeout;
 	ofono_bool_t		online;
+	struct ofono_watchlist	*online_watches;
 	GHashTable		*properties;
 	struct ofono_sim	*sim;
 	unsigned int		sim_watch;
@@ -362,6 +363,22 @@ static void flush_atoms(struct ofono_modem *modem, enum modem_state new_state)
 	}
 }
 
+static void notify_online_watches(struct ofono_modem *modem)
+{
+	struct ofono_watchlist_item *item;
+	GSList *l;
+	ofono_modem_online_notify_func notify;
+
+	if (modem->online_watches == NULL)
+		return;
+
+	for (l = modem->online_watches->items; l; l = l->next) {
+		item = l->data;
+		notify = item->notify;
+		notify(modem->online, item->notify_data);
+	}
+}
+
 static void modem_change_state(struct ofono_modem *modem,
 				enum modem_state new_state)
 {
@@ -404,11 +421,13 @@ static void modem_change_state(struct ofono_modem *modem,
 			__ofono_history_probe_drivers(modem);
 			__ofono_nettime_probe_drivers(modem);
 		}
+		notify_online_watches(modem);
 		break;
 
 	case MODEM_STATE_ONLINE:
 		if (driver->post_online)
 			driver->post_online(modem);
+		notify_online_watches(modem);
 		break;
 	}
 }
@@ -437,6 +456,29 @@ static void sim_state_watch(enum ofono_sim_state new_state, void *user)
 	}
 }
 
+unsigned __ofono_modem_add_online_watch(struct ofono_modem *modem,
+		ofono_modem_online_notify_func notify,
+		void *data, ofono_destroy_func destroy)
+{
+	struct ofono_watchlist_item *item;
+
+	if (modem == NULL || notify == NULL)
+		return 0;
+
+	item = g_new0(struct ofono_watchlist_item, 1);
+
+	item->notify = notify;
+	item->destroy = destroy;
+	item->notify_data = data;
+
+	return __ofono_watchlist_add_item(modem->online_watches, item);
+}
+
+void __ofono_modem_remove_online_watch(struct ofono_modem *modem, unsigned id)
+{
+	__ofono_watchlist_remove_item(modem->online_watches, id);
+}
+
 static void online_cb(const struct ofono_error *error, void *data)
 {
 	struct ofono_modem *modem = data;
@@ -1472,6 +1514,7 @@ int ofono_modem_register(struct ofono_modem *modem)
 	modem->driver_type = NULL;
 
 	modem->atom_watches = __ofono_watchlist_new(g_free);
+	modem->online_watches = __ofono_watchlist_new(g_free);
 
 	emit_modem_added(modem);
 	call_modemwatches(modem, TRUE);
@@ -1503,6 +1546,9 @@ static void modem_unregister(struct ofono_modem *modem)
 	__ofono_watchlist_free(modem->atom_watches);
 	modem->atom_watches = NULL;
 
+	__ofono_watchlist_free(modem->online_watches);
+	modem->online_watches = NULL;
+
 	modem->sim_watch = 0;
 	modem->sim_ready_watch = 0;
 
diff --git a/src/ofono.h b/src/ofono.h
index ab6ecd2..01cd6c0 100644
--- a/src/ofono.h
+++ b/src/ofono.h
@@ -177,6 +177,14 @@ unsigned int __ofono_modemwatch_add(ofono_modemwatch_cb_t cb, void *user,
 					ofono_destroy_func destroy);
 gboolean __ofono_modemwatch_remove(unsigned int id);
 
+typedef void (*ofono_modem_online_notify_func)(ofono_bool_t online,
+		void *data);
+unsigned int __ofono_modem_add_online_watch(struct ofono_modem *modem,
+		ofono_modem_online_notify_func notify,
+		void *data, ofono_destroy_func destroy);
+void __ofono_modem_remove_online_watch(struct ofono_modem *modem,
+		unsigned int id);
+
 #include <ofono/call-barring.h>
 
 gboolean __ofono_call_barring_is_busy(struct ofono_call_barring *cb);
-- 
1.7.0.4

Re: [RFC PATCH 1/4] modem: add modem online-offline watch

[Denis Kenzior]

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

Hi Andras,

On 11/15/2010 10:57 AM, Andras Domokos wrote:
> From: Andras Domokos <andras.domokos@nokia.com>
> 
> ---
>  src/modem.c |   46 ++++++++++++++++++++++++++++++++++++++++++++++
>  src/ofono.h |    8 ++++++++
>  2 files changed, 54 insertions(+), 0 deletions(-)
> 

Patch has been applied, but I tweaked style and made one minor change
afterwards.  Please let me know if you agree with it.

Regards,
-Denis

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

[Andras Domokos]

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

From: Andras Domokos <andras.domokos@nokia.com>

---
 src/modem.c |  134 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 src/ofono.h |    4 ++
 2 files changed, 138 insertions(+), 0 deletions(-)

diff --git a/src/modem.c b/src/modem.c
index f73cc1d..4307914 100644
--- a/src/modem.c
+++ b/src/modem.c
@@ -61,6 +61,7 @@ enum modem_state {
 struct ofono_modem {
 	char			*path;
 	enum modem_state	modem_state;
+	enum modem_state	modem_state_pre_emergency;
 	GSList			*atoms;
 	struct ofono_watchlist	*atom_watches;
 	GSList			*interface_list;
@@ -72,6 +73,7 @@ struct ofono_modem {
 	ofono_bool_t		powered_pending;
 	guint			timeout;
 	ofono_bool_t		online;
+	unsigned int		emergency_mode;
 	struct ofono_watchlist	*online_watches;
 	GHashTable		*properties;
 	struct ofono_sim	*sim;
@@ -514,6 +516,127 @@ static void offline_cb(const struct ofono_error *error, void *data)
 		modem_change_state(modem, MODEM_STATE_OFFLINE);
 }
 
+ofono_bool_t ofono_modem_get_emergency_mode(struct ofono_modem *modem)
+{
+	if (modem == NULL)
+		return FALSE;
+
+	return modem->emergency_mode != 0;
+}
+
+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;
+
+	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)
+		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);
+
+	modem->emergency_mode--;
+
+	ofono_dbus_signal_property_changed(conn, path,
+						OFONO_MODEM_INTERFACE,
+						"EmergencyMode",
+						DBUS_TYPE_BOOLEAN,
+						&state);
+}
+
+void ofono_modem_inc_emergency_mode(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_mode++;
+	if (modem->emergency_mode > 1)
+		return;
+
+	ofono_dbus_signal_property_changed(conn, path,
+						OFONO_MODEM_INTERFACE,
+						"EmergencyMode",
+						DBUS_TYPE_BOOLEAN,
+						&state);
+
+	modem->modem_state_pre_emergency = modem->modem_state;
+
+	if (modem->modem_state == MODEM_STATE_ONLINE)
+		return;
+
+	modem->driver->set_online(modem, TRUE, emergency_online_cb, modem);
+}
+
+void ofono_modem_dec_emergency_mode(struct ofono_modem *modem)
+{
+	DBusConnection *conn = ofono_dbus_get_connection();
+	const char *path = ofono_modem_get_path(modem);
+	gboolean state = FALSE;
+
+	DBG("modem: %p", modem);
+
+	if (modem->emergency_mode == 0)
+		return;
+
+	if (modem->emergency_mode > 1) {
+		modem->emergency_mode--;
+		return;
+	}
+
+	if (modem->modem_state == MODEM_STATE_ONLINE &&
+		modem->modem_state_pre_emergency != MODEM_STATE_ONLINE) {
+		modem->driver->set_online(modem, FALSE,
+						emergency_offline_cb, modem);
+		return;
+	}
+
+	modem->emergency_mode--;
+
+	ofono_dbus_signal_property_changed(conn, path,
+						OFONO_MODEM_INTERFACE,
+						"EmergencyMode",
+						DBUS_TYPE_BOOLEAN,
+						&state);
+}
+
 static DBusMessage *set_property_online(struct ofono_modem *modem,
 					DBusMessage *msg,
 					DBusMessageIter *var)
@@ -535,6 +658,9 @@ static DBusMessage *set_property_online(struct ofono_modem *modem,
 	if (modem->modem_state < MODEM_STATE_OFFLINE)
 		return __ofono_error_not_available(msg);
 
+	if (modem->emergency_mode)
+		return __ofono_error_failed(msg);
+
 	if (modem->online == online)
 		return dbus_message_new_method_return(msg);
 
@@ -562,6 +688,7 @@ void __ofono_modem_append_properties(struct ofono_modem *modem,
 	int i;
 	GSList *l;
 	struct ofono_atom *devinfo_atom;
+	ofono_bool_t state;
 
 	ofono_dbus_dict_append(dict, "Online", DBUS_TYPE_BOOLEAN,
 				&modem->online);
@@ -569,6 +696,10 @@ void __ofono_modem_append_properties(struct ofono_modem *modem,
 	ofono_dbus_dict_append(dict, "Powered", DBUS_TYPE_BOOLEAN,
 				&modem->powered);
 
+	state = ofono_modem_get_emergency_mode(modem);
+	ofono_dbus_dict_append(dict, "EmergencyMode",
+				DBUS_TYPE_BOOLEAN, &state);
+
 	devinfo_atom = __ofono_modem_find_atom(modem, OFONO_ATOM_TYPE_DEVINFO);
 
 	/* We cheat a little here and don't check the registered status */
@@ -647,6 +778,9 @@ static int set_powered(struct ofono_modem *modem, ofono_bool_t powered)
 	if (modem->powered_pending == powered)
 		return -EALREADY;
 
+	if (modem->emergency_mode)
+		return -EINVAL;
+
 	/* Remove the atoms even if the driver is no longer available */
 	if (powered == FALSE)
 		modem_change_state(modem, MODEM_STATE_POWER_OFF);
diff --git a/src/ofono.h b/src/ofono.h
index 01cd6c0..ac56a85 100644
--- a/src/ofono.h
+++ b/src/ofono.h
@@ -185,6 +185,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_mode(struct ofono_modem *modem);
+void ofono_modem_inc_emergency_mode(struct ofono_modem *modem);
+void ofono_modem_dec_emergency_mode(struct ofono_modem *modem);
+
 #include <ofono/call-barring.h>
 
 gboolean __ofono_call_barring_is_busy(struct ofono_call_barring *cb);
-- 
1.7.0.4

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

[Denis Kenzior]

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

Hi Andras,

On 11/15/2010 10:57 AM, Andras Domokos wrote:
> From: Andras Domokos <andras.domokos@nokia.com>
> 
> ---
>  src/modem.c |  134 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  src/ofono.h |    4 ++
>  2 files changed, 138 insertions(+), 0 deletions(-)
> 
> diff --git a/src/modem.c b/src/modem.c
> index f73cc1d..4307914 100644
> --- a/src/modem.c
> +++ b/src/modem.c
> @@ -61,6 +61,7 @@ enum modem_state {
>  struct ofono_modem {
>  	char			*path;
>  	enum modem_state	modem_state;
> +	enum modem_state	modem_state_pre_emergency;
>  	GSList			*atoms;
>  	struct ofono_watchlist	*atom_watches;
>  	GSList			*interface_list;
> @@ -72,6 +73,7 @@ struct ofono_modem {
>  	ofono_bool_t		powered_pending;
>  	guint			timeout;
>  	ofono_bool_t		online;
> +	unsigned int		emergency_mode;

I really prefer this to be called 'emergency'

>  	struct ofono_watchlist	*online_watches;
>  	GHashTable		*properties;
>  	struct ofono_sim	*sim;
> @@ -514,6 +516,127 @@ static void offline_cb(const struct ofono_error *error, void *data)
>  		modem_change_state(modem, MODEM_STATE_OFFLINE);
>  }
>  
> +ofono_bool_t ofono_modem_get_emergency_mode(struct ofono_modem *modem)
> +{
> +	if (modem == NULL)
> +		return FALSE;
> +
> +	return modem->emergency_mode != 0;
> +}
> +
> +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;
> +
> +	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)
> +		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);
> +
> +	modem->emergency_mode--;
> +
> +	ofono_dbus_signal_property_changed(conn, path,
> +						OFONO_MODEM_INTERFACE,
> +						"EmergencyMode",

The property should really be called 'Emergency' to be in line with the
current API proposal (doc/modem-api.txt & TODO)

> +						DBUS_TYPE_BOOLEAN,
> +						&state);
> +}
> +
> +void ofono_modem_inc_emergency_mode(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_mode++;
> +	if (modem->emergency_mode > 1)
> +		return;
> +
> +	ofono_dbus_signal_property_changed(conn, path,
> +						OFONO_MODEM_INTERFACE,
> +						"EmergencyMode",

Again, 'Emergency' here

> +						DBUS_TYPE_BOOLEAN,
> +						&state);
> +
> +	modem->modem_state_pre_emergency = modem->modem_state;
> +
> +	if (modem->modem_state == MODEM_STATE_ONLINE)
> +		return;
> +
> +	modem->driver->set_online(modem, TRUE, emergency_online_cb, modem);
> +}
> +
> +void ofono_modem_dec_emergency_mode(struct ofono_modem *modem)
> +{
> +	DBusConnection *conn = ofono_dbus_get_connection();
> +	const char *path = ofono_modem_get_path(modem);
> +	gboolean state = FALSE;
> +
> +	DBG("modem: %p", modem);
> +
> +	if (modem->emergency_mode == 0)
> +		return;

Can you be a bit more pedantic and send an ofono_error for this case?
We probably want to track whether some plugin abuses the reference counting.

> +
> +	if (modem->emergency_mode > 1) {
> +		modem->emergency_mode--;
> +		return;
> +	}
> +
> +	if (modem->modem_state == MODEM_STATE_ONLINE &&
> +		modem->modem_state_pre_emergency != MODEM_STATE_ONLINE) {

Please indent one more, item M4 in coding-style.txt

> +		modem->driver->set_online(modem, FALSE,
> +						emergency_offline_cb, modem);
> +		return;
> +	}
> +
> +	modem->emergency_mode--;
> +
> +	ofono_dbus_signal_property_changed(conn, path,
> +						OFONO_MODEM_INTERFACE,
> +						"EmergencyMode",

And again, 'Emergency' here.

> +						DBUS_TYPE_BOOLEAN,
> +						&state);
> +}
> +
>  static DBusMessage *set_property_online(struct ofono_modem *modem,
>  					DBusMessage *msg,
>  					DBusMessageIter *var)
> @@ -535,6 +658,9 @@ static DBusMessage *set_property_online(struct ofono_modem *modem,
>  	if (modem->modem_state < MODEM_STATE_OFFLINE)
>  		return __ofono_error_not_available(msg);
>  
> +	if (modem->emergency_mode)
> +		return __ofono_error_failed(msg);
> +

Is it worth to create a new dbus error for this case?  Perhaps
ofono_error_emergency, or emergency_active?

>  	if (modem->online == online)
>  		return dbus_message_new_method_return(msg);
>  
> @@ -562,6 +688,7 @@ void __ofono_modem_append_properties(struct ofono_modem *modem,
>  	int i;
>  	GSList *l;
>  	struct ofono_atom *devinfo_atom;
> +	ofono_bool_t state;

Please rename this variable to 'val' or 'value'

>  
>  	ofono_dbus_dict_append(dict, "Online", DBUS_TYPE_BOOLEAN,
>  				&modem->online);
> @@ -569,6 +696,10 @@ void __ofono_modem_append_properties(struct ofono_modem *modem,
>  	ofono_dbus_dict_append(dict, "Powered", DBUS_TYPE_BOOLEAN,
>  				&modem->powered);
>  
> +	state = ofono_modem_get_emergency_mode(modem);
> +	ofono_dbus_dict_append(dict, "EmergencyMode",

And 'Emergency' here

> +				DBUS_TYPE_BOOLEAN, &state);
> +
>  	devinfo_atom = __ofono_modem_find_atom(modem, OFONO_ATOM_TYPE_DEVINFO);
>  
>  	/* We cheat a little here and don't check the registered status */
> @@ -647,6 +778,9 @@ static int set_powered(struct ofono_modem *modem, ofono_bool_t powered)
>  	if (modem->powered_pending == powered)
>  		return -EALREADY;
>  
> +	if (modem->emergency_mode)

I really would prefer modem->emergency > 0 here

> +		return -EINVAL;
> +

If we introduce a new ofono error, then returning something like EPERM
or EACCESS for the case where emergency mode is active might be a good idea.

>  	/* Remove the atoms even if the driver is no longer available */
>  	if (powered == FALSE)
>  		modem_change_state(modem, MODEM_STATE_POWER_OFF);
> diff --git a/src/ofono.h b/src/ofono.h
> index 01cd6c0..ac56a85 100644
> --- a/src/ofono.h
> +++ b/src/ofono.h
> @@ -185,6 +185,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_mode(struct ofono_modem *modem);
> +void ofono_modem_inc_emergency_mode(struct ofono_modem *modem);
> +void ofono_modem_dec_emergency_mode(struct ofono_modem *modem);
> +
>  #include <ofono/call-barring.h>
>  
>  gboolean __ofono_call_barring_is_busy(struct ofono_call_barring *cb);

Otherwise this one looks good to me.

Regards,
-Denis

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

[Andras Domokos]

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

Hi Denis,

On 11/23/2010 10:46 AM, ext Denis Kenzior wrote:
> Hi Andras,
>
> On 11/15/2010 10:57 AM, Andras Domokos wrote:
>    
>> From: Andras Domokos<andras.domokos@nokia.com>
>>
>> ---
>>   src/modem.c |  134 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>   src/ofono.h |    4 ++
>>   2 files changed, 138 insertions(+), 0 deletions(-)
>>
>> diff --git a/src/modem.c b/src/modem.c
>> index f73cc1d..4307914 100644
>> --- a/src/modem.c
>> +++ b/src/modem.c
>> @@ -61,6 +61,7 @@ enum modem_state {
>>   struct ofono_modem {
>>        char                    *path;
>>        enum modem_state        modem_state;
>> +     enum modem_state        modem_state_pre_emergency;
>>        GSList                  *atoms;
>>        struct ofono_watchlist  *atom_watches;
>>        GSList                  *interface_list;
>> @@ -72,6 +73,7 @@ struct ofono_modem {
>>        ofono_bool_t            powered_pending;
>>        guint                   timeout;
>>        ofono_bool_t            online;
>> +     unsigned int            emergency_mode;
>>      
> I really prefer this to be called 'emergency'
>
>    
>>        struct ofono_watchlist  *online_watches;
>>        GHashTable              *properties;
>>        struct ofono_sim        *sim;
>> @@ -514,6 +516,127 @@ static void offline_cb(const struct ofono_error *error, void *data)
>>                modem_change_state(modem, MODEM_STATE_OFFLINE);
>>   }
>>
>> +ofono_bool_t ofono_modem_get_emergency_mode(struct ofono_modem *modem)
>> +{
>> +     if (modem == NULL)
>> +             return FALSE;
>> +
>> +     return modem->emergency_mode != 0;
>> +}
>> +
>> +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;
>> +
>> +     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)
>> +             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);
>> +
>> +     modem->emergency_mode--;
>> +
>> +     ofono_dbus_signal_property_changed(conn, path,
>> +                                             OFONO_MODEM_INTERFACE,
>> +                                             "EmergencyMode",
>>      
> The property should really be called 'Emergency' to be in line with the
> current API proposal (doc/modem-api.txt&  TODO)
>
>    
>> +                                             DBUS_TYPE_BOOLEAN,
>> +&state);
>> +}
>> +
>> +void ofono_modem_inc_emergency_mode(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_mode++;
>> +     if (modem->emergency_mode>  1)
>> +             return;
>> +
>> +     ofono_dbus_signal_property_changed(conn, path,
>> +                                             OFONO_MODEM_INTERFACE,
>> +                                             "EmergencyMode",
>>      
> Again, 'Emergency' here
>
>    
>> +                                             DBUS_TYPE_BOOLEAN,
>> +&state);
>> +
>> +     modem->modem_state_pre_emergency = modem->modem_state;
>> +
>> +     if (modem->modem_state == MODEM_STATE_ONLINE)
>> +             return;
>> +
>> +     modem->driver->set_online(modem, TRUE, emergency_online_cb, modem);
>> +}
>> +
>> +void ofono_modem_dec_emergency_mode(struct ofono_modem *modem)
>> +{
>> +     DBusConnection *conn = ofono_dbus_get_connection();
>> +     const char *path = ofono_modem_get_path(modem);
>> +     gboolean state = FALSE;
>> +
>> +     DBG("modem: %p", modem);
>> +
>> +     if (modem->emergency_mode == 0)
>> +             return;
>>      
> Can you be a bit more pedantic and send an ofono_error for this case?
> We probably want to track whether some plugin abuses the reference counting.
>
>    
>> +
>> +     if (modem->emergency_mode>  1) {
>> +             modem->emergency_mode--;
>> +             return;
>> +     }
>> +
>> +     if (modem->modem_state == MODEM_STATE_ONLINE&&
>> +             modem->modem_state_pre_emergency != MODEM_STATE_ONLINE) {
>>      
> Please indent one more, item M4 in coding-style.txt
>
>    
>> +             modem->driver->set_online(modem, FALSE,
>> +                                             emergency_offline_cb, modem);
>> +             return;
>> +     }
>> +
>> +     modem->emergency_mode--;
>> +
>> +     ofono_dbus_signal_property_changed(conn, path,
>> +                                             OFONO_MODEM_INTERFACE,
>> +                                             "EmergencyMode",
>>      
> And again, 'Emergency' here.
>
>    
>> +                                             DBUS_TYPE_BOOLEAN,
>> +&state);
>> +}
>> +
>>   static DBusMessage *set_property_online(struct ofono_modem *modem,
>>                                        DBusMessage *msg,
>>                                        DBusMessageIter *var)
>> @@ -535,6 +658,9 @@ static DBusMessage *set_property_online(struct ofono_modem *modem,
>>        if (modem->modem_state<  MODEM_STATE_OFFLINE)
>>                return __ofono_error_not_available(msg);
>>
>> +     if (modem->emergency_mode)
>> +             return __ofono_error_failed(msg);
>> +
>>      
> Is it worth to create a new dbus error for this case?  Perhaps
> ofono_error_emergency, or emergency_active?
>
>    
>>        if (modem->online == online)
>>                return dbus_message_new_method_return(msg);
>>
>> @@ -562,6 +688,7 @@ void __ofono_modem_append_properties(struct ofono_modem *modem,
>>        int i;
>>        GSList *l;
>>        struct ofono_atom *devinfo_atom;
>> +     ofono_bool_t state;
>>      
> Please rename this variable to 'val' or 'value'
>
>    
>>        ofono_dbus_dict_append(dict, "Online", DBUS_TYPE_BOOLEAN,
>>                                &modem->online);
>> @@ -569,6 +696,10 @@ void __ofono_modem_append_properties(struct ofono_modem *modem,
>>        ofono_dbus_dict_append(dict, "Powered", DBUS_TYPE_BOOLEAN,
>>                                &modem->powered);
>>
>> +     state = ofono_modem_get_emergency_mode(modem);
>> +     ofono_dbus_dict_append(dict, "EmergencyMode",
>>      
> And 'Emergency' here
>
>    
>> +                             DBUS_TYPE_BOOLEAN,&state);
>> +
>>        devinfo_atom = __ofono_modem_find_atom(modem, OFONO_ATOM_TYPE_DEVINFO);
>>
>>        /* We cheat a little here and don't check the registered status */
>> @@ -647,6 +778,9 @@ static int set_powered(struct ofono_modem *modem, ofono_bool_t powered)
>>        if (modem->powered_pending == powered)
>>                return -EALREADY;
>>
>> +     if (modem->emergency_mode)
>>      
> I really would prefer modem->emergency>  0 here
>
>    
>> +             return -EINVAL;
>> +
>>      
> If we introduce a new ofono error, then returning something like EPERM
> or EACCESS for the case where emergency mode is active might be a good idea.
>
>    
>>        /* Remove the atoms even if the driver is no longer available */
>>        if (powered == FALSE)
>>                modem_change_state(modem, MODEM_STATE_POWER_OFF);
>> diff --git a/src/ofono.h b/src/ofono.h
>> index 01cd6c0..ac56a85 100644
>> --- a/src/ofono.h
>> +++ b/src/ofono.h
>> @@ -185,6 +185,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_mode(struct ofono_modem *modem);
>> +void ofono_modem_inc_emergency_mode(struct ofono_modem *modem);
>> +void ofono_modem_dec_emergency_mode(struct ofono_modem *modem);
>> +
>>   #include<ofono/call-barring.h>
>>
>>   gboolean __ofono_call_barring_is_busy(struct ofono_call_barring *cb);
>>      
> Otherwise this one looks good to me.
>    
I made changes based on your comments and I am going to
resend the patch.

> Regards,
> -Denis
>    
Regards,
Andras

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

[Andras Domokos]

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

---
 src/dbus.c  |    7 +++
 src/modem.c |  137 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 src/ofono.h |    5 ++
 3 files changed, 149 insertions(+), 0 deletions(-)

diff --git a/src/dbus.c b/src/dbus.c
index ad29241..0b4fc06 100644
--- a/src/dbus.c
+++ b/src/dbus.c
@@ -348,6 +348,13 @@ DBusMessage *__ofono_error_access_denied(DBusMessage *msg)
 					"Operation not permitted");
 }
 
+DBusMessage *__ofono_error_emergency_active(DBusMessage *msg)
+{
+	return g_dbus_create_error(msg, OFONO_ERROR_INTERFACE
+					".EmergencyActive",
+					"Emergency state active");
+}
+
 void __ofono_dbus_pending_reply(DBusMessage **msg, DBusMessage *reply)
 {
 	DBusConnection *conn = ofono_dbus_get_connection();
diff --git a/src/modem.c b/src/modem.c
index cfc767e..f005877 100644
--- a/src/modem.c
+++ b/src/modem.c
@@ -61,6 +61,7 @@ enum modem_state {
 struct ofono_modem {
 	char			*path;
 	enum modem_state	modem_state;
+	enum modem_state	modem_state_pre_emergency;
 	GSList			*atoms;
 	struct ofono_watchlist	*atom_watches;
 	GSList			*interface_list;
@@ -73,6 +74,7 @@ struct ofono_modem {
 	guint			timeout;
 	ofono_bool_t		online;
 	struct ofono_watchlist	*online_watches;
+	unsigned int		emergency;
 	GHashTable		*properties;
 	struct ofono_sim	*sim;
 	unsigned int		sim_watch;
@@ -517,6 +519,130 @@ static void offline_cb(const struct ofono_error *error, void *data)
 		modem_change_state(modem, MODEM_STATE_OFFLINE);
 }
 
+ofono_bool_t ofono_modem_get_emergency(struct ofono_modem *modem)
+{
+	if (modem == NULL)
+		return FALSE;
+
+	return modem->emergency != 0;
+}
+
+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;
+
+	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)
+		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);
+
+	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;
+
+	modem->driver->set_online(modem, TRUE, emergency_online_cb, modem);
+}
+
+void ofono_modem_dec_emergency(struct ofono_modem *modem)
+{
+	DBusConnection *conn = ofono_dbus_get_connection();
+	const char *path = ofono_modem_get_path(modem);
+	gboolean state = FALSE;
+
+	DBG("modem: %p", modem);
+
+	if (modem->emergency == 0) {
+		ofono_error("Emergency reference counter already 0 on path %s",
+				modem->path);
+		return;
+	}
+
+	if (modem->emergency > 1) {
+		modem->emergency--;
+		return;
+	}
+
+	if (modem->modem_state_pre_emergency != MODEM_STATE_ONLINE &&
+			modem->modem_state == MODEM_STATE_ONLINE) {
+		modem->driver->set_online(modem, FALSE,
+		emergency_offline_cb, modem);
+		return;
+	}
+
+	modem->emergency--;
+
+	ofono_dbus_signal_property_changed(conn, path,
+						OFONO_MODEM_INTERFACE,
+						"Emergency",
+						DBUS_TYPE_BOOLEAN,
+						&state);
+}
+
 static DBusMessage *set_property_online(struct ofono_modem *modem,
 					DBusMessage *msg,
 					DBusMessageIter *var)
@@ -538,6 +664,9 @@ static DBusMessage *set_property_online(struct ofono_modem *modem,
 	if (modem->modem_state < MODEM_STATE_OFFLINE)
 		return __ofono_error_not_available(msg);
 
+	if (modem->emergency > 0)
+		return __ofono_error_emergency_active(msg);
+
 	if (modem->online == online)
 		return dbus_message_new_method_return(msg);
 
@@ -565,6 +694,7 @@ void __ofono_modem_append_properties(struct ofono_modem *modem,
 	int i;
 	GSList *l;
 	struct ofono_atom *devinfo_atom;
+	ofono_bool_t val;
 
 	ofono_dbus_dict_append(dict, "Online", DBUS_TYPE_BOOLEAN,
 				&modem->online);
@@ -572,6 +702,10 @@ void __ofono_modem_append_properties(struct ofono_modem *modem,
 	ofono_dbus_dict_append(dict, "Powered", DBUS_TYPE_BOOLEAN,
 				&modem->powered);
 
+	val = ofono_modem_get_emergency(modem);
+	ofono_dbus_dict_append(dict, "Emergency",
+				DBUS_TYPE_BOOLEAN, &val);
+
 	devinfo_atom = __ofono_modem_find_atom(modem, OFONO_ATOM_TYPE_DEVINFO);
 
 	/* We cheat a little here and don't check the registered status */
@@ -650,6 +784,9 @@ static int set_powered(struct ofono_modem *modem, ofono_bool_t powered)
 	if (modem->powered_pending == powered)
 		return -EALREADY;
 
+	if (modem->emergency > 0)
+		return -EPERM;
+
 	/* Remove the atoms even if the driver is no longer available */
 	if (powered == FALSE)
 		modem_change_state(modem, MODEM_STATE_POWER_OFF);
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);
 
 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);
-- 
1.7.0.4

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

[Denis Kenzior]

[-- 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

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

[Pekka Pessi]

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

Hi Denis and Andras,

2010/12/3 Denis Kenzior <denkenz@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

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

[Denis Kenzior]

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

Hi Pekka,

>> 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.

There's really no way for the voicecall atom to know that a modem is
undergoing an online / offline transition..

> 
> 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)?
> 

So my thinking was essentially along the same lines.  We can check
whether an Online change operation is in progress by peeking at the
pending D-Bus message.  If we're going online, then not triggering
another set_online call and waiting for the online_cb should be sufficient.

If we're going offline, then returning FALSE from ofono_modem_get_online
and re-checking the emergency counter in the offline_cb seems like the
way to go.

I'd like to take care of this completely inside oFono and not bother the
dialer with these details.  Thoughts?

Regards,
-Denis

[RFC PATCH 3/4] modem: move dial_request_cb function

[Andras Domokos]

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

---
 src/voicecall.c |   70 +++++++++++++++++++++++++++---------------------------
 1 files changed, 35 insertions(+), 35 deletions(-)

diff --git a/src/voicecall.c b/src/voicecall.c
index bd64432..3af614b 100644
--- a/src/voicecall.c
+++ b/src/voicecall.c
@@ -2202,6 +2202,41 @@ static void sim_watch(struct ofono_atom *atom,
 	sim_state_watch(ofono_sim_get_state(sim), vc);
 }
 
+static void dial_request_cb(const struct ofono_error *error, void *data)
+{
+	struct ofono_voicecall *vc = data;
+	gboolean need_to_emit;
+	struct voicecall *v;
+
+	v = dial_handle_result(vc, error,
+				phone_number_to_string(&vc->dial_req->ph),
+				&need_to_emit);
+
+	if (v == NULL) {
+		dial_request_finish(vc);
+		return;
+	}
+
+	v->message = vc->dial_req->message;
+	v->icon_id = vc->dial_req->icon_id;
+
+	vc->dial_req->message = NULL;
+	vc->dial_req->call = v;
+
+	/*
+	 * TS 102 223 Section 6.4.13: The terminal shall not store
+	 * in the UICC the call set-up details (called party number
+	 * and associated parameters)
+	 */
+	v->untracked = TRUE;
+
+	if (v->call->status == CALL_STATUS_ACTIVE)
+		dial_request_finish(vc);
+
+	if (need_to_emit)
+		voicecalls_emit_call_added(vc, v);
+}
+
 void ofono_voicecall_register(struct ofono_voicecall *vc)
 {
 	DBusConnection *conn = ofono_dbus_get_connection();
@@ -2294,41 +2329,6 @@ ofono_bool_t __ofono_voicecall_is_busy(struct ofono_voicecall *vc,
 	return TRUE;
 }
 
-static void dial_request_cb(const struct ofono_error *error, void *data)
-{
-	struct ofono_voicecall *vc = data;
-	gboolean need_to_emit;
-	struct voicecall *v;
-
-	v = dial_handle_result(vc, error,
-				phone_number_to_string(&vc->dial_req->ph),
-				&need_to_emit);
-
-	if (v == NULL) {
-		dial_request_finish(vc);
-		return;
-	}
-
-	v->message = vc->dial_req->message;
-	v->icon_id = vc->dial_req->icon_id;
-
-	vc->dial_req->message = NULL;
-	vc->dial_req->call = v;
-
-	/*
-	 * TS 102 223 Section 6.4.13: The terminal shall not store
-	 * in the UICC the call set-up details (called party number
-	 * and associated parameters)
-	 */
-	v->untracked = TRUE;
-
-	if (v->call->status == CALL_STATUS_ACTIVE)
-		dial_request_finish(vc);
-
-	if (need_to_emit)
-		voicecalls_emit_call_added(vc, v);
-}
-
 static void dial_request(struct ofono_voicecall *vc)
 {
 	vc->driver->dial(vc, &vc->dial_req->ph, OFONO_CLIR_OPTION_DEFAULT,
-- 
1.7.0.4

[RFC PATCH 4/4] voicecall: add emergency call handling

[Andras Domokos]

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

---
 src/voicecall.c |  111 ++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 files changed, 110 insertions(+), 1 deletions(-)

diff --git a/src/voicecall.c b/src/voicecall.c
index 3af614b..066cdb9 100644
--- a/src/voicecall.c
+++ b/src/voicecall.c
@@ -52,6 +52,7 @@ struct ofono_voicecall {
 	struct ofono_sim *sim;
 	unsigned int sim_watch;
 	unsigned int sim_state_watch;
+	unsigned int modem_online_watch;
 	const struct ofono_voicecall_driver *driver;
 	void *driver_data;
 	struct ofono_atom *atom;
@@ -133,6 +134,22 @@ static void add_to_en_list(GSList **l, const char **list)
 		*l = g_slist_prepend(*l, g_strdup(list[i++]));
 }
 
+static gint number_compare(gconstpointer a, gconstpointer b)
+{
+	const char *s1 = a, *s2 = b;
+	return strcmp(s1, s2);
+}
+
+static ofono_bool_t emergency_number(struct ofono_voicecall *vc,
+					const char *number)
+{
+	if (!number)
+		return FALSE;
+
+	return g_slist_find_custom(vc->en_list,
+				number, number_compare) ? TRUE : FALSE;
+}
+
 static const char *disconnect_reason_to_string(enum ofono_disconnect_reason r)
 {
 	switch (r) {
@@ -1125,6 +1142,7 @@ static struct voicecall *dial_handle_result(struct ofono_voicecall *vc,
 static void manager_dial_callback(const struct ofono_error *error, void *data)
 {
 	struct ofono_voicecall *vc = data;
+	struct ofono_modem *modem = __ofono_atom_get_modem(vc->atom);
 	DBusMessage *reply;
 	const char *number;
 	gboolean need_to_emit;
@@ -1143,8 +1161,12 @@ static void manager_dial_callback(const struct ofono_error *error, void *data)
 
 		dbus_message_append_args(reply, DBUS_TYPE_OBJECT_PATH, &path,
 						DBUS_TYPE_INVALID);
-	} else
+	} else {
+		if (emergency_number(vc, number))
+			ofono_modem_dec_emergency_mode(modem);
+
 		reply = __ofono_error_failed(vc->pending);
+	}
 
 	__ofono_dbus_pending_reply(&vc->pending, reply);
 
@@ -1156,6 +1178,7 @@ static DBusMessage *manager_dial(DBusConnection *conn,
 					DBusMessage *msg, void *data)
 {
 	struct ofono_voicecall *vc = data;
+	struct ofono_modem *modem = __ofono_atom_get_modem(vc->atom);
 	const char *number;
 	struct ofono_phone_number ph;
 	const char *clirstr;
@@ -1195,6 +1218,15 @@ static DBusMessage *manager_dial(DBusConnection *conn,
 
 	string_to_phone_number(number, &ph);
 
+	if (emergency_number(vc, number)) {
+		ofono_bool_t online = ofono_modem_get_online(modem);
+
+		ofono_modem_inc_emergency_mode(modem);
+
+		if (!online)
+			return NULL;
+	}
+
 	vc->driver->dial(vc, &ph, clir, OFONO_CUG_OPTION_DEFAULT,
 				manager_dial_callback, vc);
 
@@ -1748,6 +1780,7 @@ void ofono_voicecall_disconnected(struct ofono_voicecall *vc, int id,
 				const struct ofono_error *error)
 {
 	struct ofono_modem *modem = __ofono_atom_get_modem(vc->atom);
+	const char *number;
 	GSList *l;
 	struct voicecall *call;
 	time_t ts;
@@ -1767,6 +1800,7 @@ void ofono_voicecall_disconnected(struct ofono_voicecall *vc, int id,
 	}
 
 	call = l->data;
+	number = phone_number_to_string(&call->call->phone_number);
 
 	ts = time(NULL);
 	prev_status = call->call->status;
@@ -1805,6 +1839,9 @@ void ofono_voicecall_disconnected(struct ofono_voicecall *vc, int id,
 
 	voicecalls_emit_call_removed(vc, call);
 
+	if (emergency_number(vc, number))
+		ofono_modem_dec_emergency_mode(modem);
+
 	voicecall_dbus_unregister(vc, call);
 
 	vc->call_list = g_slist_remove(vc->call_list, call);
@@ -2067,6 +2104,7 @@ static void voicecall_unregister(struct ofono_atom *atom)
 static void voicecall_remove(struct ofono_atom *atom)
 {
 	struct ofono_voicecall *vc = __ofono_atom_get_data(atom);
+	struct ofono_modem *modem = __ofono_atom_get_modem(atom);
 
 	DBG("atom: %p", atom);
 
@@ -2108,6 +2146,12 @@ static void voicecall_remove(struct ofono_atom *atom)
 		g_queue_free(vc->toneq);
 	}
 
+	if (vc->modem_online_watch) {
+		__ofono_modem_remove_online_watch(modem,
+						vc->modem_online_watch);
+		vc->modem_online_watch = 0;
+	}
+
 	g_free(vc);
 }
 
@@ -2205,6 +2249,7 @@ static void sim_watch(struct ofono_atom *atom,
 static void dial_request_cb(const struct ofono_error *error, void *data)
 {
 	struct ofono_voicecall *vc = data;
+	struct ofono_modem *modem = __ofono_atom_get_modem(vc->atom);
 	gboolean need_to_emit;
 	struct voicecall *v;
 
@@ -2214,6 +2259,9 @@ static void dial_request_cb(const struct ofono_error *error, void *data)
 
 	if (v == NULL) {
 		dial_request_finish(vc);
+		if (emergency_number(vc,
+				phone_number_to_string(&vc->dial_req->ph)))
+			ofono_modem_dec_emergency_mode(modem);
 		return;
 	}
 
@@ -2237,6 +2285,53 @@ static void dial_request_cb(const struct ofono_error *error, void *data)
 		voicecalls_emit_call_added(vc, v);
 }
 
+static void modem_online_watch(ofono_bool_t online, void *data)
+{
+	struct ofono_voicecall *vc = data;
+	struct ofono_modem *modem = __ofono_atom_get_modem(vc->atom);
+	DBusMessage *reply;
+	const char *number;
+	struct ofono_phone_number ph;
+	const char *clirstr;
+	enum ofono_clir_option clir;
+
+	if (ofono_modem_get_emergency_mode(modem) != TRUE)
+		return;
+
+	if (vc->dial_req)
+		vc->driver->dial(vc, &vc->dial_req->ph,
+					OFONO_CLIR_OPTION_DEFAULT,
+					OFONO_CUG_OPTION_DEFAULT,
+					dial_request_cb, vc);
+
+	if (!vc->pending)
+		return;
+
+	if (strcmp(dbus_message_get_member(vc->pending), "Dial"))
+		return;
+
+	if (dbus_message_get_args(vc->pending, NULL, DBUS_TYPE_STRING, &number,
+					DBUS_TYPE_STRING, &clirstr,
+					DBUS_TYPE_INVALID) == FALSE)
+		return;
+
+	if (!emergency_number(vc, number))
+		return;
+
+	if (!online) {
+		reply = __ofono_error_failed(vc->pending);
+		__ofono_dbus_pending_reply(&vc->pending, reply);
+		ofono_modem_dec_emergency_mode(modem);
+		return;
+	}
+
+	clir_string_to_clir(clirstr, &clir);
+	string_to_phone_number(number, &ph);
+
+	vc->driver->dial(vc, &ph, clir, OFONO_CUG_OPTION_DEFAULT,
+				manager_dial_callback, vc);
+}
+
 void ofono_voicecall_register(struct ofono_voicecall *vc)
 {
 	DBusConnection *conn = ofono_dbus_get_connection();
@@ -2255,6 +2350,9 @@ void ofono_voicecall_register(struct ofono_voicecall *vc)
 	}
 
 	ofono_modem_add_interface(modem, OFONO_VOICECALL_MANAGER_INTERFACE);
+	vc->modem_online_watch = __ofono_modem_add_online_watch(modem,
+							modem_online_watch,
+							vc, NULL);
 
 	/*
 	 * Start out with the 22.101 mandated numbers, if we have a SIM and
@@ -2331,6 +2429,17 @@ ofono_bool_t __ofono_voicecall_is_busy(struct ofono_voicecall *vc,
 
 static void dial_request(struct ofono_voicecall *vc)
 {
+	struct ofono_modem *modem = __ofono_atom_get_modem(vc->atom);
+
+	if (emergency_number(vc, phone_number_to_string(&vc->dial_req->ph))) {
+		ofono_bool_t online = ofono_modem_get_online(modem);
+
+		ofono_modem_inc_emergency_mode(modem);
+
+		if (!online)
+			return;
+	}
+
 	vc->driver->dial(vc, &vc->dial_req->ph, OFONO_CLIR_OPTION_DEFAULT,
 				OFONO_CUG_OPTION_DEFAULT, dial_request_cb, vc);
 }
-- 
1.7.0.4

Re: [RFC PATCH 4/4] voicecall: add emergency call handling

[Denis Kenzior]

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

Hi Andras,

On 11/15/2010 10:58 AM, Andras Domokos wrote:
> ---
>  src/voicecall.c |  111 ++++++++++++++++++++++++++++++++++++++++++++++++++++++-
>  1 files changed, 110 insertions(+), 1 deletions(-)
> 
> diff --git a/src/voicecall.c b/src/voicecall.c
> index 3af614b..066cdb9 100644
> --- a/src/voicecall.c
> +++ b/src/voicecall.c
> @@ -52,6 +52,7 @@ struct ofono_voicecall {
>  	struct ofono_sim *sim;
>  	unsigned int sim_watch;
>  	unsigned int sim_state_watch;
> +	unsigned int modem_online_watch;
>  	const struct ofono_voicecall_driver *driver;
>  	void *driver_data;
>  	struct ofono_atom *atom;
> @@ -133,6 +134,22 @@ static void add_to_en_list(GSList **l, const char **list)
>  		*l = g_slist_prepend(*l, g_strdup(list[i++]));
>  }
>  
> +static gint number_compare(gconstpointer a, gconstpointer b)
> +{
> +	const char *s1 = a, *s2 = b;
> +	return strcmp(s1, s2);
> +}
> +
> +static ofono_bool_t emergency_number(struct ofono_voicecall *vc,
> +					const char *number)
> +{
> +	if (!number)

Just nit picking here, but in general we really prefer this to be
written like this:

if (number == NULL)

This is much easier to read when you don't know if number is a string or
an integer.  Yes I know we're not always consistent about doing this,
particularly in voicecall.c.

> +		return FALSE;
> +
> +	return g_slist_find_custom(vc->en_list,
> +				number, number_compare) ? TRUE : FALSE;
> +}
> +
>  static const char *disconnect_reason_to_string(enum ofono_disconnect_reason r)
>  {
>  	switch (r) {
> @@ -1125,6 +1142,7 @@ static struct voicecall *dial_handle_result(struct ofono_voicecall *vc,
>  static void manager_dial_callback(const struct ofono_error *error, void *data)
>  {
>  	struct ofono_voicecall *vc = data;
> +	struct ofono_modem *modem = __ofono_atom_get_modem(vc->atom);
>  	DBusMessage *reply;
>  	const char *number;
>  	gboolean need_to_emit;
> @@ -1143,8 +1161,12 @@ static void manager_dial_callback(const struct ofono_error *error, void *data)
>  
>  		dbus_message_append_args(reply, DBUS_TYPE_OBJECT_PATH, &path,
>  						DBUS_TYPE_INVALID);
> -	} else
> +	} else {
> +		if (emergency_number(vc, number))
> +			ofono_modem_dec_emergency_mode(modem);
> +
>  		reply = __ofono_error_failed(vc->pending);
> +	}
>  
>  	__ofono_dbus_pending_reply(&vc->pending, reply);
>  
> @@ -1156,6 +1178,7 @@ static DBusMessage *manager_dial(DBusConnection *conn,
>  					DBusMessage *msg, void *data)
>  {
>  	struct ofono_voicecall *vc = data;
> +	struct ofono_modem *modem = __ofono_atom_get_modem(vc->atom);
>  	const char *number;
>  	struct ofono_phone_number ph;
>  	const char *clirstr;
> @@ -1195,6 +1218,15 @@ static DBusMessage *manager_dial(DBusConnection *conn,
>  
>  	string_to_phone_number(number, &ph);
>  
> +	if (emergency_number(vc, number)) {
> +		ofono_bool_t online = ofono_modem_get_online(modem);
> +
> +		ofono_modem_inc_emergency_mode(modem);
> +
> +		if (!online)

Do me a favor and change this to:
if (online == FALSE)

> +			return NULL;
> +	}
> +
>  	vc->driver->dial(vc, &ph, clir, OFONO_CUG_OPTION_DEFAULT,
>  				manager_dial_callback, vc);
>  
> @@ -1748,6 +1780,7 @@ void ofono_voicecall_disconnected(struct ofono_voicecall *vc, int id,
>  				const struct ofono_error *error)
>  {
>  	struct ofono_modem *modem = __ofono_atom_get_modem(vc->atom);
> +	const char *number;
>  	GSList *l;
>  	struct voicecall *call;
>  	time_t ts;
> @@ -1767,6 +1800,7 @@ void ofono_voicecall_disconnected(struct ofono_voicecall *vc, int id,
>  	}
>  
>  	call = l->data;
> +	number = phone_number_to_string(&call->call->phone_number);
>  
>  	ts = time(NULL);
>  	prev_status = call->call->status;
> @@ -1805,6 +1839,9 @@ void ofono_voicecall_disconnected(struct ofono_voicecall *vc, int id,
>  
>  	voicecalls_emit_call_removed(vc, call);
>  
> +	if (emergency_number(vc, number))
> +		ofono_modem_dec_emergency_mode(modem);
> +
>  	voicecall_dbus_unregister(vc, call);
>  
>  	vc->call_list = g_slist_remove(vc->call_list, call);
> @@ -2067,6 +2104,7 @@ static void voicecall_unregister(struct ofono_atom *atom)
>  static void voicecall_remove(struct ofono_atom *atom)
>  {
>  	struct ofono_voicecall *vc = __ofono_atom_get_data(atom);
> +	struct ofono_modem *modem = __ofono_atom_get_modem(atom);
>  
>  	DBG("atom: %p", atom);
>  
> @@ -2108,6 +2146,12 @@ static void voicecall_remove(struct ofono_atom *atom)
>  		g_queue_free(vc->toneq);
>  	}
>  
> +	if (vc->modem_online_watch) {
> +		__ofono_modem_remove_online_watch(modem,
> +						vc->modem_online_watch);
> +		vc->modem_online_watch = 0;
> +	}
> +
>  	g_free(vc);
>  }
>  
> @@ -2205,6 +2249,7 @@ static void sim_watch(struct ofono_atom *atom,
>  static void dial_request_cb(const struct ofono_error *error, void *data)
>  {
>  	struct ofono_voicecall *vc = data;
> +	struct ofono_modem *modem = __ofono_atom_get_modem(vc->atom);
>  	gboolean need_to_emit;
>  	struct voicecall *v;
>  
> @@ -2214,6 +2259,9 @@ static void dial_request_cb(const struct ofono_error *error, void *data)
>  
>  	if (v == NULL) {
>  		dial_request_finish(vc);

Please add an empty line here based on item M1.

> +		if (emergency_number(vc,
> +				phone_number_to_string(&vc->dial_req->ph)))
> +			ofono_modem_dec_emergency_mode(modem);
>  		return;
>  	}
>  
> @@ -2237,6 +2285,53 @@ static void dial_request_cb(const struct ofono_error *error, void *data)
>  		voicecalls_emit_call_added(vc, v);
>  }
>  
> +static void modem_online_watch(ofono_bool_t online, void *data)
> +{
> +	struct ofono_voicecall *vc = data;
> +	struct ofono_modem *modem = __ofono_atom_get_modem(vc->atom);
> +	DBusMessage *reply;
> +	const char *number;
> +	struct ofono_phone_number ph;
> +	const char *clirstr;
> +	enum ofono_clir_option clir;
> +
> +	if (ofono_modem_get_emergency_mode(modem) != TRUE)
> +		return;
> +
> +	if (vc->dial_req)
> +		vc->driver->dial(vc, &vc->dial_req->ph,
> +					OFONO_CLIR_OPTION_DEFAULT,
> +					OFONO_CUG_OPTION_DEFAULT,
> +					dial_request_cb, vc);
> +
> +	if (!vc->pending)

if (vc->pending == NULL) here please

> +		return;
> +
> +	if (strcmp(dbus_message_get_member(vc->pending), "Dial"))
> +		return;
> +
> +	if (dbus_message_get_args(vc->pending, NULL, DBUS_TYPE_STRING, &number,
> +					DBUS_TYPE_STRING, &clirstr,
> +					DBUS_TYPE_INVALID) == FALSE)
> +		return;
> +
> +	if (!emergency_number(vc, number))

Please do emergency_number() == FALSE here

> +		return;
> +
> +	if (!online) {

And online == FALSE here

> +		reply = __ofono_error_failed(vc->pending);
> +		__ofono_dbus_pending_reply(&vc->pending, reply);
> +		ofono_modem_dec_emergency_mode(modem);
> +		return;
> +	}
> +
> +	clir_string_to_clir(clirstr, &clir);
> +	string_to_phone_number(number, &ph);
> +
> +	vc->driver->dial(vc, &ph, clir, OFONO_CUG_OPTION_DEFAULT,
> +				manager_dial_callback, vc);
> +}
> +
>  void ofono_voicecall_register(struct ofono_voicecall *vc)
>  {
>  	DBusConnection *conn = ofono_dbus_get_connection();
> @@ -2255,6 +2350,9 @@ void ofono_voicecall_register(struct ofono_voicecall *vc)
>  	}
>  
>  	ofono_modem_add_interface(modem, OFONO_VOICECALL_MANAGER_INTERFACE);
> +	vc->modem_online_watch = __ofono_modem_add_online_watch(modem,
> +							modem_online_watch,
> +							vc, NULL);
>  
>  	/*
>  	 * Start out with the 22.101 mandated numbers, if we have a SIM and
> @@ -2331,6 +2429,17 @@ ofono_bool_t __ofono_voicecall_is_busy(struct ofono_voicecall *vc,
>  
>  static void dial_request(struct ofono_voicecall *vc)
>  {
> +	struct ofono_modem *modem = __ofono_atom_get_modem(vc->atom);
> +
> +	if (emergency_number(vc, phone_number_to_string(&vc->dial_req->ph))) {
> +		ofono_bool_t online = ofono_modem_get_online(modem);
> +
> +		ofono_modem_inc_emergency_mode(modem);
> +
> +		if (!online)

And online == FALSE here

> +			return;
> +	}
> +
>  	vc->driver->dial(vc, &vc->dial_req->ph, OFONO_CLIR_OPTION_DEFAULT,
>  				OFONO_CUG_OPTION_DEFAULT, dial_request_cb, vc);
>  }

Otherwise, looks good to me.

Have you figured out how to support the E911 call-back requirements?

Regards,
-Denis

Re: [RFC PATCH 4/4] voicecall: add emergency call handling

[Andras Domokos]

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

Hi Denis,

On 11/23/2010 11:00 AM, ext Denis Kenzior wrote:
> Hi Andras,
>
> On 11/15/2010 10:58 AM, Andras Domokos wrote:
>    
>> ---
>>   src/voicecall.c |  111 ++++++++++++++++++++++++++++++++++++++++++++++++++++++-
>>   1 files changed, 110 insertions(+), 1 deletions(-)
>>
>> diff --git a/src/voicecall.c b/src/voicecall.c
>> index 3af614b..066cdb9 100644
>> --- a/src/voicecall.c
>> +++ b/src/voicecall.c
>> @@ -52,6 +52,7 @@ struct ofono_voicecall {
>>        struct ofono_sim *sim;
>>        unsigned int sim_watch;
>>        unsigned int sim_state_watch;
>> +     unsigned int modem_online_watch;
>>        const struct ofono_voicecall_driver *driver;
>>        void *driver_data;
>>        struct ofono_atom *atom;
>> @@ -133,6 +134,22 @@ static void add_to_en_list(GSList **l, const char **list)
>>                *l = g_slist_prepend(*l, g_strdup(list[i++]));
>>   }
>>
>> +static gint number_compare(gconstpointer a, gconstpointer b)
>> +{
>> +     const char *s1 = a, *s2 = b;
>> +     return strcmp(s1, s2);
>> +}
>> +
>> +static ofono_bool_t emergency_number(struct ofono_voicecall *vc,
>> +                                     const char *number)
>> +{
>> +     if (!number)
>>      
> Just nit picking here, but in general we really prefer this to be
> written like this:
>
> if (number == NULL)
>
> This is much easier to read when you don't know if number is a string or
> an integer.  Yes I know we're not always consistent about doing this,
> particularly in voicecall.c.
>
>    
>> +             return FALSE;
>> +
>> +     return g_slist_find_custom(vc->en_list,
>> +                             number, number_compare) ? TRUE : FALSE;
>> +}
>> +
>>   static const char *disconnect_reason_to_string(enum ofono_disconnect_reason r)
>>   {
>>        switch (r) {
>> @@ -1125,6 +1142,7 @@ static struct voicecall *dial_handle_result(struct ofono_voicecall *vc,
>>   static void manager_dial_callback(const struct ofono_error *error, void *data)
>>   {
>>        struct ofono_voicecall *vc = data;
>> +     struct ofono_modem *modem = __ofono_atom_get_modem(vc->atom);
>>        DBusMessage *reply;
>>        const char *number;
>>        gboolean need_to_emit;
>> @@ -1143,8 +1161,12 @@ static void manager_dial_callback(const struct ofono_error *error, void *data)
>>
>>                dbus_message_append_args(reply, DBUS_TYPE_OBJECT_PATH,&path,
>>                                                DBUS_TYPE_INVALID);
>> -     } else
>> +     } else {
>> +             if (emergency_number(vc, number))
>> +                     ofono_modem_dec_emergency_mode(modem);
>> +
>>                reply = __ofono_error_failed(vc->pending);
>> +     }
>>
>>        __ofono_dbus_pending_reply(&vc->pending, reply);
>>
>> @@ -1156,6 +1178,7 @@ static DBusMessage *manager_dial(DBusConnection *conn,
>>                                        DBusMessage *msg, void *data)
>>   {
>>        struct ofono_voicecall *vc = data;
>> +     struct ofono_modem *modem = __ofono_atom_get_modem(vc->atom);
>>        const char *number;
>>        struct ofono_phone_number ph;
>>        const char *clirstr;
>> @@ -1195,6 +1218,15 @@ static DBusMessage *manager_dial(DBusConnection *conn,
>>
>>        string_to_phone_number(number,&ph);
>>
>> +     if (emergency_number(vc, number)) {
>> +             ofono_bool_t online = ofono_modem_get_online(modem);
>> +
>> +             ofono_modem_inc_emergency_mode(modem);
>> +
>> +             if (!online)
>>      
> Do me a favor and change this to:
> if (online == FALSE)
>
>    
>> +                     return NULL;
>> +     }
>> +
>>        vc->driver->dial(vc,&ph, clir, OFONO_CUG_OPTION_DEFAULT,
>>                                manager_dial_callback, vc);
>>
>> @@ -1748,6 +1780,7 @@ void ofono_voicecall_disconnected(struct ofono_voicecall *vc, int id,
>>                                const struct ofono_error *error)
>>   {
>>        struct ofono_modem *modem = __ofono_atom_get_modem(vc->atom);
>> +     const char *number;
>>        GSList *l;
>>        struct voicecall *call;
>>        time_t ts;
>> @@ -1767,6 +1800,7 @@ void ofono_voicecall_disconnected(struct ofono_voicecall *vc, int id,
>>        }
>>
>>        call = l->data;
>> +     number = phone_number_to_string(&call->call->phone_number);
>>
>>        ts = time(NULL);
>>        prev_status = call->call->status;
>> @@ -1805,6 +1839,9 @@ void ofono_voicecall_disconnected(struct ofono_voicecall *vc, int id,
>>
>>        voicecalls_emit_call_removed(vc, call);
>>
>> +     if (emergency_number(vc, number))
>> +             ofono_modem_dec_emergency_mode(modem);
>> +
>>        voicecall_dbus_unregister(vc, call);
>>
>>        vc->call_list = g_slist_remove(vc->call_list, call);
>> @@ -2067,6 +2104,7 @@ static void voicecall_unregister(struct ofono_atom *atom)
>>   static void voicecall_remove(struct ofono_atom *atom)
>>   {
>>        struct ofono_voicecall *vc = __ofono_atom_get_data(atom);
>> +     struct ofono_modem *modem = __ofono_atom_get_modem(atom);
>>
>>        DBG("atom: %p", atom);
>>
>> @@ -2108,6 +2146,12 @@ static void voicecall_remove(struct ofono_atom *atom)
>>                g_queue_free(vc->toneq);
>>        }
>>
>> +     if (vc->modem_online_watch) {
>> +             __ofono_modem_remove_online_watch(modem,
>> +                                             vc->modem_online_watch);
>> +             vc->modem_online_watch = 0;
>> +     }
>> +
>>        g_free(vc);
>>   }
>>
>> @@ -2205,6 +2249,7 @@ static void sim_watch(struct ofono_atom *atom,
>>   static void dial_request_cb(const struct ofono_error *error, void *data)
>>   {
>>        struct ofono_voicecall *vc = data;
>> +     struct ofono_modem *modem = __ofono_atom_get_modem(vc->atom);
>>        gboolean need_to_emit;
>>        struct voicecall *v;
>>
>> @@ -2214,6 +2259,9 @@ static void dial_request_cb(const struct ofono_error *error, void *data)
>>
>>        if (v == NULL) {
>>                dial_request_finish(vc);
>>      
> Please add an empty line here based on item M1.
>
>    
>> +             if (emergency_number(vc,
>> +                             phone_number_to_string(&vc->dial_req->ph)))
>> +                     ofono_modem_dec_emergency_mode(modem);
>>                return;
>>        }
>>
>> @@ -2237,6 +2285,53 @@ static void dial_request_cb(const struct ofono_error *error, void *data)
>>                voicecalls_emit_call_added(vc, v);
>>   }
>>
>> +static void modem_online_watch(ofono_bool_t online, void *data)
>> +{
>> +     struct ofono_voicecall *vc = data;
>> +     struct ofono_modem *modem = __ofono_atom_get_modem(vc->atom);
>> +     DBusMessage *reply;
>> +     const char *number;
>> +     struct ofono_phone_number ph;
>> +     const char *clirstr;
>> +     enum ofono_clir_option clir;
>> +
>> +     if (ofono_modem_get_emergency_mode(modem) != TRUE)
>> +             return;
>> +
>> +     if (vc->dial_req)
>> +             vc->driver->dial(vc,&vc->dial_req->ph,
>> +                                     OFONO_CLIR_OPTION_DEFAULT,
>> +                                     OFONO_CUG_OPTION_DEFAULT,
>> +                                     dial_request_cb, vc);
>> +
>> +     if (!vc->pending)
>>      
> if (vc->pending == NULL) here please
>
>    
>> +             return;
>> +
>> +     if (strcmp(dbus_message_get_member(vc->pending), "Dial"))
>> +             return;
>> +
>> +     if (dbus_message_get_args(vc->pending, NULL, DBUS_TYPE_STRING,&number,
>> +                                     DBUS_TYPE_STRING,&clirstr,
>> +                                     DBUS_TYPE_INVALID) == FALSE)
>> +             return;
>> +
>> +     if (!emergency_number(vc, number))
>>      
> Please do emergency_number() == FALSE here
>
>    
>> +             return;
>> +
>> +     if (!online) {
>>      
> And online == FALSE here
>
>    
>> +             reply = __ofono_error_failed(vc->pending);
>> +             __ofono_dbus_pending_reply(&vc->pending, reply);
>> +             ofono_modem_dec_emergency_mode(modem);
>> +             return;
>> +     }
>> +
>> +     clir_string_to_clir(clirstr,&clir);
>> +     string_to_phone_number(number,&ph);
>> +
>> +     vc->driver->dial(vc,&ph, clir, OFONO_CUG_OPTION_DEFAULT,
>> +                             manager_dial_callback, vc);
>> +}
>> +
>>   void ofono_voicecall_register(struct ofono_voicecall *vc)
>>   {
>>        DBusConnection *conn = ofono_dbus_get_connection();
>> @@ -2255,6 +2350,9 @@ void ofono_voicecall_register(struct ofono_voicecall *vc)
>>        }
>>
>>        ofono_modem_add_interface(modem, OFONO_VOICECALL_MANAGER_INTERFACE);
>> +     vc->modem_online_watch = __ofono_modem_add_online_watch(modem,
>> +                                                     modem_online_watch,
>> +                                                     vc, NULL);
>>
>>        /*
>>         * Start out with the 22.101 mandated numbers, if we have a SIM and
>> @@ -2331,6 +2429,17 @@ ofono_bool_t __ofono_voicecall_is_busy(struct ofono_voicecall *vc,
>>
>>   static void dial_request(struct ofono_voicecall *vc)
>>   {
>> +     struct ofono_modem *modem = __ofono_atom_get_modem(vc->atom);
>> +
>> +     if (emergency_number(vc, phone_number_to_string(&vc->dial_req->ph))) {
>> +             ofono_bool_t online = ofono_modem_get_online(modem);
>> +
>> +             ofono_modem_inc_emergency_mode(modem);
>> +
>> +             if (!online)
>>      
> And online == FALSE here
>
>    
>> +                     return;
>> +     }
>> +
>>        vc->driver->dial(vc,&vc->dial_req->ph, OFONO_CLIR_OPTION_DEFAULT,
>>                                OFONO_CUG_OPTION_DEFAULT, dial_request_cb, vc);
>>   }
>>      
> Otherwise, looks good to me.
>    
Thanks for the comments, I am going to resend the patch with
the changes I made based on your comments.
> Have you figured out how to support the E911 call-back requirements?
>    
This is something that still needs to be figured out and can
be added in a separate patch.

> Regards,
> -Denis
>    
Regards,
Andras

Re: [RFC PATCH 4/4] voicecall: add emergency call handling

[Aki Niemi]

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

Hi Andras,

2010/11/24 Andras Domokos <andras.domokos@nokia.com>:
> Thanks for the comments, I am going to resend the patch with
> the changes I made based on your comments.

Can you resend the entire set? At least I've lost track with where
each individual patch stands...

Cheers,
Aki

[RFC PATCH 4/4] voicecall: add emergency call handling

[Andras Domokos]

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

---
 src/voicecall.c |  112 ++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 files changed, 111 insertions(+), 1 deletions(-)

diff --git a/src/voicecall.c b/src/voicecall.c
index 3307db0..882d94f 100644
--- a/src/voicecall.c
+++ b/src/voicecall.c
@@ -52,6 +52,7 @@ struct ofono_voicecall {
 	struct ofono_sim *sim;
 	unsigned int sim_watch;
 	unsigned int sim_state_watch;
+	unsigned int modem_online_watch;
 	const struct ofono_voicecall_driver *driver;
 	void *driver_data;
 	struct ofono_atom *atom;
@@ -133,6 +134,22 @@ static void add_to_en_list(GSList **l, const char **list)
 		*l = g_slist_prepend(*l, g_strdup(list[i++]));
 }
 
+static gint number_compare(gconstpointer a, gconstpointer b)
+{
+	const char *s1 = a, *s2 = b;
+	return strcmp(s1, s2);
+}
+
+static ofono_bool_t emergency_number(struct ofono_voicecall *vc,
+					const char *number)
+{
+	if (number == NULL)
+		return FALSE;
+
+	return g_slist_find_custom(vc->en_list,
+				number, number_compare) ? TRUE : FALSE;
+}
+
 static const char *disconnect_reason_to_string(enum ofono_disconnect_reason r)
 {
 	switch (r) {
@@ -1125,6 +1142,7 @@ static struct voicecall *dial_handle_result(struct ofono_voicecall *vc,
 static void manager_dial_callback(const struct ofono_error *error, void *data)
 {
 	struct ofono_voicecall *vc = data;
+	struct ofono_modem *modem = __ofono_atom_get_modem(vc->atom);
 	DBusMessage *reply;
 	const char *number;
 	gboolean need_to_emit;
@@ -1143,8 +1161,12 @@ static void manager_dial_callback(const struct ofono_error *error, void *data)
 
 		dbus_message_append_args(reply, DBUS_TYPE_OBJECT_PATH, &path,
 						DBUS_TYPE_INVALID);
-	} else
+	} else {
+		if (emergency_number(vc, number))
+			ofono_modem_dec_emergency(modem);
+
 		reply = __ofono_error_failed(vc->pending);
+	}
 
 	__ofono_dbus_pending_reply(&vc->pending, reply);
 
@@ -1156,6 +1178,7 @@ static DBusMessage *manager_dial(DBusConnection *conn,
 					DBusMessage *msg, void *data)
 {
 	struct ofono_voicecall *vc = data;
+	struct ofono_modem *modem = __ofono_atom_get_modem(vc->atom);
 	const char *number;
 	struct ofono_phone_number ph;
 	const char *clirstr;
@@ -1195,6 +1218,15 @@ static DBusMessage *manager_dial(DBusConnection *conn,
 
 	string_to_phone_number(number, &ph);
 
+	if (emergency_number(vc, number)) {
+		ofono_bool_t online = ofono_modem_get_online(modem);
+
+		ofono_modem_inc_emergency(modem);
+
+		if (online == FALSE)
+			return NULL;
+	}
+
 	vc->driver->dial(vc, &ph, clir, OFONO_CUG_OPTION_DEFAULT,
 				manager_dial_callback, vc);
 
@@ -1748,6 +1780,7 @@ void ofono_voicecall_disconnected(struct ofono_voicecall *vc, int id,
 				const struct ofono_error *error)
 {
 	struct ofono_modem *modem = __ofono_atom_get_modem(vc->atom);
+	const char *number;
 	GSList *l;
 	struct voicecall *call;
 	time_t ts;
@@ -1767,6 +1800,7 @@ void ofono_voicecall_disconnected(struct ofono_voicecall *vc, int id,
 	}
 
 	call = l->data;
+	number = phone_number_to_string(&call->call->phone_number);
 
 	ts = time(NULL);
 	prev_status = call->call->status;
@@ -1805,6 +1839,9 @@ void ofono_voicecall_disconnected(struct ofono_voicecall *vc, int id,
 
 	voicecalls_emit_call_removed(vc, call);
 
+	if (emergency_number(vc, number))
+		ofono_modem_dec_emergency(modem);
+
 	voicecall_dbus_unregister(vc, call);
 
 	vc->call_list = g_slist_remove(vc->call_list, call);
@@ -2067,6 +2104,7 @@ static void voicecall_unregister(struct ofono_atom *atom)
 static void voicecall_remove(struct ofono_atom *atom)
 {
 	struct ofono_voicecall *vc = __ofono_atom_get_data(atom);
+	struct ofono_modem *modem = __ofono_atom_get_modem(atom);
 
 	DBG("atom: %p", atom);
 
@@ -2108,6 +2146,12 @@ static void voicecall_remove(struct ofono_atom *atom)
 		g_queue_free(vc->toneq);
 	}
 
+	if (vc->modem_online_watch) {
+		__ofono_modem_remove_online_watch(modem,
+						vc->modem_online_watch);
+		vc->modem_online_watch = 0;
+	}
+
 	g_free(vc);
 }
 
@@ -2205,6 +2249,7 @@ static void sim_watch(struct ofono_atom *atom,
 static void dial_request_cb(const struct ofono_error *error, void *data)
 {
 	struct ofono_voicecall *vc = data;
+	struct ofono_modem *modem = __ofono_atom_get_modem(vc->atom);
 	gboolean need_to_emit;
 	struct voicecall *v;
 
@@ -2214,6 +2259,10 @@ static void dial_request_cb(const struct ofono_error *error, void *data)
 
 	if (v == NULL) {
 		dial_request_finish(vc);
+
+		if (emergency_number(vc,
+				phone_number_to_string(&vc->dial_req->ph)))
+			ofono_modem_dec_emergency(modem);
 		return;
 	}
 
@@ -2237,6 +2286,53 @@ static void dial_request_cb(const struct ofono_error *error, void *data)
 		voicecalls_emit_call_added(vc, v);
 }
 
+static void modem_online_watch(ofono_bool_t online, void *data)
+{
+	struct ofono_voicecall *vc = data;
+	struct ofono_modem *modem = __ofono_atom_get_modem(vc->atom);
+	DBusMessage *reply;
+	const char *number;
+	struct ofono_phone_number ph;
+	const char *clirstr;
+	enum ofono_clir_option clir;
+
+	if (ofono_modem_get_emergency(modem) != TRUE)
+		return;
+
+	if (vc->dial_req)
+		vc->driver->dial(vc, &vc->dial_req->ph,
+					OFONO_CLIR_OPTION_DEFAULT,
+					OFONO_CUG_OPTION_DEFAULT,
+					dial_request_cb, vc);
+
+	if (vc->pending == NULL)
+		return;
+
+	if (strcmp(dbus_message_get_member(vc->pending), "Dial"))
+		return;
+
+	if (dbus_message_get_args(vc->pending, NULL, DBUS_TYPE_STRING, &number,
+					DBUS_TYPE_STRING, &clirstr,
+					DBUS_TYPE_INVALID) == FALSE)
+		return;
+
+	if (emergency_number(vc, number) == FALSE)
+		return;
+
+	if (online == FALSE) {
+		reply = __ofono_error_failed(vc->pending);
+		__ofono_dbus_pending_reply(&vc->pending, reply);
+		ofono_modem_dec_emergency(modem);
+		return;
+	}
+
+	clir_string_to_clir(clirstr, &clir);
+	string_to_phone_number(number, &ph);
+
+	vc->driver->dial(vc, &ph, clir, OFONO_CUG_OPTION_DEFAULT,
+				manager_dial_callback, vc);
+}
+
 void ofono_voicecall_register(struct ofono_voicecall *vc)
 {
 	DBusConnection *conn = ofono_dbus_get_connection();
@@ -2255,6 +2351,9 @@ void ofono_voicecall_register(struct ofono_voicecall *vc)
 	}
 
 	ofono_modem_add_interface(modem, OFONO_VOICECALL_MANAGER_INTERFACE);
+	vc->modem_online_watch = __ofono_modem_add_online_watch(modem,
+							modem_online_watch,
+							vc, NULL);
 
 	/*
 	 * Start out with the 22.101 mandated numbers, if we have a SIM and
@@ -2331,6 +2430,17 @@ ofono_bool_t __ofono_voicecall_is_busy(struct ofono_voicecall *vc,
 
 static void dial_request(struct ofono_voicecall *vc)
 {
+	struct ofono_modem *modem = __ofono_atom_get_modem(vc->atom);
+
+	if (emergency_number(vc, phone_number_to_string(&vc->dial_req->ph))) {
+		ofono_bool_t online = ofono_modem_get_online(modem);
+
+		ofono_modem_inc_emergency(modem);
+
+		if (online == FALSE)
+			return;
+	}
+
 	vc->driver->dial(vc, &vc->dial_req->ph, OFONO_CLIR_OPTION_DEFAULT,
 				OFONO_CUG_OPTION_DEFAULT, dial_request_cb, vc);
 }
-- 
1.7.0.4