[PATCHv2 0/8] Call forwarding state handling change

[PATCHv2 0/8] Call forwarding state handling change

[Oleg Zhurakivskyy]

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

Hello,

Please find the changes in order to correct call forwarding states.

Changes from v2:

- Don't run conditional queries if cfu is active.
- Clear the conditional cache flag on conditional deactivation
  while cfu is active.

Regards,
Oleg

Oleg Zhurakivskyy (8):
  call-forwarding: Minor style fixes
  call-forwarding: Minor refactoring of is_cfu_enabled()
  call-forwarding: Don't set conditional cfs when cfu is active
  call-forwarding: Don't report conditional cfs when cfu is active
  call-forwarding: Emit signals to mask/unmask conditional cfs
  call-forwarding: Don't run conditional queries if cfu is active
  call-forwarding: Clear the conditional cache flag
  TODO: Remove completed call forwarding state task

 TODO                  |   17 -----
 src/call-forwarding.c |  182 ++++++++++++++++++++++++++++++++++--------------
 2 files changed, 129 insertions(+), 70 deletions(-)

-- 
1.7.5.4

[PATCHv2 1/8] call-forwarding: Minor style fixes

[Oleg Zhurakivskyy]

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

---
 src/call-forwarding.c |   56 ++++++++++++++++++++++++++----------------------
 1 files changed, 30 insertions(+), 26 deletions(-)

diff --git a/src/call-forwarding.c b/src/call-forwarding.c
index a58ca21..731a38a 100644
--- a/src/call-forwarding.c
+++ b/src/call-forwarding.c
@@ -38,21 +38,19 @@
 
 #define uninitialized_var(x) x = x
 
-#define CALL_FORWARDING_FLAG_CACHED 0x1
-#define CALL_FORWARDING_FLAG_CPHS_CFF 0x2
+#define CALL_FORWARDING_FLAG_CACHED	0x1
+#define CALL_FORWARDING_FLAG_CPHS_CFF	0x2
 
 /* According to 27.007 Spec */
 #define DEFAULT_NO_REPLY_TIMEOUT 20
 
-static GSList *g_drivers = NULL;
-
 enum call_forwarding_type {
-	CALL_FORWARDING_TYPE_UNCONDITIONAL =		0,
-	CALL_FORWARDING_TYPE_BUSY =			1,
-	CALL_FORWARDING_TYPE_NO_REPLY =			2,
-	CALL_FORWARDING_TYPE_NOT_REACHABLE =		3,
-	CALL_FORWARDING_TYPE_ALL =			4,
-	CALL_FORWARDING_TYPE_ALL_CONDITIONAL =		5
+	CALL_FORWARDING_TYPE_UNCONDITIONAL =	0,
+	CALL_FORWARDING_TYPE_BUSY =		1,
+	CALL_FORWARDING_TYPE_NO_REPLY =		2,
+	CALL_FORWARDING_TYPE_NOT_REACHABLE =	3,
+	CALL_FORWARDING_TYPE_ALL =		4,
+	CALL_FORWARDING_TYPE_ALL_CONDITIONAL =	5
 };
 
 struct ofono_call_forwarding {
@@ -72,10 +70,6 @@ struct ofono_call_forwarding {
 	struct ofono_atom *atom;
 };
 
-static void get_query_next_cf_cond(struct ofono_call_forwarding *cf);
-static void set_query_next_cf_cond(struct ofono_call_forwarding *cf);
-static void ss_set_query_next_cf_cond(struct ofono_call_forwarding *cf);
-
 struct cf_ss_request {
 	int ss_type;
 	int cf_type;
@@ -83,6 +77,12 @@ struct cf_ss_request {
 	GSList *cf_list[4];
 };
 
+static GSList *g_drivers = NULL;
+
+static void get_query_next_cf_cond(struct ofono_call_forwarding *cf);
+static void set_query_next_cf_cond(struct ofono_call_forwarding *cf);
+static void ss_set_query_next_cf_cond(struct ofono_call_forwarding *cf);
+
 static gint cf_condition_compare(gconstpointer a, gconstpointer b)
 {
 	const struct ofono_call_forwarding_condition *ca = a;
@@ -163,7 +163,8 @@ static GSList *cf_cond_list_create(int total,
 			if (list[i].status == 0)
 				continue;
 
-			cond = g_try_new0(struct ofono_call_forwarding_condition, 1);
+			cond = g_try_new0(
+				struct ofono_call_forwarding_condition, 1);
 			if (cond == NULL)
 				continue;
 
@@ -497,7 +498,7 @@ static void property_append_cf_conditions(DBusMessageIter *dict,
 }
 
 static DBusMessage *cf_get_properties_reply(DBusMessage *msg,
-						struct ofono_call_forwarding *cf)
+					struct ofono_call_forwarding *cf)
 {
 	DBusMessage *reply;
 	DBusMessageIter iter;
@@ -891,11 +892,11 @@ static DBusMessage *cf_disable_all(DBusConnection *conn, DBusMessage *msg,
 
 static GDBusMethodTable cf_methods[] = {
 	{ "GetProperties",	"",	"a{sv}",	cf_get_properties,
-							G_DBUS_METHOD_FLAG_ASYNC },
+						G_DBUS_METHOD_FLAG_ASYNC },
 	{ "SetProperty",	"sv",	"",		cf_set_property,
-							G_DBUS_METHOD_FLAG_ASYNC },
+						G_DBUS_METHOD_FLAG_ASYNC },
 	{ "DisableAll",		"s",	"",		cf_disable_all,
-							G_DBUS_METHOD_FLAG_ASYNC },
+						G_DBUS_METHOD_FLAG_ASYNC },
 	{ }
 };
 
@@ -1431,7 +1432,8 @@ static void sim_read_cf_indicator(struct ofono_call_forwarding *cf)
 	}
 }
 
