[PATCHv4 00/13] Call forwarding state handling change

[PATCHv4 00/13] Call forwarding state handling change

[Oleg Zhurakivskyy]

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

Hello,

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

Changes from v3:

- Re-run conditional queries on cfu removal and mark cached.
- End querying once cfu is active.
- Handle the caching on supplementary services path
  cfs modifications.

Regards,
Oleg

Oleg Zhurakivskyy (13):
  call-forwarding: Refactor cf_condition_compare()
  call-forwarding: Refactor cf_condition_find_with_cls()
  call-forwarding: Get rid of extra variable
  call-forwarding: Streamline number assignment
  call-forwarding: Streamline cf_find_timeout() logic
  call-forwarding: Refactor cf_find_unconditional()
  call-forwarding: Streamline set_query_cf_callback()
  call-forwarding: Remove unneeded variable
  call-forwarding: End querying once cfu is active
  call-forwarding: CFU unset, update conditionals
  call-forwarding: Re-run ss path queries on CFU unset
  call-forwarding: Cache ss TYPE_ALL modifications
  TODO: Remove completed call forwarding state task

 TODO                  |   17 -----
 src/call-forwarding.c |  168 ++++++++++++++++++++-----------------------------
 2 files changed, 68 insertions(+), 117 deletions(-)

-- 
1.7.5.4

[PATCHv4 01/13] call-forwarding: Refactor cf_condition_compare()

[Oleg Zhurakivskyy]

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

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

diff --git a/src/call-forwarding.c b/src/call-forwarding.c
index 8b6659a..2b91250 100644
--- a/src/call-forwarding.c
+++ b/src/call-forwarding.c
@@ -86,18 +86,12 @@ 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_cond_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 ca->cls - cb->cls;
 }
 
 static gint cf_condition_find_with_cls(gconstpointer a, gconstpointer b)
@@ -175,8 +169,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_cond_compare);
 		}
 	}
 
-- 
1.7.5.4

[PATCHv4 02/13] call-forwarding: Refactor cf_condition_find_with_cls()

[Oleg Zhurakivskyy]

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

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

diff --git a/src/call-forwarding.c b/src/call-forwarding.c
index 2b91250..beae53b 100644
--- a/src/call-forwarding.c
+++ b/src/call-forwarding.c
@@ -94,33 +94,25 @@ static gint cf_cond_compare(gconstpointer a, gconstpointer b)
 	return ca->cls - cb->cls;
 }
 
-static gint cf_condition_find_with_cls(gconstpointer a, gconstpointer b)
+static struct ofono_call_forwarding_condition *cf_cond_find(GSList *l, int cls)
 {
-	const struct ofono_call_forwarding_condition *c = a;
-	int cls = GPOINTER_TO_INT(b);
+	for (; l; l = l->next)
+		if (((struct ofono_call_forwarding_condition *)
+				(l->data))->cls == cls)
+			return l->data;
 
-	if (c->cls < cls)
-		return -1;
-
-	if (c->cls > cls)
-		return 1;
-
-	return 0;
+	return NULL;
 }
 
 static int cf_find_timeout(GSList *cf_list, 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);
+	c = cf_cond_find(cf_list, cls);
 
-	if (l == NULL)
+	if (c == NULL)
 		return DEFAULT_NO_REPLY_TIMEOUT;
 
-	c = l->data;
-
 	return c->time;
 }
 
@@ -323,12 +315,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_cond_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)) {
@@ -349,8 +337,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);
 
@@ -428,19 +416,14 @@ 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_cond_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(
 							&lc->phone_number);
 			}
 
@@ -789,7 +772,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;
@@ -802,15 +784,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_cond_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

[PATCHv4 03/13] call-forwarding: Get rid of extra variable

[Oleg Zhurakivskyy]

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

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

