Re: [RFC PATCH 2/3] voicecall: emergency call handling added

[Denis Kenzior <denkenz@gmail.com>]

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

Hi Andras,

On 10/25/2010 03:03 AM, Andras Domokos wrote:
> From: Andras Domokos <andras.domokos@nokia.com>
> 
> 
> Signed-off-by: Andras Domokos <andras.domokos@nokia.com>
> ---
>  include/voicecall.h |   12 +++
>  src/voicecall.c     |  221 ++++++++++++++++++++++++++++++++++++++++++++++----
>  2 files changed, 215 insertions(+), 18 deletions(-)
> 
> diff --git a/include/voicecall.h b/include/voicecall.h
> index 2356fcf..d530148 100644
> --- a/include/voicecall.h
> +++ b/include/voicecall.h
> @@ -38,6 +38,9 @@ typedef void (*ofono_call_list_cb_t)(const struct ofono_error *error,
>  					const struct ofono_call *call_list,
>  					void *data);
>  
> +typedef void (*ofono_voicecall_emergency_notify_cb_t)(ofono_bool_t state,
> +							void *data);
> +
>  /* Voice call related functionality, including ATD, ATA, +CHLD, CTFR, CLCC
>   * and VTS.
>   *
> @@ -116,6 +119,15 @@ void ofono_voicecall_set_data(struct ofono_voicecall *vc, void *data);
>  void *ofono_voicecall_get_data(struct ofono_voicecall *vc);
>  int ofono_voicecall_get_next_callid(struct ofono_voicecall *vc);
>  
> +unsigned int __ofono_voicecall_add_emergency_watch(struct ofono_voicecall *vc,
> +				ofono_voicecall_emergency_notify_cb_t notify,
> +				void *data, ofono_destroy_func destroy);
> +void __ofono_voicecall_remove_emergency_watch(struct ofono_voicecall *vc,
> +						unsigned int id);
> +ofono_bool_t ofono_voicecall_get_emergency_state(struct ofono_voicecall *vc);
> +void __ofono_voicecall_set_emergency_state(struct ofono_voicecall *vc,
> +						int state);
> +

Just some general comments, but this patch seems to be backwards from
the earlier proposal.  Namely EmergencyMode is a property on the Modem
interface, not on the VoiceCallManager.  See doc/modem-api.txt,
Emergency property.

In general I think that the emergency_watch is unnecessary.  Having a
reference counted emergency tracking inside the modem object and a modem
online state watch should be sufficient.

>  #ifdef __cplusplus
>  }
>  #endif
> diff --git a/src/voicecall.c b/src/voicecall.c
> index 7b5fe3b..a619b30 100644
> --- a/src/voicecall.c
> +++ b/src/voicecall.c
> @@ -25,6 +25,7 @@
>  
>  #include <string.h>
>  #include <stdio.h>
> +#include <stdlib.h>
>  #include <time.h>
>  #include <errno.h>
>  #include <stdint.h>
> @@ -56,6 +57,9 @@ struct ofono_voicecall {
>  	void *driver_data;
>  	struct ofono_atom *atom;
>  	struct dial_request *dial_req;
> +	struct ofono_watchlist *emergency_watches;
> +	unsigned int emergency_state;
> +	unsigned int modem_state_watch;
>  };
>  
>  struct voicecall {
> @@ -85,6 +89,8 @@ static const char *default_en_list_no_sim[] = { "119", "118", "999", "110",
>  
>  static void generic_callback(const struct ofono_error *error, void *data);
>  static void multirelease_callback(const struct ofono_error *err, void *data);
> +static const char *voicecall_build_path(struct ofono_voicecall *vc,
> +					const struct ofono_call *call);
>  

It is generally against the coding style to forward-declare static
functions.  If this function is needed, please simply move it higher.

>  static gint call_compare_by_id(gconstpointer a, gconstpointer b)
>  {
> @@ -121,6 +127,145 @@ 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;
> +}
> +

Please add this as a separate patch handling the 'Extend the voicecall
interface with a property indicating whether this call is an emergency
call' task.

> +static void notify_emergency_watches(struct ofono_voicecall *vc,
> +					ofono_bool_t state)
> +{
> +	struct ofono_watchlist_item *item;
> +	GSList *l;
> +	ofono_voicecall_emergency_notify_cb_t notify;
> +
> +	if (vc->emergency_watches == NULL)
> +		return;
> +
> +	for (l = vc->emergency_watches->items; l; l = l->next) {
> +		item = l->data;
> +		notify = item->notify;
> +		notify(state, item->notify_data);
> +	}
> +}
> +
> +unsigned int __ofono_voicecall_add_emergency_watch(struct ofono_voicecall *vc,
> +				ofono_voicecall_emergency_notify_cb_t notify,
> +				void *data, ofono_destroy_func destroy)
> +{
> +	struct ofono_watchlist_item *item;
> +
> +	if (vc == 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(vc->emergency_watches, item);
> +}
> +
> +void __ofono_voicecall_remove_emergency_watch(struct ofono_voicecall *vc,
> +						unsigned int id)
> +{
> +	__ofono_watchlist_remove_item(vc->emergency_watches, id);
> +}
> +
> +ofono_bool_t ofono_voicecall_get_emergency_state(struct ofono_voicecall *vc)
> +{
> +	return vc->emergency_state ? 1 : 0;
> +}
> +
> +void __ofono_voicecall_inc_emergency_state(struct ofono_voicecall *vc)
> +{
> +	DBusConnection *conn = ofono_dbus_get_connection();
> +	const char *path = __ofono_atom_get_path(vc->atom);
> +	gboolean state = TRUE;
> +
> +	if (!vc->emergency_state++) {

Please avoid such coding style.  Parsing such a statement takes
considerable effort.

vc->emergency_state++;

if (vc->emergency_state > 1)
	return;

ofono_dbus_signal...

would be much more readable.

> +		ofono_dbus_signal_property_changed(conn, path,
> +						OFONO_VOICECALL_INTERFACE,
> +						"EmergencyMode",
> +						DBUS_TYPE_BOOLEAN,
> +						&state);
> +		notify_emergency_watches(vc, state);
> +	}
> +}
> +
> +void __ofono_voicecall_dec_emergency_state(struct ofono_voicecall *vc)
> +{
> +	DBusConnection *conn = ofono_dbus_get_connection();
> +	const char *path = __ofono_atom_get_path(vc->atom);
> +	gboolean state = FALSE;
> +
> +	if (!--vc->emergency_state) {
> +		ofono_dbus_signal_property_changed(conn, path,
> +						OFONO_VOICECALL_INTERFACE,
> +						"EmergencyMode",
> +						DBUS_TYPE_BOOLEAN,
> +						&state);
> +		notify_emergency_watches(vc, state);
> +	}
> +}
> +
> +static ofono_bool_t clir_string_to_clir(const char *clirstr,
> +					enum ofono_clir_option *clir);
> +static void manager_dial_callback(const struct ofono_error *error, void *data);

Again, please avoid forward declarations

> +static void modem_state_watch(enum ofono_modem_state state, 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 (!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) {
> +		reply = __ofono_error_invalid_args(vc->pending);
> +		__ofono_dbus_pending_reply(&vc->pending, reply);
> +		return;
> +	}
> +
> +	/* don't care here about non-emergency calls */
> +	if (!emergency_number(vc, number))
> +		return;
> +
> +	if (!ofono_modem_get_online(modem)) {
> +		reply = __ofono_error_failed(vc->pending);
> +		__ofono_dbus_pending_reply(&vc->pending, reply);
> +		__ofono_voicecall_dec_emergency_state(vc);
> +		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);
> +}
> +
>  static const char *disconnect_reason_to_string(enum ofono_disconnect_reason r)
>  {
>  	switch (r) {
> @@ -718,7 +863,8 @@ static gboolean voicecalls_can_dtmf(struct ofono_voicecall *vc)
>  	return FALSE;
>  }
>  
> -static gboolean voicecalls_have_with_status(struct ofono_voicecall *vc, int status)
> +static gboolean voicecalls_have_with_status(struct ofono_voicecall *vc,
> +						int status)

Why is this here? This chunk does not seem related to this patch

>  {
>  	GSList *l;
>  	struct voicecall *v;
> @@ -918,6 +1064,7 @@ static DBusMessage *manager_get_properties(DBusConnection *conn,
>  	int i;
>  	GSList *l;
>  	char **list;
> +	ofono_bool_t emergency_state;
>  
>  	reply = dbus_message_new_method_return(msg);
>  
> @@ -930,6 +1077,10 @@ static DBusMessage *manager_get_properties(DBusConnection *conn,
>  					OFONO_PROPERTIES_ARRAY_SIGNATURE,
>  					&dict);
>  
> +	emergency_state = ofono_voicecall_get_emergency_state(vc);
> +	ofono_dbus_dict_append(&dict, "EmergencyMode",
> +				DBUS_TYPE_BOOLEAN, &emergency_state);
> +

As mentioned before, Emergency was agreed to be on the Modem interface

>  	/* property EmergencyNumbers */
>  	list = g_new0(char *, g_slist_length(vc->en_list) + 1);
>  
> @@ -1066,8 +1217,11 @@ 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_voicecall_dec_emergency_state(vc);
>  		reply = __ofono_error_failed(vc->pending);
> +	}
>  
>  	__ofono_dbus_pending_reply(&vc->pending, reply);
>  
> @@ -1118,6 +1272,14 @@ static DBusMessage *manager_dial(DBusConnection *conn,
>  
>  	string_to_phone_number(number, &ph);
>  
> +	if (emergency_number(vc, number)) {
> +		struct ofono_modem *modem = __ofono_atom_get_modem(vc->atom);
> +		ofono_bool_t online = ofono_modem_get_online(modem);
> +		__ofono_voicecall_inc_emergency_state(vc);
> +		if (!online)
> +			return NULL;
> +	}
> +
>  	vc->driver->dial(vc, &ph, clir, OFONO_CUG_OPTION_DEFAULT,
>  				manager_dial_callback, vc);
>  
> @@ -1667,7 +1829,7 @@ void ofono_voicecall_disconnected(struct ofono_voicecall *vc, int id,
>  {
>  	struct ofono_modem *modem = __ofono_atom_get_modem(vc->atom);
>  	GSList *l;
> -	struct voicecall *call;
> +	struct voicecall *v;

Why?  Please refrain from doing this.  If you feel the variable naming
could be improved, then send a separate patch.  As it is, it has no
bearing on emergency call handling...

>  	time_t ts;
>  	enum call_status prev_status;
>  
> @@ -1684,48 +1846,52 @@ void ofono_voicecall_disconnected(struct ofono_voicecall *vc, int id,
>  		return;
>  	}
>  
> -	call = l->data;
> +	v = l->data;
>  
>  	ts = time(NULL);
> -	prev_status = call->call->status;
> +	prev_status = v->call->status;
>  
>  	l = g_slist_find_custom(vc->multiparty_list, GUINT_TO_POINTER(id),
>  				call_compare_by_id);
>  
>  	if (l) {
>  		vc->multiparty_list =
> -			g_slist_remove(vc->multiparty_list, call);
> +			g_slist_remove(vc->multiparty_list, v);
>  
>  		if (vc->multiparty_list->next == NULL) { /* Size == 1 */
> -			struct voicecall *v = vc->multiparty_list->data;
> +			struct voicecall *v2 = vc->multiparty_list->data;
>  
> -			voicecall_emit_multiparty(v, FALSE);
> +			voicecall_emit_multiparty(v2, FALSE);
>  			g_slist_free(vc->multiparty_list);
>  			vc->multiparty_list = 0;
>  		}
>  	}
>  
> -	vc->release_list = g_slist_remove(vc->release_list, call);
> +	vc->release_list = g_slist_remove(vc->release_list, v);
>  
>  	if (reason != OFONO_DISCONNECT_REASON_UNKNOWN)
> -		voicecall_emit_disconnect_reason(call, reason);
> +		voicecall_emit_disconnect_reason(v, reason);
>  
> -	voicecall_set_call_status(call, CALL_STATUS_DISCONNECTED);
> +	voicecall_set_call_status(v, CALL_STATUS_DISCONNECTED);
>  
> -	if (!call->untracked) {
> +	if (!v->untracked) {
>  		if (prev_status == CALL_STATUS_INCOMING ||
>  				prev_status == CALL_STATUS_WAITING)
> -			__ofono_history_call_missed(modem, call->call, ts);
> +			__ofono_history_call_missed(modem, v->call, ts);
>  		else
> -			__ofono_history_call_ended(modem, call->call,
> -							call->detect_time, ts);
> +			__ofono_history_call_ended(modem, v->call,
> +							v->detect_time, ts);
>  	}
>  
> -	voicecalls_emit_call_removed(vc, call);
> +	voicecalls_emit_call_removed(vc, v);
> +
> +	if (emergency_number(vc,
> +			phone_number_to_string(&v->call->phone_number)))
> +		__ofono_voicecall_dec_emergency_state(vc);
>  
> -	voicecall_dbus_unregister(vc, call);
> +	voicecall_dbus_unregister(vc, v);
>  
> -	vc->call_list = g_slist_remove(vc->call_list, call);
> +	vc->call_list = g_slist_remove(vc->call_list, v);
>  }
>  
>  void ofono_voicecall_notify(struct ofono_voicecall *vc,
> @@ -1969,6 +2135,9 @@ static void voicecall_unregister(struct ofono_atom *atom)
>  		vc->sim_watch = 0;
>  	}
>  
> +	__ofono_watchlist_free(vc->emergency_watches);
> +	vc->emergency_watches = NULL;
> +
>  	if (vc->dial_req)
>  		dial_request_finish(vc);
>  
> @@ -1985,6 +2154,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);
>  
> @@ -2012,6 +2182,11 @@ static void voicecall_remove(struct ofono_atom *atom)
>  		vc->sim = NULL;
>  	}
>  
> +	if (vc->modem_state_watch) {
> +		__ofono_modem_remove_state_watch(modem, vc->modem_state_watch);
> +		vc->modem_state_watch = 0;
> +	}
> +
>  	g_free(vc);
>  }
>  
> @@ -2122,6 +2297,10 @@ void ofono_voicecall_register(struct ofono_voicecall *vc)
>  	}
>  
>  	ofono_modem_add_interface(modem, OFONO_VOICECALL_MANAGER_INTERFACE);
> +	vc->emergency_watches = __ofono_watchlist_new(g_free);
> +	vc->modem_state_watch = __ofono_modem_add_state_watch(modem,
> +							modem_state_watch,
> +							vc, NULL);
>  
>  	/*
>  	 * Start out with the 22.101 mandated numbers, if we have a SIM and
> @@ -2208,6 +2387,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_voicecall_inc_emergency_state(vc);
>  		return;
>  	}
>  
> @@ -2233,6 +2415,9 @@ static void dial_request_cb(const struct ofono_error *error, void *data)
>  
>  static void dial_request(struct ofono_voicecall *vc)
>  {
> +	if (emergency_number(vc, phone_number_to_string(&vc->dial_req->ph)))
> +		__ofono_voicecall_inc_emergency_state(vc);
> +
>  	vc->driver->dial(vc, &vc->dial_req->ph, OFONO_CLIR_OPTION_DEFAULT,
>  				OFONO_CUG_OPTION_DEFAULT, dial_request_cb, vc);
>  }

Regards,
-Denis