[PATCHv3 00/12] Call forwarding state handling change

[PATCHv3 00/12] Call forwarding state handling change

[Oleg Zhurakivskyy]

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

Hello,

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

Changes from v2:

- Re-run conditional queries on cfu removal and mark cached.
- Handle the caching properly on supplementary services path
  unconditional/all/all conditional cfs modifications.

Regards,
Oleg

Oleg Zhurakivskyy (12):
  call-forwarding: Remove cf_list_clear()
  call-forwarding: Inline get_query_next_cf_cond()
  call-forwarding: Refactor cf_condition_find_with_cls() slightly
  call-forwarding: Refactor cf_find_unconditional()
  call-forwarding: Minor cleanup of set_query_cf_callback
  call-forwarding: Don't run conditional queries if cfu is active
  call-forwarding: Re-run conditional queries on cfu removal
  call-forwarding: Toggle the cached flag on CFU changes
  call-forwarding: Cache cfs on CFU removal
  call-forwarding: Re-run ss path cfs queries on cfu changes
  call-forwarding: Cache cfs on all/all conditional removal
  TODO: Remove completed call forwarding state task

 TODO                  |   17 ----
 src/call-forwarding.c |  237 +++++++++++++++++++++----------------------------
 2 files changed, 100 insertions(+), 154 deletions(-)

-- 
1.7.5.4

[PATCHv3 01/12] call-forwarding: Remove cf_list_clear()

[Oleg Zhurakivskyy]

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

Use g_slist_free_full() instead.
---
 src/call-forwarding.c |   14 ++------------
 1 files changed, 2 insertions(+), 12 deletions(-)

diff --git a/src/call-forwarding.c b/src/call-forwarding.c
index 864be1b..0857362 100644
--- a/src/call-forwarding.c
+++ b/src/call-forwarding.c
@@ -183,22 +183,12 @@ static GSList *cf_cond_list_create(int total,
 	return l;
 }
 
-static inline void cf_list_clear(GSList *cf_list)
-{
-	GSList *l;
-
-	for (l = cf_list; l; l = l->next)
-		g_free(l->data);
-
-	g_slist_free(cf_list);
-}
-
 static inline void cf_clear_all(struct ofono_call_forwarding *cf)
 {
 	int i;
 
 	for (i = 0; i < 4; i++) {
-		cf_list_clear(cf->cf_conditions[i]);
+		g_slist_free_full(cf->cf_conditions[i], g_free);
 		cf->cf_conditions[i] = NULL;
 	}
 }
@@ -422,7 +412,7 @@ static void set_new_cond_list(struct ofono_call_forwarding *cf,
 						&timeout);
 	}
 
-	cf_list_clear(old);
+	g_slist_free_full(old, g_free);
 	cf->cf_conditions[type] = list;
 
 	if (update_sim == TRUE)
-- 
1.7.5.4

[PATCHv3 02/12] call-forwarding: Inline get_query_next_cf_cond()

[Oleg Zhurakivskyy]

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

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

diff --git a/src/call-forwarding.c b/src/call-forwarding.c
index 0857362..8b6659a 100644
--- a/src/call-forwarding.c
+++ b/src/call-forwarding.c
@@ -595,7 +595,7 @@ static void get_query_cf_callback(const struct ofono_error *error, int total,
 	get_query_next_cf_cond(cf);
 }
 
-static void get_query_next_cf_cond(struct ofono_call_forwarding *cf)
+static inline void get_query_next_cf_cond(struct ofono_call_forwarding *cf)
 {
 	cf->driver->query(cf, cf->query_next, BEARER_CLASS_DEFAULT,
 				get_query_cf_callback, cf);
-- 
1.7.5.4

[PATCHv3 03/12] call-forwarding: Refactor cf_condition_find_with_cls() slightly

[Oleg Zhurakivskyy]

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

Mainly to simplify the usage.
---
 src/call-forwarding.c |   98 +++++++++++++-----------------------------------
 1 files changed, 27 insertions(+), 71 deletions(-)

diff --git a/src/call-forwarding.c b/src/call-forwarding.c
index 8b6659a..5a2ab28 100644
--- a/src/call-forwarding.c
+++ b/src/call-forwarding.c
@@ -42,6 +42,8 @@
 /* According to 27.007 Spec */
 #define DEFAULT_NO_REPLY_TIMEOUT 20
 
+#define CFC(_x) ((struct ofono_call_forwarding_condition *)(_x))
+
 #define is_cfu_enabled(_cf)				\
 ({							\
 	cf_find_unconditional(_cf) ? TRUE : FALSE;	\
@@ -86,63 +88,36 @@ 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)
+static gint cf_compare(gconstpointer a, gconstpointer b)
 {
-	const struct ofono_call_forwarding_condition *ca = a;
-	const struct ofono_call_forwarding_condition *cb = b;
-
-	if (ca->cls < cb->cls)
-		return -1;
-
-	if (ca->cls > cb->cls)
-		return 1;
-
-	return 0;
+	return CFC(a)->cls - CFC(b)->cls;
 }
 
-static gint cf_condition_find_with_cls(gconstpointer a, gconstpointer b)
+static struct ofono_call_forwarding_condition *cf_find(GSList *l, int cls)
 {
-	const struct ofono_call_forwarding_condition *c = a;
-	int cls = GPOINTER_TO_INT(b);
-
-	if (c->cls < cls)
-		return -1;
-
-	if (c->cls > cls)
-		return 1;
+	for (; l; l = l->next)
+		if (cls == CFC(l->data)->cls)
+			return CFC(l->data);
 
-	return 0;
+	return NULL;
 }
 
-static int cf_find_timeout(GSList *cf_list, int cls)
+static int cf_find_timeout(GSList *l, int cls)
 {
-	GSList *l;
-	struct ofono_call_forwarding_condition *c;
-
-	l = g_slist_find_custom(cf_list, GINT_TO_POINTER(cls),
-		cf_condition_find_with_cls);
-
-	if (l == NULL)
-		return DEFAULT_NO_REPLY_TIMEOUT;
-
-	c = l->data;
+	struct ofono_call_forwarding_condition *c = cf_find(l, cls);
 
-	return c->time;
+	return c ? c->time : DEFAULT_NO_REPLY_TIMEOUT;
 }
 
-static void cf_cond_list_print(GSList *list)
+static void cf_cond_list_print(GSList *l)
 {
-	GSList *l;
-	struct ofono_call_forwarding_condition *cond;
-
-	for (l = list; l; l = l->next) {
-		cond = l->data;
+	struct ofono_call_forwarding_condition *c;
 
+	for (; l && (c = l->data); l = l->next)
 		DBG("CF Condition status: %d, class: %d, number: %s,"
 			" number_type: %d, time: %d",
-			cond->status, cond->cls, cond->phone_number.number,
-			cond->phone_number.type, cond->time);
-	}
+			c->status, c->cls, c->phone_number.number,
+			c->phone_number.type, c->time);
 }
 
 static GSList *cf_cond_list_create(int total,
@@ -175,8 +150,7 @@ static GSList *cf_cond_list_create(int total,
 				sizeof(struct ofono_call_forwarding_condition));
 			cond->cls = j;
 
-			l = g_slist_insert_sorted(l, cond,
-							cf_condition_compare);
+			l = g_slist_insert_sorted(l, cond, cf_compare);
 		}
 	}
 