diff --git a/src/call-forwarding.c b/src/call-forwarding.c
index beae53b..cda54e7 100644
--- a/src/call-forwarding.c
+++ b/src/call-forwarding.c
@@ -116,12 +116,11 @@ static int cf_find_timeout(GSList *cf_list, int cls)
 	return c->time;
 }
 
-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) {
+	for (; l ; l = l->next) {
 		cond = l->data;
 
 		DBG("CF Condition status: %d, class: %d, number: %s,"
-- 
1.7.5.4

[PATCHv4 04/13] call-forwarding: Streamline number assignment

[Oleg Zhurakivskyy]

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

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

diff --git a/src/call-forwarding.c b/src/call-forwarding.c
index cda54e7..5e68bba 100644
--- a/src/call-forwarding.c
+++ b/src/call-forwarding.c
@@ -420,11 +420,8 @@ static void set_new_cond_list(struct ofono_call_forwarding *cf,
 			if (lc == NULL)
 				continue;
 
-			if (new_cfu)
-				number = "";
-			else {
+			number = new_cfu ? "" : phone_number_to_string(
 							&lc->phone_number);
-			}
 
 			ofono_dbus_signal_property_changed(conn, path,
 						OFONO_CALL_FORWARDING_INTERFACE,
-- 
1.7.5.4

[PATCHv4 05/13] call-forwarding: Streamline cf_find_timeout() logic

[Oleg Zhurakivskyy]

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

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

diff --git a/src/call-forwarding.c b/src/call-forwarding.c
index 5e68bba..424b111 100644
--- a/src/call-forwarding.c
+++ b/src/call-forwarding.c
@@ -104,16 +104,11 @@ static struct ofono_call_forwarding_condition *cf_cond_find(GSList *l, int cls)
 	return NULL;
 }
 
-static int cf_find_timeout(GSList *cf_list, int cls)
+static int cf_cond_find_timeout(GSList *l, int cls)
 {
-	struct ofono_call_forwarding_condition *c;
+	struct ofono_call_forwarding_condition *cond = cf_cond_find(l, cls);
 
-	c = cf_cond_find(cf_list, cls);
-
-	if (c == NULL)
-		return DEFAULT_NO_REPLY_TIMEOUT;
-
-	return c->time;
+	return cond ? cond->time : DEFAULT_NO_REPLY_TIMEOUT;
 }
 
 static void cf_cond_list_print(GSList *l)
@@ -813,7 +808,7 @@ static DBusMessage *cf_set_property(DBusConnection *conn, DBusMessage *msg,
 		if (number[0] != '\0')
 			string_to_phone_number(number, &ph);
 
-		timeout = cf_find_timeout(cf->cf_conditions[type], cls);
+		timeout = cf_cond_find_timeout(cf->cf_conditions[type], cls);
 
 		return set_property_request(cf, msg, type, cls, &ph,
 						timeout);
-- 
1.7.5.4

[PATCHv4 06/13] call-forwarding: Refactor cf_find_unconditional()

[Oleg Zhurakivskyy]

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

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

diff --git a/src/call-forwarding.c b/src/call-forwarding.c
index 424b111..49398c9 100644
--- a/src/call-forwarding.c
+++ b/src/call-forwarding.c
@@ -193,26 +193,12 @@ 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(
+static inline 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;
+	return cf_cond_find(
+		cf->cf_conditions[CALL_FORWARDING_TYPE_UNCONDITIONAL],
+		BEARER_CLASS_VOICE);
 }
 
 static void sim_set_cf_indicator(struct ofono_call_forwarding *cf)
-- 
1.7.5.4

[PATCHv4 07/13] call-forwarding: Streamline set_query_cf_callback()

[Oleg Zhurakivskyy]

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

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

diff --git a/src/call-forwarding.c b/src/call-forwarding.c
index 49398c9..20a510b 100644
--- a/src/call-forwarding.c
+++ b/src/call-forwarding.c
@@ -639,32 +639,29 @@ 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);
-	}
+	if (cf->query_next == cf->query_end)
+		__ofono_dbus_pending_reply(&cf->pending,
+				dbus_message_new_method_return(cf->pending));
 
-	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)
+		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

[PATCHv4 08/13] call-forwarding: Remove unneeded variable

[Oleg Zhurakivskyy]

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

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

diff --git a/src/call-forwarding.c b/src/call-forwarding.c
index 20a510b..ba52609 100644
--- a/src/call-forwarding.c
+++ b/src/call-forwarding.c
@@ -539,8 +539,8 @@ static void get_query_cf_callback(const struct ofono_error *error, int total,
 	}
 
 	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

[PATCHv4 09/13] call-forwarding: End querying once cfu is active

[Oleg Zhurakivskyy]

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

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

diff --git a/src/call-forwarding.c b/src/call-forwarding.c
index ba52609..786d163 100644
--- a/src/call-forwarding.c
+++ b/src/call-forwarding.c
@@ -533,12 +533,14 @@ 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)
+	if (cf->query_next == CALL_FORWARDING_TYPE_NOT_REACHABLE ||
+			(cf->query_next == CALL_FORWARDING_TYPE_UNCONDITIONAL &&
+			is_cfu_enabled(cf))) {
+		if (error->type == OFONO_ERROR_TYPE_NO_ERROR)
 			cf->flags |= CALL_FORWARDING_FLAG_CACHED;
-	}
 
-	if (cf->query_next == CALL_FORWARDING_TYPE_NOT_REACHABLE) {
 		__ofono_dbus_pending_reply(&cf->pending,
 				cf_get_properties_reply(cf->pending, cf));
 		return;
-- 
1.7.5.4

[PATCHv4 10/13] call-forwarding: CFU unset, update conditionals

[Oleg Zhurakivskyy]

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

Also set the cached flag on CFU activation and
re-querying of the conditionals.
---
 src/call-forwarding.c |   10 ++++++++++
 1 files changed, 10 insertions(+), 0 deletions(-)

diff --git a/src/call-forwarding.c b/src/call-forwarding.c
index 786d163..aa1ece7 100644
--- a/src/call-forwarding.c
+++ b/src/call-forwarding.c
@@ -659,6 +659,16 @@ 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]);
 