-int ofono_call_forwarding_driver_register(const struct ofono_call_forwarding_driver *d)
+int ofono_call_forwarding_driver_register(
+				const struct ofono_call_forwarding_driver *d)
 {
 	DBG("driver: %p, name: %s", d, d->name);
 
@@ -1443,7 +1445,8 @@ int ofono_call_forwarding_driver_register(const struct ofono_call_forwarding_dri
 	return 0;
 }
 
-void ofono_call_forwarding_driver_unregister(const struct ofono_call_forwarding_driver *d)
+void ofono_call_forwarding_driver_unregister(
+				const struct ofono_call_forwarding_driver *d)
 {
 	DBG("driver: %p, name: %s", d, d->name);
 
@@ -1467,10 +1470,10 @@ static void call_forwarding_remove(struct ofono_atom *atom)
 	g_free(cf);
 }
 
-struct ofono_call_forwarding *ofono_call_forwarding_create(struct ofono_modem *modem,
-							unsigned int vendor,
-							const char *driver,
-							void *data)
+struct ofono_call_forwarding *ofono_call_forwarding_create(
+						struct ofono_modem *modem,
+						unsigned int vendor,
+						const char *driver, void *data)
 {
 	struct ofono_call_forwarding *cf;
 	GSList *l;
@@ -1552,7 +1555,8 @@ void ofono_call_forwarding_remove(struct ofono_call_forwarding *cf)
 	__ofono_atom_free(cf->atom);
 }
 
-void ofono_call_forwarding_set_data(struct ofono_call_forwarding *cf, void *data)
+void ofono_call_forwarding_set_data(struct ofono_call_forwarding *cf,
+								void *data)
 {
 	cf->driver_data = data;
 }
-- 
1.7.5.4

[PATCHv2 2/8] call-forwarding: Minor refactoring of is_cfu_enabled()

[Oleg Zhurakivskyy]

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

This also removes the need for uninitialized_var() macro.
---
 src/call-forwarding.c |   36 +++++++++++++++++-------------------
 1 files changed, 17 insertions(+), 19 deletions(-)

diff --git a/src/call-forwarding.c b/src/call-forwarding.c
index 731a38a..8b8d5a8 100644
--- a/src/call-forwarding.c
+++ b/src/call-forwarding.c
@@ -36,14 +36,17 @@
 #include "common.h"
 #include "simutil.h"
 
-#define uninitialized_var(x) x = x
-
 #define CALL_FORWARDING_FLAG_CACHED	0x1
 #define CALL_FORWARDING_FLAG_CPHS_CFF	0x2
 
 /* According to 27.007 Spec */
 #define DEFAULT_NO_REPLY_TIMEOUT 20
 
+#define is_cfu_enabled(_cf)				\
+({							\
+	cf_find_unconditional(_cf) ? TRUE : FALSE;	\
+})
+
 enum call_forwarding_type {
 	CALL_FORWARDING_TYPE_UNCONDITIONAL =	0,
 	CALL_FORWARDING_TYPE_BUSY =		1,
@@ -221,8 +224,8 @@ static void sim_cphs_cff_update_cb(int ok, void *data)
 		ofono_info("Failed to update EFcphs-cff");
 }
 
-static gboolean is_cfu_enabled(struct ofono_call_forwarding *cf,
-				struct ofono_call_forwarding_condition **out)
+static struct ofono_call_forwarding_condition *cf_find_unconditional(
+					struct ofono_call_forwarding *cf)
 {
 	GSList *l = cf->cf_conditions[CALL_FORWARDING_TYPE_UNCONDITIONAL];
 	struct ofono_call_forwarding_condition *cond;
@@ -237,21 +240,16 @@ static gboolean is_cfu_enabled(struct ofono_call_forwarding *cf,
 		if (cond->cls > BEARER_CLASS_VOICE)
 			continue;
 
-		if (out)
-			*out = cond;
-
-		return TRUE;
+		return cond;
 	}
 
-	return FALSE;
+	return NULL;
 }
 
 static void sim_set_cf_indicator(struct ofono_call_forwarding *cf)
 {
-	gboolean cfu_voice;
-	struct ofono_call_forwarding_condition *uninitialized_var(cond);
-
-	cfu_voice = is_cfu_enabled(cf, &cond);
+	struct ofono_call_forwarding_condition *cfu_voice =
+						cf_find_unconditional(cf);
 
 	if (cf->cfis_record_id) {
 		unsigned char data[16];
@@ -263,15 +261,15 @@ static void sim_set_cf_indicator(struct ofono_call_forwarding *cf)
 		data[0] = 0x01;
 
 		if (cfu_voice) {
-			number_len = strlen(cond->phone_number.number);
+			number_len = strlen(cfu_voice->phone_number.number);
 
 			/* CFU indicator Status - Voice */
 			data[1] = 0x01;
 			number_len = (number_len + 1) / 2;
 			data[2] = number_len + 1;
-			data[3] = cond->phone_number.type;
+			data[3] = cfu_voice->phone_number.type;
 
-			sim_encode_bcd_number(cond->phone_number.number,
+			sim_encode_bcd_number(cfu_voice->phone_number.number,
 						data + 4);
 		} else {
 			data[1] = 0x00;
@@ -317,7 +315,7 @@ static void set_new_cond_list(struct ofono_call_forwarding *cf,
 
 	if ((cf->flags & CALL_FORWARDING_FLAG_CPHS_CFF) ||
 			cf->cfis_record_id > 0)
-		old_cfu = is_cfu_enabled(cf, NULL);
+		old_cfu = is_cfu_enabled(cf);
 	else
 		old_cfu = FALSE;
 
@@ -432,7 +430,7 @@ static void set_new_cond_list(struct ofono_call_forwarding *cf,
 
 	if ((cf->flags & CALL_FORWARDING_FLAG_CPHS_CFF) ||
 			cf->cfis_record_id > 0)
-		new_cfu = is_cfu_enabled(cf, NULL);
+		new_cfu = is_cfu_enabled(cf);
 	else
 		new_cfu = FALSE;
 
@@ -523,7 +521,7 @@ static DBusMessage *cf_get_properties_reply(DBusMessage *msg,
 
 	if ((cf->flags & CALL_FORWARDING_FLAG_CPHS_CFF) ||
 			cf->cfis_record_id > 0)
-		status = is_cfu_enabled(cf, NULL);
+		status = is_cfu_enabled(cf);
 	else
 		status = FALSE;
 
-- 
1.7.5.4

[PATCHv2 3/8] call-forwarding: Don't set conditional cfs when cfu is active

[Oleg Zhurakivskyy]

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

---
 src/call-forwarding.c |   18 ++++++++++++++++++
 1 files changed, 18 insertions(+), 0 deletions(-)

diff --git a/src/call-forwarding.c b/src/call-forwarding.c
index 8b8d5a8..3f9d816 100644
--- a/src/call-forwarding.c
+++ b/src/call-forwarding.c
@@ -802,6 +802,13 @@ static DBusMessage *cf_set_property(DBusConnection *conn, DBusMessage *msg,
 		if (strlen(number) > 0 && !valid_phone_number_format(number))
 			return __ofono_error_invalid_format(msg);
 
+		/*
+		 * Don't set conditional cfs when cfu is active
+		 */
+		if (type != CALL_FORWARDING_TYPE_UNCONDITIONAL &&
+				number[0] != '\0' && is_cfu_enabled(cf))
+			return __ofono_error_not_available(msg);
+
 		if (number[0] != '\0')
 			string_to_phone_number(number, &ph);
 
@@ -1159,6 +1166,17 @@ static gboolean cf_ss_control(int type, const char *sc,
 		return TRUE;
 	}
 
+	/*
+	 * Don't set conditional cfs when cfu is active
+	 */
+	if (type == SS_CONTROL_TYPE_REGISTRATION &&
+			cf_type != CALL_FORWARDING_TYPE_UNCONDITIONAL &&
+			sia[0] != '\0' && is_cfu_enabled(cf)) {
+		g_dbus_send_message(conn, __ofono_error_not_available(msg));
+
+		return TRUE;
+	}
+
 	cf->ss_req = g_try_new0(struct cf_ss_request, 1);
 
 	if (cf->ss_req == NULL) {
-- 
1.7.5.4

[PATCHv2 4/8] call-forwarding: Don't report conditional cfs when cfu is active

[Oleg Zhurakivskyy]

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

---
 src/call-forwarding.c |   20 +++++++++++++++++---
 1 files changed, 17 insertions(+), 3 deletions(-)

diff --git a/src/call-forwarding.c b/src/call-forwarding.c
index 3f9d816..f9bee6b 100644
--- a/src/call-forwarding.c
+++ b/src/call-forwarding.c
@@ -503,6 +503,8 @@ static DBusMessage *cf_get_properties_reply(DBusMessage *msg,
 	DBusMessageIter dict;
 	int i;
 	dbus_bool_t status;
+	gboolean cfu_enabled;
+	GSList *cf_list;
 
 	reply = dbus_message_new_method_return(msg);
 	if (reply == NULL)
@@ -514,14 +516,26 @@ static DBusMessage *cf_get_properties_reply(DBusMessage *msg,
 					OFONO_PROPERTIES_ARRAY_SIGNATURE,
 						&dict);
 
-	for (i = 0; i < 4; i++)
-		property_append_cf_conditions(&dict, cf->cf_conditions[i],
+	cfu_enabled = is_cfu_enabled(cf);
+
+	for (i = 0; i < 4; i++) {
+
+		/*
+		 * Report conditional cfs as empty when CFU is active
+		 */
+		if (cfu_enabled && (i != CALL_FORWARDING_TYPE_UNCONDITIONAL))
+			cf_list = NULL;
+		else
+			cf_list = cf->cf_conditions[i];
+
+		property_append_cf_conditions(&dict, cf_list,
 						BEARER_CLASS_VOICE,
 						cf_type_lut[i]);
+	}
 
 	if ((cf->flags & CALL_FORWARDING_FLAG_CPHS_CFF) ||
 			cf->cfis_record_id > 0)
-		status = is_cfu_enabled(cf);
+		status = cfu_enabled;
 	else
 		status = FALSE;
 
-- 
1.7.5.4

[PATCHv2 5/8] call-forwarding: Emit signals to mask/unmask conditional cfs

[Oleg Zhurakivskyy]

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

Emit signals to mask/unmask conditional cfs on cfu
activation/deactivation.
---
 src/call-forwarding.c |   30 ++++++++++++++++++++++++++++++
 1 files changed, 30 insertions(+), 0 deletions(-)

diff --git a/src/call-forwarding.c b/src/call-forwarding.c
index f9bee6b..7b086e0 100644
--- a/src/call-forwarding.c
+++ b/src/call-forwarding.c
@@ -436,6 +436,36 @@ static void set_new_cond_list(struct ofono_call_forwarding *cf,
 
 	if (new_cfu != old_cfu) {
 		ofono_bool_t status = new_cfu;
+		int i;
+
+		/*
+		 * Emit signals to mask/unmask conditional cfs on cfu change
+		 */
+		for (i = 0; i < 4; i++) {
+			if (i == CALL_FORWARDING_TYPE_UNCONDITIONAL)
+				continue;
+
+			l = g_slist_find_custom(cf->cf_conditions[i],
+					GINT_TO_POINTER(BEARER_CLASS_VOICE),
+					cf_condition_find_with_cls);
+
+			if (l == NULL)
+				continue;
+
+			if (new_cfu)
+				number = "";
+			else {
+				lc = l->data;
+
+				number = phone_number_to_string(
+							&lc->phone_number);
+			}
+
+			ofono_dbus_signal_property_changed(conn, path,
+						OFONO_CALL_FORWARDING_INTERFACE,
+						cf_type_lut[i],
+						DBUS_TYPE_STRING, &number);
+		}
 
 		ofono_dbus_signal_property_changed(conn, path,
 					OFONO_CALL_FORWARDING_INTERFACE,
-- 
1.7.5.4

[PATCHv2 6/8] call-forwarding: Don't run conditional queries if cfu is active

[Oleg Zhurakivskyy]

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

---
 src/call-forwarding.c |   18 +++++++++++-------
 1 files changed, 11 insertions(+), 7 deletions(-)

diff --git a/src/call-forwarding.c b/src/call-forwarding.c
index 7b086e0..2813005 100644
--- a/src/call-forwarding.c
+++ b/src/call-forwarding.c
@@ -584,20 +584,24 @@ static void get_query_cf_callback(const struct ofono_error *error, int total,
 	struct ofono_call_forwarding *cf = data;
 
 	if (error->type == OFONO_ERROR_TYPE_NO_ERROR) {
-		GSList *l;
-		l = cf_cond_list_create(total, list);
+		GSList *l = cf_cond_list_create(total, list);
+
 		set_new_cond_list(cf, cf->query_next, l);
 
 		DBG("%s conditions:", cf_type_lut[cf->query_next]);
+
 		cf_cond_list_print(l);
+	}
 
-		if (cf->query_next == CALL_FORWARDING_TYPE_NOT_REACHABLE)
+	if ((is_cfu_enabled(cf) &&
+			cf->query_next == CALL_FORWARDING_TYPE_UNCONDITIONAL) ||
+			cf->query_next == CALL_FORWARDING_TYPE_NOT_REACHABLE) {
+
+		if (error->type == OFONO_ERROR_TYPE_NO_ERROR)
 			cf->flags |= CALL_FORWARDING_FLAG_CACHED;
-	}
 
-	if (cf->query_next == CALL_FORWARDING_TYPE_NOT_REACHABLE) {
-		DBusMessage *reply = cf_get_properties_reply(cf->pending, cf);
-		__ofono_dbus_pending_reply(&cf->pending, reply);
+		__ofono_dbus_pending_reply(&cf->pending,
+				cf_get_properties_reply(cf->pending, cf));
 		return;
 	}
 
-- 
1.7.5.4

[PATCHv2 7/8] call-forwarding: Clear the conditional cache flag

[Oleg Zhurakivskyy]

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

If there is an successful attempt to deactivate conditional cf
while cfu is active, the conditional cache flag is cleared.
---
 src/call-forwarding.c |    8 ++++++++
 1 files changed, 8 insertions(+), 0 deletions(-)

diff --git a/src/call-forwarding.c b/src/call-forwarding.c
index 2813005..a38a743 100644
--- a/src/call-forwarding.c
+++ b/src/call-forwarding.c
@@ -722,6 +722,14 @@ static void set_query_cf_callback(const struct ofono_error *error, int total,
 	DBG("%s conditions:", cf_type_lut[cf->query_next]);
 	cf_cond_list_print(l);
 
+	/*
+	 * If there is an successful attempt to deactivate conditional cf
+	 * while cfu is active, the conditional cache flag is cleared.
+	 */
+	if (cf->query_next != CALL_FORWARDING_TYPE_UNCONDITIONAL &&
+			is_cfu_enabled(cf))
+		cf->flags &= ~CALL_FORWARDING_FLAG_CACHED;
+
 	if (cf->query_next != cf->query_end) {
 		cf->query_next++;
 		set_query_next_cf_cond(cf);
-- 
1.7.5.4

[PATCHv2 8/8] TODO: Remove completed call forwarding state task

[Oleg Zhurakivskyy]

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

---
 TODO |   17 -----------------
 1 files changed, 0 insertions(+), 17 deletions(-)

diff --git a/TODO b/TODO
index 1fef651..9948117 100644
--- a/TODO
+++ b/TODO
@@ -213,23 +213,6 @@ Supplementary Services
   Priority: Low
   Complexity: C8
 
-- Call forwarding state handling change
-
-  At the moment call forwarding states are not always correct. Any active
-  conditional call forwarding should become quiescent while unconditional call
-  forwarding is activate. If call forwarding unconditional is subsequently
-  deactivated, all the quiescent forwardings should become operative again.
-  I.e. No conditional call forwarding string should be returned while
-  unconditional call forwarding is active even if they exist.
-
-  If there is an successful attempt to activate/deactivate conditional call
-  forwarding while unconditional call forwarding is active the conditional cache
-  flag should cleared.
-
-  Priority: High
-  Complexity: C1
-  Owner: Nicolas Bertrand <nicolas.bertrand@linux.intel.com>
-
 
 Voicecall
 =========
-- 
1.7.5.4

Re: [PATCHv2 1/8] call-forwarding: Minor style fixes

[Denis Kenzior]

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

Hi Oleg,

On 02/06/2012 06:33 AM, Oleg Zhurakivskyy wrote:
> ---
>  src/call-forwarding.c |   56 ++++++++++++++++++++++++++----------------------
>  1 files changed, 30 insertions(+), 26 deletions(-)
> 

Patch has been applied, thanks.

Regards,
-Denis

Re: [PATCHv2 2/8] call-forwarding: Minor refactoring of is_cfu_enabled()

[Denis Kenzior]

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

Hi Oleg,

On 02/06/2012 06:33 AM, Oleg Zhurakivskyy wrote:
> This also removes the need for uninitialized_var() macro.
> ---
>  src/call-forwarding.c |   36 +++++++++++++++++-------------------
>  1 files changed, 17 insertions(+), 19 deletions(-)
> 

Patch has been applied, thanks.

Regards,
-Denis

Re: [PATCHv2 3/8] call-forwarding: Don't set conditional cfs when cfu is active

[Denis Kenzior]

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

Hi Oleg,

On 02/06/2012 06:33 AM, Oleg Zhurakivskyy wrote:
> ---
>  src/call-forwarding.c |   18 ++++++++++++++++++
>  1 files changed, 18 insertions(+), 0 deletions(-)
> 
> diff --git a/src/call-forwarding.c b/src/call-forwarding.c
> index 8b8d5a8..3f9d816 100644
> --- a/src/call-forwarding.c
> +++ b/src/call-forwarding.c
> @@ -802,6 +802,13 @@ static DBusMessage *cf_set_property(DBusConnection *conn, DBusMessage *msg,
>  		if (strlen(number) > 0 && !valid_phone_number_format(number))
>  			return __ofono_error_invalid_format(msg);
>  
> +		/*
> +		 * Don't set conditional cfs when cfu is active
> +		 */
> +		if (type != CALL_FORWARDING_TYPE_UNCONDITIONAL &&
> +				number[0] != '\0' && is_cfu_enabled(cf))
> +			return __ofono_error_not_available(msg);
> +
>  		if (number[0] != '\0')
>  			string_to_phone_number(number, &ph);
>  

I applied this chunk...

> @@ -1159,6 +1166,17 @@ static gboolean cf_ss_control(int type, const char *sc,
>  		return TRUE;
>  	}
>  
> +	/*
> +	 * Don't set conditional cfs when cfu is active
> +	 */
> +	if (type == SS_CONTROL_TYPE_REGISTRATION &&

You are not handling SS_CONTROL_TYPE_ACTIVATION here...

> +			cf_type != CALL_FORWARDING_TYPE_UNCONDITIONAL &&
> +			sia[0] != '\0' && is_cfu_enabled(cf)) {
> +		g_dbus_send_message(conn, __ofono_error_not_available(msg));
> +
> +		return TRUE;
> +	}
> +
>  	cf->ss_req = g_try_new0(struct cf_ss_request, 1);
>  
>  	if (cf->ss_req == NULL) {

However, I think we should leave this part out.  The supplementary
service control path is almost a back door directly to the network, so
if the input is valid it should go through to the driver.  This is why
interrogation is performed here regardless of whether the CACHED flag is
set.

Regards,
-Denis

Re: [PATCHv2 4/8] call-forwarding: Don't report conditional cfs when cfu is active

[Denis Kenzior]

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

Hi Oleg,

On 02/06/2012 06:33 AM, Oleg Zhurakivskyy wrote:
> ---
>  src/call-forwarding.c |   20 +++++++++++++++++---
>  1 files changed, 17 insertions(+), 3 deletions(-)
> 

Patch has been applied, thanks.

Regards,
-Denis

Re: [PATCHv2 5/8] call-forwarding: Emit signals to mask/unmask conditional cfs

[Denis Kenzior]

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

Hi Oleg,

On 02/06/2012 06:33 AM, Oleg Zhurakivskyy wrote:
> Emit signals to mask/unmask conditional cfs on cfu
> activation/deactivation.
> ---
>  src/call-forwarding.c |   30 ++++++++++++++++++++++++++++++
>  1 files changed, 30 insertions(+), 0 deletions(-)
> 

Patch has been applied, thanks.

Regards,
-Denis

Re: [PATCHv2 6/8] call-forwarding: Don't run conditional queries if cfu is active

[Denis Kenzior]

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

Hi Oleg,

On 02/06/2012 06:33 AM, Oleg Zhurakivskyy wrote:
> ---
>  src/call-forwarding.c |   18 +++++++++++-------
>  1 files changed, 11 insertions(+), 7 deletions(-)
> 
> diff --git a/src/call-forwarding.c b/src/call-forwarding.c
> index 7b086e0..2813005 100644
> --- a/src/call-forwarding.c
> +++ b/src/call-forwarding.c
> @@ -584,20 +584,24 @@ static void get_query_cf_callback(const struct ofono_error *error, int total,
>  	struct ofono_call_forwarding *cf = data;
>  
>  	if (error->type == OFONO_ERROR_TYPE_NO_ERROR) {
> -		GSList *l;
> -		l = cf_cond_list_create(total, list);
> +		GSList *l = cf_cond_list_create(total, list);
> +
>  		set_new_cond_list(cf, cf->query_next, l);
>  
>  		DBG("%s conditions:", cf_type_lut[cf->query_next]);
> +
>  		cf_cond_list_print(l);

I separated out two above chunks as a separate patch and applied them,
since they are just cleanup anyway.


> +	}
>  
> -		if (cf->query_next == CALL_FORWARDING_TYPE_NOT_REACHABLE)
> +	if ((is_cfu_enabled(cf) &&
> +			cf->query_next == CALL_FORWARDING_TYPE_UNCONDITIONAL) ||
> +			cf->query_next == CALL_FORWARDING_TYPE_NOT_REACHABLE) {
> +
> +		if (error->type == OFONO_ERROR_TYPE_NO_ERROR)
>  			cf->flags |= CALL_FORWARDING_FLAG_CACHED;
> -	}
>  
> -	if (cf->query_next == CALL_FORWARDING_TYPE_NOT_REACHABLE) {
> -		DBusMessage *reply = cf_get_properties_reply(cf->pending, cf);
> -		__ofono_dbus_pending_reply(&cf->pending, reply);
> +		__ofono_dbus_pending_reply(&cf->pending,
> +				cf_get_properties_reply(cf->pending, cf));
>  		return;
>  	}
>  

The rest is on the right track, however I don't think this really works
in the case where CFU is set when the query is being performed for the
first time.

e.g.
- GetProperties -> query all
- CFU is set, so don't query conditionals (which is correct since they
are useless)
- CFU is removed -> conditionals need to be updated now since they don't
reflect reality

Regards,
-Denis

Re: [PATCHv2 7/8] call-forwarding: Clear the conditional cache flag

[Denis Kenzior]

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

Hi Oleg,

On 02/06/2012 06:34 AM, Oleg Zhurakivskyy wrote:
> If there is an successful attempt to deactivate conditional cf
> while cfu is active, the conditional cache flag is cleared.
> ---
>  src/call-forwarding.c |    8 ++++++++
>  1 files changed, 8 insertions(+), 0 deletions(-)
> 
> diff --git a/src/call-forwarding.c b/src/call-forwarding.c
> index 2813005..a38a743 100644
> --- a/src/call-forwarding.c
> +++ b/src/call-forwarding.c
> @@ -722,6 +722,14 @@ static void set_query_cf_callback(const struct ofono_error *error, int total,
>  	DBG("%s conditions:", cf_type_lut[cf->query_next]);
>  	cf_cond_list_print(l);
>  
> +	/*
> +	 * If there is an successful attempt to deactivate conditional cf
> +	 * while cfu is active, the conditional cache flag is cleared.
> +	 */
> +	if (cf->query_next != CALL_FORWARDING_TYPE_UNCONDITIONAL &&
> +			is_cfu_enabled(cf))
> +		cf->flags &= ~CALL_FORWARDING_FLAG_CACHED;
> +
>  	if (cf->query_next != cf->query_end) {
>  		cf->query_next++;
>  		set_query_next_cf_cond(cf);

This would cause us to re-query everything, which is a bit wasteful.
Also, you have to handle the same action that might occur during
supplementary services path, e.g. the user might deactivate / erase a
conditional service there (see cf_ss_control and
cf_ss_control_callback).  The DisableAll path also needs to be handled
properly.

Also, there is another path you need to take under consideration and
that is the supplementary service interrogation path.  That one can
update conditional settings erroneously if CFU is active.  See
ss_set_query_cf_callback.

We might need to track CFU and conditional caches separately to make
things easier.  Let me know if you have any questions.  CallForwarding
was one of the earliest implemented interfaces and turned out to be
quite a bit more complicated than originally thought ;)

Regards,
-Denis

Re: [PATCHv2 6/8] call-forwarding: Don't run conditional queries if cfu is active

[Oleg Zhurakivskyy]

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

Hello Denis,

On 02/22/2012 03:24 PM, Denis Kenzior wrote:
> The rest is on the right track, however I don't think this really works
> in the case where CFU is set when the query is being performed for the
> first time.
>
> e.g.
> - GetProperties ->  query all
> - CFU is set, so don't query conditionals (which is correct since they
> are useless)
> - CFU is removed ->  conditionals need to be updated now since they don't
> reflect reality

Thanks for the review and for pointing this out. I will check how this could be 
corrected.

Regards,
Oleg
-- 
Intel Finland Oy
Registered Address: PL 281, 00181 Helsinki
Business Identity Code: 0357606 - 4
Domiciled in Helsinki

Re: [PATCHv2 7/8] call-forwarding: Clear the conditional cache flag

[Oleg Zhurakivskyy]

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

Hello Denis,

On 02/22/2012 03:33 PM, Denis Kenzior wrote:
> This would cause us to re-query everything, which is a bit wasteful.
> Also, you have to handle the same action that might occur during
> supplementary services path, e.g. the user might deactivate / erase a
> conditional service there (see cf_ss_control and
> cf_ss_control_callback).  The DisableAll path also needs to be handled
> properly.
>
> Also, there is another path you need to take under consideration and
> that is the supplementary service interrogation path.  That one can
> update conditional settings erroneously if CFU is active.  See
> ss_set_query_cf_callback.
>
> We might need to track CFU and conditional caches separately to make
> things easier.  Let me know if you have any questions.  CallForwarding
> was one of the earliest implemented interfaces and turned out to be
> quite a bit more complicated than originally thought ;)

Thanks for the help and ideas here. Let me check the supplementary services path 
and how to deal with the caching. I will prepare patches to address these.

Regards,
Oleg
-- 
Intel Finland Oy
Registered Address: PL 281, 00181 Helsinki
Business Identity Code: 0357606 - 4
Domiciled in Helsinki