@@ -330,12 +304,8 @@ static void set_new_cond_list(struct ofono_call_forwarding *cf,
 		if (type == CALL_FORWARDING_TYPE_NO_REPLY)
 			snprintf(tattr, sizeof(tattr), "%sTimeout", attr);
 
-		o = g_slist_find_custom(old, GINT_TO_POINTER(lc->cls),
-					cf_condition_find_with_cls);
-
-		if (o) { /* On the old list, must be active */
-			oc = o->data;
-
+		oc = cf_find(old, lc->cls);
+		if (oc) { /* On the old list, must be active */
 			if (oc->phone_number.type != lc->phone_number.type ||
 				strcmp(oc->phone_number.number,
 					lc->phone_number.number)) {
@@ -356,8 +326,8 @@ static void set_new_cond_list(struct ofono_call_forwarding *cf,
 						&timeout);
 
 			/* Remove from the old list */
-			g_free(o->data);
-			old = g_slist_remove(old, o->data);
+			old = g_slist_remove(old, oc);
+			g_free(oc);
 		} else {
 			number = phone_number_to_string(&lc->phone_number);
 
@@ -435,21 +405,12 @@ static void set_new_cond_list(struct ofono_call_forwarding *cf,
 			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)
+			lc = cf_find(cf->cf_conditions[i], BEARER_CLASS_VOICE);
+			if (lc == NULL)
 				continue;
 
-			if (new_cfu)
-				number = "";
-			else {
-				lc = l->data;
-
-				number = phone_number_to_string(
+			number = new_cfu ? "" : phone_number_to_string(
 							&lc->phone_number);
-			}
 
 			ofono_dbus_signal_property_changed(conn, path,
 						OFONO_CALL_FORWARDING_INTERFACE,
@@ -796,7 +757,6 @@ static DBusMessage *cf_set_property(DBusConnection *conn, DBusMessage *msg,
 
 	if (cf_condition_timeout_property(property, &cls)) {
 		dbus_uint16_t timeout;
-		GSList *l;
 		struct ofono_call_forwarding_condition *c;
 
 		type = CALL_FORWARDING_TYPE_NO_REPLY;
@@ -809,15 +769,11 @@ static DBusMessage *cf_set_property(DBusConnection *conn, DBusMessage *msg,
 		if (timeout < 1 || timeout > 30)
 			return __ofono_error_invalid_format(msg);
 
-		l = g_slist_find_custom(cf->cf_conditions[type],
-					GINT_TO_POINTER(cls),
-					cf_condition_find_with_cls);
 
-		if (l == NULL)
+		c = cf_find(cf->cf_conditions[type], cls);
+		if (c == NULL)
 			return __ofono_error_failed(msg);
 
-		c = l->data;
-
 		return set_property_request(cf, msg, type, cls,
 						&c->phone_number, timeout);
 	} else if (cf_condition_enabled_property(cf, property, &type, &cls)) {
-- 
1.7.5.4

[PATCHv3 04/12] call-forwarding: Refactor cf_find_unconditional()

[Oleg Zhurakivskyy]

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

Re-use cf_find() instead.
---
 src/call-forwarding.c |   28 ++++++----------------------
 1 files changed, 6 insertions(+), 22 deletions(-)

diff --git a/src/call-forwarding.c b/src/call-forwarding.c
index 5a2ab28..b1a1a72 100644
--- a/src/call-forwarding.c
+++ b/src/call-forwarding.c
@@ -44,6 +44,12 @@
 
 #define CFC(_x) ((struct ofono_call_forwarding_condition *)(_x))
 
+#define cf_find_unconditional(_cf)					\
+({									\
+	cf_find((_cf)->cf_conditions[CALL_FORWARDING_TYPE_UNCONDITIONAL],\
+		BEARER_CLASS_VOICE);					\
+})
+
 #define is_cfu_enabled(_cf)				\
 ({							\
 	cf_find_unconditional(_cf) ? TRUE : FALSE;	\
@@ -188,28 +194,6 @@ static void sim_cphs_cff_update_cb(int ok, void *data)
 		ofono_info("Failed to update EFcphs-cff");
 }
 
-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;
-
-	/*
-	 * For now we only support Voice, although Fax & all Data
-	 * basic services are applicable as well.
-	 */
-	for (; l; l = l->next) {
-		cond = l->data;
-
-		if (cond->cls > BEARER_CLASS_VOICE)
-			continue;
-
-		return cond;
-	}
-
-	return NULL;
-}
-
 static void sim_set_cf_indicator(struct ofono_call_forwarding *cf)
 {
 	struct ofono_call_forwarding_condition *cfu_voice =
-- 
1.7.5.4

[PATCHv3 05/12] call-forwarding: Minor cleanup of set_query_cf_callback

[Oleg Zhurakivskyy]

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

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

diff --git a/src/call-forwarding.c b/src/call-forwarding.c
index b1a1a72..7e5edcb 100644
--- a/src/call-forwarding.c
+++ b/src/call-forwarding.c
@@ -631,32 +631,28 @@ static void set_query_cf_callback(const struct ofono_error *error, int total,
 			void *data)
 {
 	struct ofono_call_forwarding *cf = data;
-	GSList *l;
-	DBusMessage *reply;
 
 	if (error->type != OFONO_ERROR_TYPE_NO_ERROR) {
 		ofono_error("Setting succeeded, but query failed");
 		cf->flags &= ~CALL_FORWARDING_FLAG_CACHED;
-		reply = __ofono_error_failed(cf->pending);
-		__ofono_dbus_pending_reply(&cf->pending, reply);
+		__ofono_dbus_pending_reply(&cf->pending,
+					__ofono_error_failed(cf->pending));
 		return;
 	}
 
-	if (cf->query_next == cf->query_end) {
-		reply = dbus_message_new_method_return(cf->pending);
-		__ofono_dbus_pending_reply(&cf->pending, reply);
-	}
-
-	l = cf_cond_list_create(total, list);
-	set_new_cond_list(cf, cf->query_next, l);
+	set_new_cond_list(cf, cf->query_next, cf_cond_list_create(total, list));
 
 	DBG("%s conditions:", cf_type_lut[cf->query_next]);
-	cf_cond_list_print(l);
+	cf_cond_list_print(cf->cf_conditions[cf->query_next]);
 
-	if (cf->query_next != cf->query_end) {
-		cf->query_next++;
-		set_query_next_cf_cond(cf);
+	if (cf->query_next == cf->query_end) {
+		__ofono_dbus_pending_reply(&cf->pending,
+				dbus_message_new_method_return(cf->pending));
+		return;
 	}
+
+	cf->query_next++;
+	set_query_next_cf_cond(cf);
 }
 
 static void set_query_next_cf_cond(struct ofono_call_forwarding *cf)
-- 
1.7.5.4

[PATCHv3 06/12] call-forwarding: Don't run conditional queries if cfu is active

[Oleg Zhurakivskyy]

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

---
 src/call-forwarding.c |   19 ++++++++++---------
 1 files changed, 10 insertions(+), 9 deletions(-)

diff --git a/src/call-forwarding.c b/src/call-forwarding.c
index 7e5edcb..ad5050b 100644
--- a/src/call-forwarding.c
+++ b/src/call-forwarding.c
@@ -525,19 +525,18 @@ static void get_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 (cf->query_next == CALL_FORWARDING_TYPE_NOT_REACHABLE)
-			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);
+	if (cf->query_next++ < cf->query_end) {
+		get_query_next_cf_cond(cf);
 		return;
 	}
 
-	cf->query_next++;
-	get_query_next_cf_cond(cf);
+	if (error->type == OFONO_ERROR_TYPE_NO_ERROR)
+		cf->flags |= CALL_FORWARDING_FLAG_CACHED;
+
+	__ofono_dbus_pending_reply(&cf->pending,
+				cf_get_properties_reply(cf->pending, cf));
 }
 
 static inline void get_query_next_cf_cond(struct ofono_call_forwarding *cf)
@@ -565,7 +564,9 @@ static DBusMessage *cf_get_properties(DBusConnection *conn, DBusMessage *msg,
 
 	cf->pending = dbus_message_ref(msg);
 	cf->query_next = 0;
-
+	cf->query_end = is_cfu_enabled(cf) ?
+					CALL_FORWARDING_TYPE_UNCONDITIONAL :
+					CALL_FORWARDING_TYPE_NOT_REACHABLE;
 	get_query_next_cf_cond(cf);
 
 	return NULL;
-- 
1.7.5.4

[PATCHv3 07/12] call-forwarding: Re-run conditional queries on cfu removal

[Oleg Zhurakivskyy]

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

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

diff --git a/src/call-forwarding.c b/src/call-forwarding.c
index ad5050b..edd1891 100644
--- a/src/call-forwarding.c
+++ b/src/call-forwarding.c
@@ -691,7 +691,15 @@ static DBusMessage *set_property_request(struct ofono_call_forwarding *cf,
 
 	cf->pending = dbus_message_ref(msg);
 	cf->query_next = type;
-	cf->query_end = type;
+
+	if (type == CALL_FORWARDING_TYPE_UNCONDITIONAL &&
+		ph->number[0] == '\0' && is_cfu_enabled(cf))
+		/*
+		 * CFU is removed, conditionals need to be updated
+		 */
+		cf->query_end = CALL_FORWARDING_TYPE_NOT_REACHABLE;
+	else
+		cf->query_end = type;
 
 	DBG("Farming off request, will be erasure: %d", ph->number[0] == '\0');
 
-- 
1.7.5.4

[PATCHv3 08/12] call-forwarding: Toggle the cached flag on CFU changes

[Oleg Zhurakivskyy]

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

Set the flag on CFU activation, unset on deactivation.
---
 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 edd1891..575d96d 100644
--- a/src/call-forwarding.c
+++ b/src/call-forwarding.c
@@ -646,6 +646,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(cf->cf_conditions[cf->query_next]);
 
+	/*
+	 * Toggle the cached flag on CFU changes
+	 */
+	if (cf->query_next == CALL_FORWARDING_TYPE_UNCONDITIONAL &&
+			((cf->flags & CALL_FORWARDING_FLAG_CACHED) !=
+				is_cfu_enabled(cf)))
+		cf->flags ^= CALL_FORWARDING_FLAG_CACHED;
+
 	if (cf->query_next == cf->query_end) {
 		__ofono_dbus_pending_reply(&cf->pending,
 				dbus_message_new_method_return(cf->pending));
-- 
1.7.5.4

[PATCHv3 09/12] call-forwarding: Cache cfs on CFU removal

[Oleg Zhurakivskyy]

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

Since the re-querying is done, cache the call forwardings.
---
 src/call-forwarding.c |   14 +++++++++++---
 1 files changed, 11 insertions(+), 3 deletions(-)

diff --git a/src/call-forwarding.c b/src/call-forwarding.c
index 575d96d..7109c21 100644
--- a/src/call-forwarding.c
+++ b/src/call-forwarding.c
@@ -38,6 +38,7 @@
 
 #define CALL_FORWARDING_FLAG_CACHED	0x1
 #define CALL_FORWARDING_FLAG_CPHS_CFF	0x2
+#define CALL_FORWARDING_FLAG_CACHE	0x4
 
 /* According to 27.007 Spec */
 #define DEFAULT_NO_REPLY_TIMEOUT 20
@@ -635,7 +636,8 @@ static void set_query_cf_callback(const struct ofono_error *error, int total,
 
 	if (error->type != OFONO_ERROR_TYPE_NO_ERROR) {
 		ofono_error("Setting succeeded, but query failed");
-		cf->flags &= ~CALL_FORWARDING_FLAG_CACHED;
+		cf->flags &= ~(CALL_FORWARDING_FLAG_CACHED |
+				CALL_FORWARDING_FLAG_CACHE);
 		__ofono_dbus_pending_reply(&cf->pending,
 					__ofono_error_failed(cf->pending));
 		return;
@@ -655,6 +657,11 @@ static void set_query_cf_callback(const struct ofono_error *error, int total,
 		cf->flags ^= CALL_FORWARDING_FLAG_CACHED;
 
 	if (cf->query_next == cf->query_end) {
+		if (cf->flags & CALL_FORWARDING_FLAG_CACHE) {
+			cf->flags &= ~CALL_FORWARDING_FLAG_CACHE;
+			cf->flags |= CALL_FORWARDING_FLAG_CACHED;
+		}
+
 		__ofono_dbus_pending_reply(&cf->pending,
 				dbus_message_new_method_return(cf->pending));
 		return;
@@ -701,12 +708,13 @@ static DBusMessage *set_property_request(struct ofono_call_forwarding *cf,
 	cf->query_next = type;
 
 	if (type == CALL_FORWARDING_TYPE_UNCONDITIONAL &&
-		ph->number[0] == '\0' && is_cfu_enabled(cf))
+		ph->number[0] == '\0' && is_cfu_enabled(cf)) {
 		/*
 		 * CFU is removed, conditionals need to be updated
 		 */
 		cf->query_end = CALL_FORWARDING_TYPE_NOT_REACHABLE;
-	else
+		cf->flags |= CALL_FORWARDING_FLAG_CACHE;
+	} else
 		cf->query_end = type;
 
 	DBG("Farming off request, will be erasure: %d", ph->number[0] == '\0');
-- 
1.7.5.4

[PATCHv3 10/12] call-forwarding: Re-run ss path cfs queries on cfu changes

[Oleg Zhurakivskyy]

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

Re-run conditional call forwarding queries on cfu removal
via supplementary services path and cache.
---
 src/call-forwarding.c |   14 +++++++++++++-
 1 files changed, 13 insertions(+), 1 deletions(-)

diff --git a/src/call-forwarding.c b/src/call-forwarding.c
index 7109c21..b168dc3 100644
--- a/src/call-forwarding.c
+++ b/src/call-forwarding.c
@@ -987,7 +987,8 @@ static void ss_set_query_cf_callback(const struct ofono_error *error, int total,
 
 	if (error->type != OFONO_ERROR_TYPE_NO_ERROR) {
 		ofono_error("Setting succeeded, but query failed");
-		cf->flags &= ~CALL_FORWARDING_FLAG_CACHED;
+		cf->flags &= ~(CALL_FORWARDING_FLAG_CACHE |
+				CALL_FORWARDING_FLAG_CACHED);
 		reply = __ofono_error_failed(cf->pending);
 		__ofono_dbus_pending_reply(&cf->pending, reply);
 		return;
@@ -1000,6 +1001,10 @@ static void ss_set_query_cf_callback(const struct ofono_error *error, int total,
 	cf->ss_req->cf_list[cf->query_next] = l;
 
 	if (cf->query_next == cf->query_end) {
+		if (cf->flags & CALL_FORWARDING_FLAG_CACHE) {
+			cf->flags &= ~CALL_FORWARDING_FLAG_CACHE;
+			cf->flags |= CALL_FORWARDING_FLAG_CACHED;
+		}
 		reply = cf_ss_control_reply(cf, cf->ss_req);
 		__ofono_dbus_pending_reply(&cf->pending, reply);
 		g_free(cf->ss_req);
@@ -1177,6 +1182,13 @@ static gboolean cf_ss_control(int type, const char *sc,
 	cf->pending = dbus_message_ref(msg);
 
 	switch (cf->ss_req->cf_type) {
+	case CALL_FORWARDING_TYPE_UNCONDITIONAL:
+		cf->flags |= CALL_FORWARDING_FLAG_CACHE;
+		cf->query_next = CALL_FORWARDING_TYPE_UNCONDITIONAL;
+		cf->query_end = is_cfu_enabled(cf) ?
+					CALL_FORWARDING_TYPE_NOT_REACHABLE :
+					CALL_FORWARDING_TYPE_UNCONDITIONAL;
+		break;
 	case CALL_FORWARDING_TYPE_ALL:
 		cf->query_next = CALL_FORWARDING_TYPE_UNCONDITIONAL;
 		cf->query_end = CALL_FORWARDING_TYPE_NOT_REACHABLE;
-- 
1.7.5.4

[PATCHv3 11/12] call-forwarding: Cache cfs on all/all conditional removal

[Oleg Zhurakivskyy]

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

Re-run conditional call forwarding queries on cfu removal
via supplementary services path and cache.
---
 src/call-forwarding.c |   10 +++++-----
 1 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/src/call-forwarding.c b/src/call-forwarding.c
index b168dc3..edfb87a 100644
--- a/src/call-forwarding.c
+++ b/src/call-forwarding.c
@@ -1181,7 +1181,7 @@ static gboolean cf_ss_control(int type, const char *sc,
 
 	cf->pending = dbus_message_ref(msg);
 
-	switch (cf->ss_req->cf_type) {
+	switch (cf_type) {
 	case CALL_FORWARDING_TYPE_UNCONDITIONAL:
 		cf->flags |= CALL_FORWARDING_FLAG_CACHE;
 		cf->query_next = CALL_FORWARDING_TYPE_UNCONDITIONAL;
@@ -1190,11 +1190,11 @@ static gboolean cf_ss_control(int type, const char *sc,
 					CALL_FORWARDING_TYPE_UNCONDITIONAL;
 		break;
 	case CALL_FORWARDING_TYPE_ALL:
-		cf->query_next = CALL_FORWARDING_TYPE_UNCONDITIONAL;
-		cf->query_end = CALL_FORWARDING_TYPE_NOT_REACHABLE;
-		break;
 	case CALL_FORWARDING_TYPE_ALL_CONDITIONAL:
-		cf->query_next = CALL_FORWARDING_TYPE_BUSY;
+		cf->flags |= CALL_FORWARDING_FLAG_CACHE;
+		cf->query_next = (cf_type == CALL_FORWARDING_TYPE_ALL) ?
+					CALL_FORWARDING_TYPE_UNCONDITIONAL :
+					CALL_FORWARDING_TYPE_BUSY;
 		cf->query_end = CALL_FORWARDING_TYPE_NOT_REACHABLE;
 		break;
 	default:
-- 
1.7.5.4

[PATCHv3 12/12] 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: [PATCHv3 01/12] call-forwarding: Remove cf_list_clear()

[Denis Kenzior]

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

Hi Oleg,

On 03/13/2012 08:46 AM, Oleg Zhurakivskyy wrote:
> Use g_slist_free_full() instead.
> ---
>  src/call-forwarding.c |   14 ++------------
>  1 files changed, 2 insertions(+), 12 deletions(-)
> 

Patch has been applied, thanks.

Regards,
-Denis

Re: [PATCHv3 02/12] call-forwarding: Inline get_query_next_cf_cond()

[Denis Kenzior]

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

Hi Oleg,

On 03/13/2012 08:46 AM, Oleg Zhurakivskyy wrote:
> ---
>  src/call-forwarding.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 

Patch has been applied, thanks.

Regards,
-Denis

Re: [PATCHv3 03/12] call-forwarding: Refactor cf_condition_find_with_cls() slightly

[Denis Kenzior]

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

Hi Oleg,

On 03/13/2012 08:46 AM, Oleg Zhurakivskyy wrote:
> Mainly to simplify the usage.
> ---
>  src/call-forwarding.c |   98 +++++++++++++-----------------------------------
>  1 files changed, 27 insertions(+), 71 deletions(-)
> 
> diff --git a/src/call-forwarding.c b/src/call-forwarding.c
> index 8b6659a..5a2ab28 100644
> --- a/src/call-forwarding.c
> +++ b/src/call-forwarding.c
> @@ -42,6 +42,8 @@
>  /* According to 27.007 Spec */
>  #define DEFAULT_NO_REPLY_TIMEOUT 20
>  
> +#define CFC(_x) ((struct ofono_call_forwarding_condition *)(_x))
> +

I'm okay with such helpers, but...

>  #define is_cfu_enabled(_cf)				\
>  ({							\
>  	cf_find_unconditional(_cf) ? TRUE : FALSE;	\
> @@ -86,63 +88,36 @@ 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)
> +static gint cf_compare(gconstpointer a, gconstpointer b)
>  {
> -	const struct ofono_call_forwarding_condition *ca = a;
> -	const struct ofono_call_forwarding_condition *cb = b;
> -
> -	if (ca->cls < cb->cls)
> -		return -1;
> -
> -	if (ca->cls > cb->cls)
> -		return 1;
> -
> -	return 0;
> +	return CFC(a)->cls - CFC(b)->cls;

You're breaking const-correctness here.

Also, why do you rename the function?  I do think we should have
'condition' or at least 'cond' in the name to make it clear what it
does.  My vote is for cf_cond_compare...

>  }
>  
> -static gint cf_condition_find_with_cls(gconstpointer a, gconstpointer b)
> +static struct ofono_call_forwarding_condition *cf_find(GSList *l, int cls)

Same as above, at least name it cf_cond_find.  Also, I'd prefer changes
to this function to be in a separate patch.

>  {
> -	const struct ofono_call_forwarding_condition *c = a;
> -	int cls = GPOINTER_TO_INT(b);
> -
> -	if (c->cls < cls)
> -		return -1;
> -
> -	if (c->cls > cls)
> -		return 1;
> +	for (; l; l = l->next)
> +		if (cls == CFC(l->data)->cls)
> +			return CFC(l->data);
>  
> -	return 0;
> +	return NULL;
>  }
>  
> -static int cf_find_timeout(GSList *cf_list, int cls)
> +static int cf_find_timeout(GSList *l, int cls)

Same here, lets name this cf_cond_find_timeout

>  {
> -	GSList *l;
> -	struct ofono_call_forwarding_condition *c;
> -
> -	l = g_slist_find_custom(cf_list, GINT_TO_POINTER(cls),
> -		cf_condition_find_with_cls);
> -
> -	if (l == NULL)
> -		return DEFAULT_NO_REPLY_TIMEOUT;
> -
> -	c = l->data;
> +	struct ofono_call_forwarding_condition *c = cf_find(l, cls);
>  
> -	return c->time;
> +	return c ? c->time : DEFAULT_NO_REPLY_TIMEOUT;
>  }
>  
> -static void cf_cond_list_print(GSList *list)
> +static void cf_cond_list_print(GSList *l)
>  {
> -	GSList *l;
> -	struct ofono_call_forwarding_condition *cond;
> -
> -	for (l = list; l; l = l->next) {
> -		cond = l->data;
> +	struct ofono_call_forwarding_condition *c;
>  
> +	for (; l && (c = l->data); l = l->next)

After going back and forth on this one, I'm actually against this style.
 Mainly it is too hard to notice the assignment, inside the continuation
check expression.  I'd really prefer that we assign c inside the for
loop block.  Also, l->data should never be NULL, so in this case I'd
rather crash and find out early than inadvertently covering it up.

>  		DBG("CF Condition status: %d, class: %d, number: %s,"
>  			" number_type: %d, time: %d",
> -			cond->status, cond->cls, cond->phone_number.number,
> -			cond->phone_number.type, cond->time);
> -	}
> +			c->status, c->cls, c->phone_number.number,
> +			c->phone_number.type, c->time);
>  }
>  
>  static GSList *cf_cond_list_create(int total,
> @@ -175,8 +150,7 @@ static GSList *cf_cond_list_create(int total,
>  				sizeof(struct ofono_call_forwarding_condition));
>  			cond->cls = j;
>  
> -			l = g_slist_insert_sorted(l, cond,
> -							cf_condition_compare);
> +			l = g_slist_insert_sorted(l, cond, cf_compare);
>  		}
>  	}
>  
> @@ -330,12 +304,8 @@ static void set_new_cond_list(struct ofono_call_forwarding *cf,
>  		if (type == CALL_FORWARDING_TYPE_NO_REPLY)
>  			snprintf(tattr, sizeof(tattr), "%sTimeout", attr);
>  
> -		o = g_slist_find_custom(old, GINT_TO_POINTER(lc->cls),
> -					cf_condition_find_with_cls);
> -
> -		if (o) { /* On the old list, must be active */
> -			oc = o->data;
> -
> +		oc = cf_find(old, lc->cls);
> +		if (oc) { /* On the old list, must be active */
>  			if (oc->phone_number.type != lc->phone_number.type ||
>  				strcmp(oc->phone_number.number,
>  					lc->phone_number.number)) {
> @@ -356,8 +326,8 @@ static void set_new_cond_list(struct ofono_call_forwarding *cf,
>  						&timeout);
>  
>  			/* Remove from the old list */
> -			g_free(o->data);
> -			old = g_slist_remove(old, o->data);
> +			old = g_slist_remove(old, oc);
> +			g_free(oc);
>  		} else {
>  			number = phone_number_to_string(&lc->phone_number);
>  
> @@ -435,21 +405,12 @@ static void set_new_cond_list(struct ofono_call_forwarding *cf,
>  			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)
> +			lc = cf_find(cf->cf_conditions[i], BEARER_CLASS_VOICE);
> +			if (lc == NULL)
>  				continue;
>  
> -			if (new_cfu)
> -				number = "";
> -			else {
> -				lc = l->data;
> -
> -				number = phone_number_to_string(
> +			number = new_cfu ? "" : phone_number_to_string(
>  							&lc->phone_number);

This part really belongs in a separate patch.

> -			}
>  
>  			ofono_dbus_signal_property_changed(conn, path,
>  						OFONO_CALL_FORWARDING_INTERFACE,
> @@ -796,7 +757,6 @@ static DBusMessage *cf_set_property(DBusConnection *conn, DBusMessage *msg,
>  
>  	if (cf_condition_timeout_property(property, &cls)) {
>  		dbus_uint16_t timeout;
> -		GSList *l;
>  		struct ofono_call_forwarding_condition *c;
>  
>  		type = CALL_FORWARDING_TYPE_NO_REPLY;
> @@ -809,15 +769,11 @@ static DBusMessage *cf_set_property(DBusConnection *conn, DBusMessage *msg,
>  		if (timeout < 1 || timeout > 30)
>  			return __ofono_error_invalid_format(msg);
>  
> -		l = g_slist_find_custom(cf->cf_conditions[type],
> -					GINT_TO_POINTER(cls),
> -					cf_condition_find_with_cls);
>  
> -		if (l == NULL)
> +		c = cf_find(cf->cf_conditions[type], cls);
> +		if (c == NULL)
>  			return __ofono_error_failed(msg);
>  
> -		c = l->data;
> -
>  		return set_property_request(cf, msg, type, cls,
>  						&c->phone_number, timeout);
>  	} else if (cf_condition_enabled_property(cf, property, &type, &cls)) {

Regards,
-Denis

Re: [PATCHv3 04/12] call-forwarding: Refactor cf_find_unconditional()

[Denis Kenzior]

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

On 03/13/2012 08:46 AM, Oleg Zhurakivskyy wrote:
> Re-use cf_find() instead.
> ---
>  src/call-forwarding.c |   28 ++++++----------------------
>  1 files changed, 6 insertions(+), 22 deletions(-)
> 
> diff --git a/src/call-forwarding.c b/src/call-forwarding.c
> index 5a2ab28..b1a1a72 100644
> --- a/src/call-forwarding.c
> +++ b/src/call-forwarding.c
> @@ -44,6 +44,12 @@
>  
>  #define CFC(_x) ((struct ofono_call_forwarding_condition *)(_x))
>  
> +#define cf_find_unconditional(_cf)					\
> +({									\
> +	cf_find((_cf)->cf_conditions[CALL_FORWARDING_TYPE_UNCONDITIONAL],\
> +		BEARER_CLASS_VOICE);					\
> +})
> +

Actually I'd prefer if we made this an inline function rather than a
macro.  A bit easier to read that way.

Regards,
-Denis

Re: [PATCHv3 05/12] call-forwarding: Minor cleanup of set_query_cf_callback

[Denis Kenzior]

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

Hi Oleg,

On 03/13/2012 08:46 AM, Oleg Zhurakivskyy wrote:
> ---
>  src/call-forwarding.c |   26 +++++++++++---------------
>  1 files changed, 11 insertions(+), 15 deletions(-)
> 
> diff --git a/src/call-forwarding.c b/src/call-forwarding.c
> index b1a1a72..7e5edcb 100644
> --- a/src/call-forwarding.c
> +++ b/src/call-forwarding.c
> @@ -631,32 +631,28 @@ static void set_query_cf_callback(const struct ofono_error *error, int total,
>  			void *data)
>  {
>  	struct ofono_call_forwarding *cf = data;
> -	GSList *l;
> -	DBusMessage *reply;
>  
>  	if (error->type != OFONO_ERROR_TYPE_NO_ERROR) {
>  		ofono_error("Setting succeeded, but query failed");
>  		cf->flags &= ~CALL_FORWARDING_FLAG_CACHED;
> -		reply = __ofono_error_failed(cf->pending);
> -		__ofono_dbus_pending_reply(&cf->pending, reply);
> +		__ofono_dbus_pending_reply(&cf->pending,
> +					__ofono_error_failed(cf->pending));

This looks fine...

>  		return;
>  	}
>  
> -	if (cf->query_next == cf->query_end) {
> -		reply = dbus_message_new_method_return(cf->pending);
> -		__ofono_dbus_pending_reply(&cf->pending, reply);
> -	}
> -

Actually this part is here on purpose, we want to generate a reply
before we generate a signal for PropertyChanged.

> -	l = cf_cond_list_create(total, list);
> -	set_new_cond_list(cf, cf->query_next, l);
> +	set_new_cond_list(cf, cf->query_next, cf_cond_list_create(total, list));
>  
>  	DBG("%s conditions:", cf_type_lut[cf->query_next]);
> -	cf_cond_list_print(l);
> +	cf_cond_list_print(cf->cf_conditions[cf->query_next]);
>  
> -	if (cf->query_next != cf->query_end) {
> -		cf->query_next++;
> -		set_query_next_cf_cond(cf);
> +	if (cf->query_next == cf->query_end) {
> +		__ofono_dbus_pending_reply(&cf->pending,
> +				dbus_message_new_method_return(cf->pending));
> +		return;
>  	}
> +
> +	cf->query_next++;
> +	set_query_next_cf_cond(cf);
>  }
>  
>  static void set_query_next_cf_cond(struct ofono_call_forwarding *cf)

Regards,
-Denis

Re: [PATCHv3 06/12] call-forwarding: Don't run conditional queries if cfu is active

[Denis Kenzior]

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

Hi Oleg,

On 03/13/2012 08:46 AM, Oleg Zhurakivskyy wrote:
> ---
>  src/call-forwarding.c |   19 ++++++++++---------
>  1 files changed, 10 insertions(+), 9 deletions(-)
> 
> diff --git a/src/call-forwarding.c b/src/call-forwarding.c
> index 7e5edcb..ad5050b 100644
> --- a/src/call-forwarding.c
> +++ b/src/call-forwarding.c
> @@ -525,19 +525,18 @@ static void get_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 (cf->query_next == CALL_FORWARDING_TYPE_NOT_REACHABLE)
> -			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);
> +	if (cf->query_next++ < cf->query_end) {
> +		get_query_next_cf_cond(cf);
>  		return;
>  	}
>  
> -	cf->query_next++;
> -	get_query_next_cf_cond(cf);
> +	if (error->type == OFONO_ERROR_TYPE_NO_ERROR)
> +		cf->flags |= CALL_FORWARDING_FLAG_CACHED;
> +
> +	__ofono_dbus_pending_reply(&cf->pending,
> +				cf_get_properties_reply(cf->pending, cf));
>  }
>  
>  static inline void get_query_next_cf_cond(struct ofono_call_forwarding *cf)
> @@ -565,7 +564,9 @@ static DBusMessage *cf_get_properties(DBusConnection *conn, DBusMessage *msg,
>  
>  	cf->pending = dbus_message_ref(msg);
>  	cf->query_next = 0;
> -
> +	cf->query_end = is_cfu_enabled(cf) ?
> +					CALL_FORWARDING_TYPE_UNCONDITIONAL :
> +					CALL_FORWARDING_TYPE_NOT_REACHABLE;
>  	get_query_next_cf_cond(cf);
>  
>  	return NULL;

What if we're running GetProperties for the first time and CFU is active
at the network, but for some reason (e.g. driver doesn't implement it)
the SIM call forwarding rules are absent.  Won't we query conditional
CFs needlessly here?

Regards,
-Denis

Re: [PATCHv3 09/12] call-forwarding: Cache cfs on CFU removal

[Denis Kenzior]

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

Hi Oleg,

On 03/13/2012 08:46 AM, Oleg Zhurakivskyy wrote:
> Since the re-querying is done, cache the call forwardings.
> ---
>  src/call-forwarding.c |   14 +++++++++++---
>  1 files changed, 11 insertions(+), 3 deletions(-)
> 
> diff --git a/src/call-forwarding.c b/src/call-forwarding.c
> index 575d96d..7109c21 100644
> --- a/src/call-forwarding.c
> +++ b/src/call-forwarding.c
> @@ -38,6 +38,7 @@
>  
>  #define CALL_FORWARDING_FLAG_CACHED	0x1
>  #define CALL_FORWARDING_FLAG_CPHS_CFF	0x2
> +#define CALL_FORWARDING_FLAG_CACHE	0x4
>  

You really have to pick a better name for this flag, I really don't
understand its purpose right now.

>  /* According to 27.007 Spec */
>  #define DEFAULT_NO_REPLY_TIMEOUT 20
> @@ -635,7 +636,8 @@ static void set_query_cf_callback(const struct ofono_error *error, int total,
>  
>  	if (error->type != OFONO_ERROR_TYPE_NO_ERROR) {
>  		ofono_error("Setting succeeded, but query failed");
> -		cf->flags &= ~CALL_FORWARDING_FLAG_CACHED;
> +		cf->flags &= ~(CALL_FORWARDING_FLAG_CACHED |
> +				CALL_FORWARDING_FLAG_CACHE);
>  		__ofono_dbus_pending_reply(&cf->pending,
>  					__ofono_error_failed(cf->pending));
>  		return;
> @@ -655,6 +657,11 @@ static void set_query_cf_callback(const struct ofono_error *error, int total,
>  		cf->flags ^= CALL_FORWARDING_FLAG_CACHED;
>  
>  	if (cf->query_next == cf->query_end) {
> +		if (cf->flags & CALL_FORWARDING_FLAG_CACHE) {
> +			cf->flags &= ~CALL_FORWARDING_FLAG_CACHE;
> +			cf->flags |= CALL_FORWARDING_FLAG_CACHED;
> +		}
> +
>  		__ofono_dbus_pending_reply(&cf->pending,
>  				dbus_message_new_method_return(cf->pending));
>  		return;
> @@ -701,12 +708,13 @@ static DBusMessage *set_property_request(struct ofono_call_forwarding *cf,
>  	cf->query_next = type;
>  
>  	if (type == CALL_FORWARDING_TYPE_UNCONDITIONAL &&
> -		ph->number[0] == '\0' && is_cfu_enabled(cf))
> +		ph->number[0] == '\0' && is_cfu_enabled(cf)) {
>  		/*
>  		 * CFU is removed, conditionals need to be updated
>  		 */
>  		cf->query_end = CALL_FORWARDING_TYPE_NOT_REACHABLE;
> -	else
> +		cf->flags |= CALL_FORWARDING_FLAG_CACHE;
> +	} else
>  		cf->query_end = type;
>  
>  	DBG("Farming off request, will be erasure: %d", ph->number[0] == '\0');

Regards,
-Denis

Re: [PATCHv3 03/12] call-forwarding: Refactor cf_condition_find_with_cls() slightly

[Oleg Zhurakivskyy]

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

Hello Denis,

On 03/21/2012 01:50 AM, Denis Kenzior wrote:
>> +	return CFC(a)->cls - CFC(b)->cls;
>
> You're breaking const-correctness here.

Thanks for pointing this. The intention was to save a few instructions here, but 
just missed that.

> Also, why do you rename the function?  I do think we should have
> 'condition' or at least 'cond' in the name to make it clear what it
> does.  My vote is for cf_cond_compare...

Thanks, I agree, cf_cond_compare() would be more consistent and clear.

>> +static struct ofono_call_forwarding_condition *cf_find(GSList *l, int cls)
>
> Same as above, at least name it cf_cond_find.  Also, I'd prefer changes
> to this function to be in a separate patch.

OK.

>> -static int cf_find_timeout(GSList *cf_list, int cls)
>> +static int cf_find_timeout(GSList *l, int cls)
>
> Same here, lets name this cf_cond_find_timeout

OK.

>> -static void cf_cond_list_print(GSList *list)
>> +static void cf_cond_list_print(GSList *l)
>>   {
>> -	GSList *l;
>> -	struct ofono_call_forwarding_condition *cond;
>> -
>> -	for (l = list; l; l = l->next) {
>> -		cond = l->data;
>> +	struct ofono_call_forwarding_condition *c;
>>
>> +	for (; l&&  (c = l->data); l = l->next)
>
> After going back and forth on this one, I'm actually against this style.
>   Mainly it is too hard to notice the assignment, inside the continuation
> check expression.  I'd really prefer that we assign c inside the for
> loop block.  Also, l->data should never be NULL, so in this case I'd
> rather crash and find out early than inadvertently covering it up.

OK, thanks. I agree on both points here.

>> +			number = new_cfu ? "" : phone_number_to_string(
>>   							&lc->phone_number);
>
> This part really belongs in a separate patch.

OK.

Thanks for the help, I will correct all these.

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

Re: [PATCHv3 04/12] call-forwarding: Refactor cf_find_unconditional()

[Oleg Zhurakivskyy]

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

Hello Denis,

On 03/21/2012 01:51 AM, Denis Kenzior wrote:
>> +#define cf_find_unconditional(_cf)					\
>> +({									\
>> +	cf_find((_cf)->cf_conditions[CALL_FORWARDING_TYPE_UNCONDITIONAL],\
>> +		BEARER_CLASS_VOICE);					\
>> +})
>> +
>
> Actually I'd prefer if we made this an inline function rather than a
> macro.  A bit easier to read that way.

Sure, strongly agree. I was going to do this myself, it just didn't make it into 
these patches.

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

Re: [PATCHv3 05/12] call-forwarding: Minor cleanup of set_query_cf_callback

[Oleg Zhurakivskyy]

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

Hello Denis,

On 03/21/2012 01:54 AM, Denis Kenzior wrote:
>> -	if (cf->query_next == cf->query_end) {
>> -		reply = dbus_message_new_method_return(cf->pending);
>> -		__ofono_dbus_pending_reply(&cf->pending, reply);
>> -	}
>> -
>
> Actually this part is here on purpose, we want to generate a reply
> before we generate a signal for PropertyChanged.

Thanks for the help here, it really missed my attention.

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

Re: [PATCHv3 06/12] call-forwarding: Don't run conditional queries if cfu is active

[Oleg Zhurakivskyy]

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

Hello Denis,

On 03/21/2012 02:09 AM, Denis Kenzior wrote:
>>   static inline void get_query_next_cf_cond(struct ofono_call_forwarding *cf)
>> @@ -565,7 +564,9 @@ static DBusMessage *cf_get_properties(DBusConnection *conn, DBusMessage *msg,
>>
>>   	cf->pending = dbus_message_ref(msg);
>>   	cf->query_next = 0;
>> -
>> +	cf->query_end = is_cfu_enabled(cf) ?
>> +					CALL_FORWARDING_TYPE_UNCONDITIONAL :
>> +					CALL_FORWARDING_TYPE_NOT_REACHABLE;
>>   	get_query_next_cf_cond(cf);
>>
>>   	return NULL;
>
> What if we're running GetProperties for the first time and CFU is active
> at the network, but for some reason (e.g. driver doesn't implement it)
> the SIM call forwarding rules are absent.  Won't we query conditional
> CFs needlessly here?

Good point and thanks for pointing this. I will correct so, that once the CFU is 
discovered and active, the conditional queries will be skipped.

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

Re: [PATCHv3 09/12] call-forwarding: Cache cfs on CFU removal

[Oleg Zhurakivskyy]

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

Hello Denis,

On 03/21/2012 02:16 AM, Denis Kenzior wrote:
>> +#define CALL_FORWARDING_FLAG_CACHE	0x4
>
> You really have to pick a better name for this flag, I really don't
> understand its purpose right now.

The intention of this flag was to distinguish the setting of the single CF 
condition from the CFU deactivation were the conditional queries will be re-run 
and cached. But, after having another look at this, you are correct, such flag 
isn't needed. I will be glad to get rid of it, the naming is really confusing.

Thanks for reviewing and help! I will fix all these and prepare another patchset.

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