+	if (cf->query_next == CALL_FORWARDING_TYPE_UNCONDITIONAL) {
+		cf->flags |= CALL_FORWARDING_FLAG_CACHED;
+
+		/*
+		 * CFU has been disabled, conditionals need to be updated
+		 */
+		if (is_cfu_enabled(cf) == FALSE)
+			cf->query_end = CALL_FORWARDING_TYPE_NOT_REACHABLE;
+	}
+
 	if (cf->query_next == cf->query_end)
 		return;
 
-- 
1.7.5.4

[PATCHv4 11/13] call-forwarding: Re-run ss path queries on CFU unset

[Oleg Zhurakivskyy]

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

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

diff --git a/src/call-forwarding.c b/src/call-forwarding.c
index aa1ece7..204ecc7 100644
--- a/src/call-forwarding.c
+++ b/src/call-forwarding.c
@@ -1004,6 +1004,16 @@ static void ss_set_query_cf_callback(const struct ofono_error *error, int total,
 
 	set_new_cond_list(cf, cf->query_next, l);
 
+	if (cf->query_next == CALL_FORWARDING_TYPE_UNCONDITIONAL &&
+			cf->query_next == cf->query_end) {
+		cf->flags |= CALL_FORWARDING_FLAG_CACHED;
+		/*
+		 * CFU has been disabled, conditionals need to be updated
+		 */
+		if (is_cfu_enabled(cf) == FALSE)
+			cf->query_end = CALL_FORWARDING_TYPE_NOT_REACHABLE;
+	}
+
 	if (cf->query_next != cf->query_end) {
 		cf->query_next++;
 		ss_set_query_next_cf_cond(cf);
-- 
1.7.5.4

[PATCHv4 12/13] call-forwarding: Cache ss TYPE_ALL modifications

[Oleg Zhurakivskyy]

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

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

diff --git a/src/call-forwarding.c b/src/call-forwarding.c
index 204ecc7..a22fb1f 100644
--- a/src/call-forwarding.c
+++ b/src/call-forwarding.c
@@ -1184,6 +1184,7 @@ static gboolean cf_ss_control(int type, const char *sc,
 
 	switch (cf->ss_req->cf_type) {
 	case CALL_FORWARDING_TYPE_ALL:
+		cf->flags |= CALL_FORWARDING_FLAG_CACHED;
 		cf->query_next = CALL_FORWARDING_TYPE_UNCONDITIONAL;
 		cf->query_end = CALL_FORWARDING_TYPE_NOT_REACHABLE;
 		break;
-- 
1.7.5.4

[PATCHv4 13/13] 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: [PATCHv4 01/13] call-forwarding: Refactor cf_condition_compare()

[Denis Kenzior]

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

On 04/10/2012 07:17 AM, Oleg Zhurakivskyy wrote:
> ---
>  src/call-forwarding.c |   13 +++----------
>  1 files changed, 3 insertions(+), 10 deletions(-)
> 

Patch has been applied, thanks.

Regards,
-Denis

Re: [PATCHv4 02/13] call-forwarding: Refactor cf_condition_find_with_cls()

[Denis Kenzior]

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

Hi Oleg,

Patch has been applied, however see comments below:

<snip>

> +static struct ofono_call_forwarding_condition *cf_cond_find(GSList *l, int cls)
>  {
> -	const struct ofono_call_forwarding_condition *c = a;
> -	int cls = GPOINTER_TO_INT(b);
> +	for (; l; l = l->next)
> +		if (((struct ofono_call_forwarding_condition *)
> +				(l->data))->cls == cls)

I don't really find all this casting more readable, so I changed this
back to more or less the old way of assigning an
ofono_call_forwarding_condition variable first.  The parentheses make my
head hurt, so lets avoid such constructs in the future.

<snip>

>  			if (new_cfu)
>  				number = "";
>  			else {
> -				lc = l->data;
> -
> -				number = phone_number_to_string(

Something went wrong with your merge here, so I had to fix this manually
and amend your patch.

>  							&lc->phone_number);
>  			}
>  

Regards,
-Denis

Re: [PATCHv4 03/13] call-forwarding: Get rid of extra variable

[Denis Kenzior]

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

Hi Oleg,

On 04/10/2012 07:17 AM, Oleg Zhurakivskyy wrote:
> ---
>  src/call-forwarding.c |    5 ++---
>  1 files changed, 2 insertions(+), 3 deletions(-)
> 

Patch has been applied, thanks.

Regards,
-Denis

Re: [PATCHv4 04/13] call-forwarding: Streamline number assignment

[Denis Kenzior]

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

Hi Oleg,

On 04/10/2012 07:17 AM, Oleg Zhurakivskyy wrote:
> ---
>  src/call-forwarding.c |    5 +----
>  1 files changed, 1 insertions(+), 4 deletions(-)
> 

Due to the amend in patch 2, this patch no longer applies. Feel free to
resend it, though the current code is also just fine.

Regards,
-Denis

Re: [PATCHv4 05/13] call-forwarding: Streamline cf_find_timeout() logic

[Denis Kenzior]

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

Hi Oleg,

On 04/10/2012 07:17 AM, Oleg Zhurakivskyy wrote:
> ---
>  src/call-forwarding.c |   13 ++++---------
>  1 files changed, 4 insertions(+), 9 deletions(-)
> 

Patch has been applied, thanks.

Regards,
-Denis

Re: [PATCHv4 06/13] call-forwarding: Refactor cf_find_unconditional()

[Denis Kenzior]

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

Hi Oleg,

On 04/10/2012 07:17 AM, Oleg Zhurakivskyy wrote:
> ---
>  src/call-forwarding.c |   22 ++++------------------
>  1 files changed, 4 insertions(+), 18 deletions(-)
> 

Patch has been applied, thanks.

Regards,
-Denis

Re: [PATCHv4 07/13] call-forwarding: Streamline set_query_cf_callback()

[Denis Kenzior]

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

Hi Oleg,

On 04/10/2012 07:17 AM, Oleg Zhurakivskyy wrote:
> ---
>  src/call-forwarding.c |   27 ++++++++++++---------------
>  1 files changed, 12 insertions(+), 15 deletions(-)
> 

Patch has been applied, thanks.

Regards,
-Denis

Re: [PATCHv4 08/13] call-forwarding: Remove unneeded variable

[Denis Kenzior]

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

Hi Oleg,

On 04/10/2012 07:17 AM, Oleg Zhurakivskyy wrote:
> ---
>  src/call-forwarding.c |    4 ++--
>  1 files changed, 2 insertions(+), 2 deletions(-)
> 

Patch has been applied, thanks.

Regards,
-Denis

Re: [PATCHv4 10/13] call-forwarding: CFU unset, update conditionals

[Denis Kenzior]

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

Hi Oleg,

On 04/10/2012 07:17 AM, Oleg Zhurakivskyy wrote:
> Also set the cached flag on CFU activation and
> re-querying of the conditionals.
> ---
>  src/call-forwarding.c |   10 ++++++++++
>  1 files changed, 10 insertions(+), 0 deletions(-)
> 
> diff --git a/src/call-forwarding.c b/src/call-forwarding.c
> index 786d163..aa1ece7 100644
> --- a/src/call-forwarding.c
> +++ b/src/call-forwarding.c
> @@ -659,6 +659,16 @@ 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]);
>  
> +	if (cf->query_next == CALL_FORWARDING_TYPE_UNCONDITIONAL) {
> +		cf->flags |= CALL_FORWARDING_FLAG_CACHED;

I'm not quite following why we need to set this flag here?

> +
> +		/*
> +		 * CFU has been disabled, conditionals need to be updated
> +		 */
> +		if (is_cfu_enabled(cf) == FALSE)
> +			cf->query_end = CALL_FORWARDING_TYPE_NOT_REACHABLE;

Would it make more sense to make this decision in set_property_request()?

> +	}
> +
>  	if (cf->query_next == cf->query_end)
>  		return;
>  

Regards,
-Denis

Re: [PATCHv4 09/13] call-forwarding: End querying once cfu is active

[Denis Kenzior]

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

Hi Oleg,

On 04/10/2012 07:17 AM, Oleg Zhurakivskyy wrote:
> ---
>  src/call-forwarding.c |    8 +++++---
>  1 files changed, 5 insertions(+), 3 deletions(-)
> 

This patch looks fine to me, but I'd like to wait until the rest is
ready before applying it.

Regards,
-Denis

Re: [PATCHv4 11/13] call-forwarding: Re-run ss path queries on CFU unset

[Denis Kenzior]

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

Hi Oleg,

On 04/10/2012 07:17 AM, Oleg Zhurakivskyy wrote:
> ---
>  src/call-forwarding.c |   10 ++++++++++
>  1 files changed, 10 insertions(+), 0 deletions(-)
> 
> diff --git a/src/call-forwarding.c b/src/call-forwarding.c
> index aa1ece7..204ecc7 100644
> --- a/src/call-forwarding.c
> +++ b/src/call-forwarding.c
> @@ -1004,6 +1004,16 @@ static void ss_set_query_cf_callback(const struct ofono_error *error, int total,
>  
>  	set_new_cond_list(cf, cf->query_next, l);
>  
> +	if (cf->query_next == CALL_FORWARDING_TYPE_UNCONDITIONAL &&
> +			cf->query_next == cf->query_end) {
> +		cf->flags |= CALL_FORWARDING_FLAG_CACHED;
> +		/*
> +		 * CFU has been disabled, conditionals need to be updated
> +		 */
> +		if (is_cfu_enabled(cf) == FALSE)
> +			cf->query_end = CALL_FORWARDING_TYPE_NOT_REACHABLE;

So a bit of background, the original set + query logic did not mess with
the CACHED flag.  The assumption was that we're modifying a single
property.  If the CACHED flag was already set, then the modification was
queried and the CACHED flag was still valid.  If the CACHED flag was not
set, then we'd re-query the entire thing anyway.

Now we have a somewhat funny situation where when we clear CFU, we are
essentially forced into querying everything.  The immediate problem with
your approach is that we can't return from the method call until all
settings have been queried.  By convention the core can have only a
single outstanding call into the driver at a time.  We bend the rules
somewhat, but in general we need to stick to this rule.  This is why you
see busy error conditions everywhere.  So likely this needs a specific
code path ...

Also, there is an optimization we can make here, e.g. if we queried the
conditional forwarding settings prior to CFU being enabled, then we can
keep those around.  This is why the TODO item refers to the 'conditional
cache.'  In the case of CFU being flipped to enabled and then disabled,
we do not need to query.

> +	}
> +
>  	if (cf->query_next != cf->query_end) {
>  		cf->query_next++;
>  		ss_set_query_next_cf_cond(cf);

Regards,
-Denis

Re: [PATCHv4 12/13] call-forwarding: Cache ss TYPE_ALL modifications

[Denis Kenzior]

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

Hi Oleg,

On 04/10/2012 07:17 AM, Oleg Zhurakivskyy wrote:
> ---
>  src/call-forwarding.c |    1 +
>  1 files changed, 1 insertions(+), 0 deletions(-)
> 
> diff --git a/src/call-forwarding.c b/src/call-forwarding.c
> index 204ecc7..a22fb1f 100644
> --- a/src/call-forwarding.c
> +++ b/src/call-forwarding.c
> @@ -1184,6 +1184,7 @@ static gboolean cf_ss_control(int type, const char *sc,
>  
>  	switch (cf->ss_req->cf_type) {
>  	case CALL_FORWARDING_TYPE_ALL:
> +		cf->flags |= CALL_FORWARDING_FLAG_CACHED;
>  		cf->query_next = CALL_FORWARDING_TYPE_UNCONDITIONAL;
>  		cf->query_end = CALL_FORWARDING_TYPE_NOT_REACHABLE;
>  		break;

I don't believe this would work for non-queries.  For example, lets say
the user runs a registration operation on CALL_FORWARDING_TYPE_ALL and
that fails for whatever reason.  Now the cached flag is set (perhaps
erroneously)

Regards,
-Denis

Re: [PATCHv4 11/13] call-forwarding: Re-run ss path queries on CFU unset

[Denis Kenzior]

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

Hi Oleg,

On 04/25/2012 05:59 AM, Oleg Zhurakivskyy wrote:
> Hello Denis,
> 
> On 04/24/2012 12:28 AM, Denis Kenzior wrote:
>>> static void ss_set_query_cf_callback(const struct ofono_error *error,
>>> int total,
>>>
>>>       set_new_cond_list(cf, cf->query_next, l);
>>>
>>> +    if (cf->query_next == CALL_FORWARDING_TYPE_UNCONDITIONAL&&
>>> +            cf->query_next == cf->query_end) {
>>> +        cf->flags |= CALL_FORWARDING_FLAG_CACHED;
>>> +        /*
>>> +         * CFU has been disabled, conditionals need to be updated
>>> +         */
>>> +        if (is_cfu_enabled(cf) == FALSE)
>>> +            cf->query_end = CALL_FORWARDING_TYPE_NOT_REACHABLE;
>>
>> So a bit of background, the original set + query logic did not mess with
>> the CACHED flag.  The assumption was that we're modifying a single
>> property.  If the CACHED flag was already set, then the modification was
>> queried and the CACHED flag was still valid.  If the CACHED flag was not
>> set, then we'd re-query the entire thing anyway.
>>
>> Now we have a somewhat funny situation where when we clear CFU, we are
>> essentially forced into querying everything.  The immediate problem with
>> your approach is that we can't return from the method call until all
>> settings have been queried.  By convention the core can have only a
>> single outstanding call into the driver at a time.  We bend the rules
>> somewhat, but in general we need to stick to this rule.  This is why you
>> see busy error conditions everywhere.  So likely this needs a specific
>> code path ...
> 
> OK, thanks, I see. Would it make sense to return the method immediately
> and then to re-query the conditionals? Or, just to wait until anybody
> needs them?

This probably needs a bit of thought, but here's one possible strategy,
feel free to suggest any improvements:

- If unconditional is reset and conditionals are known (e.g. they were
queried before and not cleared) then we can simply signal them here

- If the unconditional is reset and the CACHED flag is not set (e.g. the
application didn't trigger a GetProperties yet) then we probably can
skip the re-query, the next GetProperties will do it for us.

- If unconditional is reset and CACHED is set but we don't know the
conditionals, then we should query all conditionals before returning
from the method call.

> 
>> Also, there is an optimization we can make here, e.g. if we queried the
>> conditional forwarding settings prior to CFU being enabled, then we can
>> keep those around.  This is why the TODO item refers to the 'conditional
>> cache.'  In the case of CFU being flipped to enabled and then disabled,
>> we do not need to query.
> 
> Thanks for the help here. Let's go for this approach too.

Okay, just remember the conditionals can be erased when CFU is active,
including through MMI codes.

Regards,
-Denis

Re: [PATCHv4 09/13] call-forwarding: End querying once cfu is active

[Oleg Zhurakivskyy]

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

Hello Denis,

On 04/24/2012 12:12 AM, Denis Kenzior wrote:
> This patch looks fine to me, but I'd like to wait until the rest is
> ready before applying it.

Sure, let's do so.

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

Re: [PATCHv4 10/13] call-forwarding: CFU unset, update conditionals

[Oleg Zhurakivskyy]

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

Hello Denis,

On 04/24/2012 12:06 AM, Denis Kenzior wrote:
>>static void set_query_cf_callback(const struct ofono_error *error, int total,
[...]
>> +	if (cf->query_next == CALL_FORWARDING_TYPE_UNCONDITIONAL) {
>> +		cf->flags |= CALL_FORWARDING_FLAG_CACHED;
>
> I'm not quite following why we need to set this flag here?

The intention was to cache for the cases we can:

- CFU is set.
- CFU is unset (this also triggers the re-querying of conditionals).

If the logic is correct, perhaps, I could document this better. It probably 
needs the in place comment.

>> +		/*
>> +		 * CFU has been disabled, conditionals need to be updated
>> +		 */
>> +		if (is_cfu_enabled(cf) == FALSE)
>> +			cf->query_end = CALL_FORWARDING_TYPE_NOT_REACHABLE;
>
> Would it make more sense to make this decision in set_property_request()?

Sure, this makes sense and will save a bit. Initially I placed the check there, 
but then tried to optimize for some corner case. Just forgot to move it back 
after reversing that optimization. Sorry, my fault.

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

Re: [PATCHv4 11/13] call-forwarding: Re-run ss path queries on CFU unset

[Oleg Zhurakivskyy]

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

Hello Denis,

On 04/24/2012 12:28 AM, Denis Kenzior wrote:
>> static void ss_set_query_cf_callback(const struct ofono_error *error, int total,
>>
>>   	set_new_cond_list(cf, cf->query_next, l);
>>
>> +	if (cf->query_next == CALL_FORWARDING_TYPE_UNCONDITIONAL&&
>> +			cf->query_next == cf->query_end) {
>> +		cf->flags |= CALL_FORWARDING_FLAG_CACHED;
>> +		/*
>> +		 * CFU has been disabled, conditionals need to be updated
>> +		 */
>> +		if (is_cfu_enabled(cf) == FALSE)
>> +			cf->query_end = CALL_FORWARDING_TYPE_NOT_REACHABLE;
>
> So a bit of background, the original set + query logic did not mess with
> the CACHED flag.  The assumption was that we're modifying a single
> property.  If the CACHED flag was already set, then the modification was
> queried and the CACHED flag was still valid.  If the CACHED flag was not
> set, then we'd re-query the entire thing anyway.
>
> Now we have a somewhat funny situation where when we clear CFU, we are
> essentially forced into querying everything.  The immediate problem with
> your approach is that we can't return from the method call until all
> settings have been queried.  By convention the core can have only a
> single outstanding call into the driver at a time.  We bend the rules
> somewhat, but in general we need to stick to this rule.  This is why you
> see busy error conditions everywhere.  So likely this needs a specific
> code path ...

OK, thanks, I see. Would it make sense to return the method immediately and then 
to re-query the conditionals? Or, just to wait until anybody needs them?

> Also, there is an optimization we can make here, e.g. if we queried the
> conditional forwarding settings prior to CFU being enabled, then we can
> keep those around.  This is why the TODO item refers to the 'conditional
> cache.'  In the case of CFU being flipped to enabled and then disabled,
> we do not need to query.

Thanks for the help here. Let's go for this approach too.

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

Re: [PATCHv4 12/13] call-forwarding: Cache ss TYPE_ALL modifications

[Oleg Zhurakivskyy]

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

Hello Denis,

On 04/24/2012 12:38 AM, Denis Kenzior wrote:
>> static gboolean cf_ss_control(int type, const char *sc,
>>
>>   	switch (cf->ss_req->cf_type) {
>>   	case CALL_FORWARDING_TYPE_ALL:
>> +		cf->flags |= CALL_FORWARDING_FLAG_CACHED;
>>   		cf->query_next = CALL_FORWARDING_TYPE_UNCONDITIONAL;
>>   		cf->query_end = CALL_FORWARDING_TYPE_NOT_REACHABLE;
>>   		break;
>
> I don't believe this would work for non-queries.  For example, lets say
> the user runs a registration operation on CALL_FORWARDING_TYPE_ALL and
> that fails for whatever reason.  Now the cached flag is set (perhaps
> erroneously)

Sorry, simply overlooked this.

Thanks for the help, I will correct the issues and prepare another patchset.

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

Re: [PATCHv4 11/13] call-forwarding: Re-run ss path queries on CFU unset

[Oleg Zhurakivskyy]

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

Hello Denis,

On 04/24/2012 11:21 PM, Denis Kenzior wrote:
>> OK, thanks, I see. Would it make sense to return the method immediately
>> and then to re-query the conditionals? Or, just to wait until anybody
>> needs them?
>
> This probably needs a bit of thought, but here's one possible strategy,
> feel free to suggest any improvements:
>
> - If unconditional is reset and conditionals are known (e.g. they were
> queried before and not cleared) then we can simply signal them here
>
> - If the unconditional is reset and the CACHED flag is not set (e.g. the
> application didn't trigger a GetProperties yet) then we probably can
> skip the re-query, the next GetProperties will do it for us.
>
> - If unconditional is reset and CACHED is set but we don't know the
> conditionals, then we should query all conditionals before returning
> from the method call.

Thanks for the help here. On a first thought, in order to differ 1) from 3) 
might require a flag, but let me check if the specific code path for re-querying 
the conditionals might be cleaner.

>>> Also, there is an optimization we can make here, e.g. if we queried the
>>> conditional forwarding settings prior to CFU being enabled, then we can
>>> keep those around.  This is why the TODO item refers to the 'conditional
>>> cache.'  In the case of CFU being flipped to enabled and then disabled,
>>> we do not need to query.
>>
>> Thanks for the help here. Let's go for this approach too.
>
> Okay, just remember the conditionals can be erased when CFU is active,
> including through MMI codes.

OK, thanks, will check regarding MMI.

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