[PATCH] call-forward: Call forwarding state handling

[PATCH] call-forward: Call forwarding state handling

[Nicolas Bertrand]

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

When CFU is active be cautious with conditional
call-forward activation/deactivation
---
 src/call-forwarding.c |   44 ++++++++++++++++++++++++++++++++++++++------
 1 files changed, 38 insertions(+), 6 deletions(-)

diff --git a/src/call-forwarding.c b/src/call-forwarding.c
index 73ce433..42d681e 100644
--- a/src/call-forwarding.c
+++ b/src/call-forwarding.c
@@ -504,6 +504,7 @@ static DBusMessage *cf_get_properties_reply(DBusMessage *msg,
 	DBusMessageIter dict;
 	int i;
 	dbus_bool_t status;
+	GSList *hidden = NULL;
 
 	reply = dbus_message_new_method_return(msg);
 	if (reply == NULL)
@@ -515,17 +516,31 @@ 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],
-						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, NULL);
 	else
 		status = FALSE;
 
+	// If unconditional call-forwarding is enabled,
+	// hide conditionnal status
+	if (status == TRUE) {
+		struct ofono_call_forwarding_condition cd = {0, 0, {"", 0}, 0};
+
+		for (i = 0; i < 4; i++)
+			hidden = g_slist_prepend(hidden, &cd);
+	}
+
+	for (i = 0; i < 4; i++)
+		property_append_cf_conditions(&dict, (status &&
+				i != CALL_FORWARDING_TYPE_UNCONDITIONAL) ?
+				hidden : cf->cf_conditions[i],
+				BEARER_CLASS_VOICE,
+				cf_type_lut[i]);
+
+	if (status == TRUE)
+		g_slist_free(hidden);
+
 	ofono_dbus_dict_append(&dict, "ForwardingFlagOnSim", DBUS_TYPE_BOOLEAN,
 					&status);
 
@@ -552,6 +567,13 @@ static void get_query_cf_callback(const struct ofono_error *error, int total,
 			cf->flags |= CALL_FORWARDING_FLAG_CACHED;
 	}
 
+	if (cf->query_next == CALL_FORWARDING_TYPE_UNCONDITIONAL &&
+			is_cfu_enabled(cf, NULL) == TRUE) {
+		DBusMessage *reply = cf_get_properties_reply(cf->pending, cf);
+		__ofono_dbus_pending_reply(&cf->pending, reply);
+		return;
+	}
+
 	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);
@@ -575,7 +597,8 @@ static DBusMessage *cf_get_properties(DBusConnection *conn, DBusMessage *msg,
 	struct ofono_modem *modem = __ofono_atom_get_modem(cf->atom);
 
 	if ((cf->flags & CALL_FORWARDING_FLAG_CACHED) ||
-			ofono_modem_get_online(modem) == FALSE)
+			ofono_modem_get_online(modem) == FALSE ||
+			is_cfu_enabled(cf, NULL) == TRUE)
 		return cf_get_properties_reply(msg, cf);
 
 	if (cf->driver->query == NULL)
@@ -698,6 +721,15 @@ static void set_property_callback(const struct ofono_error *error, void *data)
 		return;
 	}
 
+	if (cf->query_next != CALL_FORWARDING_TYPE_UNCONDITIONAL &&
+			is_cfu_enabled(cf, NULL) == TRUE) {
+		DBusMessage *reply;
+		cf->flags &= ~CALL_FORWARDING_FLAG_CACHED;
+		reply = dbus_message_new_method_return(cf->pending);
+		__ofono_dbus_pending_reply(&cf->pending, reply);
+		return;
+	}
+
 	/* Successfully set, query the entire set just in case */
 	set_query_next_cf_cond(cf);
 }
-- 
1.7.1

[PATCH] call-forward: Call forwarding state handling

[Nicolas Bertrand]

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

When CFU is active be cautious with conditional
call-forward activation/deactivation
---
 src/call-forwarding.c |   46 ++++++++++++++++++++++++++++++++++++++++------
 1 files changed, 40 insertions(+), 6 deletions(-)

diff --git a/src/call-forwarding.c b/src/call-forwarding.c
index 73ce433..eff5e9d 100644
--- a/src/call-forwarding.c
+++ b/src/call-forwarding.c
@@ -504,6 +504,7 @@ static DBusMessage *cf_get_properties_reply(DBusMessage *msg,
 	DBusMessageIter dict;
 	int i;
 	dbus_bool_t status;
+	GSList *hidden = NULL;
 
 	reply = dbus_message_new_method_return(msg);
 	if (reply == NULL)
@@ -515,17 +516,33 @@ 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],
-						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, NULL);
 	else
 		status = FALSE;
 
+	/*
+	 * If unconditional call-forwarding is enabled,
+	 * hide conditionnal status
+	 */
+	if (status == TRUE) {
+		struct ofono_call_forwarding_condition cd = {0, 0, {"", 0}, 0};
+
+		for (i = 0; i < 4; i++)
+			hidden = g_slist_prepend(hidden, &cd);
+	}
+
+	for (i = 0; i < 4; i++)
+		property_append_cf_conditions(&dict, (status &&
+				i != CALL_FORWARDING_TYPE_UNCONDITIONAL) ?
+				hidden : cf->cf_conditions[i],
+				BEARER_CLASS_VOICE,
+				cf_type_lut[i]);
+
+	if (status == TRUE)
+		g_slist_free(hidden);
+
 	ofono_dbus_dict_append(&dict, "ForwardingFlagOnSim", DBUS_TYPE_BOOLEAN,
 					&status);
 
@@ -552,6 +569,13 @@ static void get_query_cf_callback(const struct ofono_error *error, int total,
 			cf->flags |= CALL_FORWARDING_FLAG_CACHED;
 	}
 
+	if (cf->query_next == CALL_FORWARDING_TYPE_UNCONDITIONAL &&
+			is_cfu_enabled(cf, NULL) == TRUE) {
+		DBusMessage *reply = cf_get_properties_reply(cf->pending, cf);
+		__ofono_dbus_pending_reply(&cf->pending, reply);
+		return;
+	}
+
 	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);
@@ -575,7 +599,8 @@ static DBusMessage *cf_get_properties(DBusConnection *conn, DBusMessage *msg,
 	struct ofono_modem *modem = __ofono_atom_get_modem(cf->atom);
 
 	if ((cf->flags & CALL_FORWARDING_FLAG_CACHED) ||
-			ofono_modem_get_online(modem) == FALSE)
+			ofono_modem_get_online(modem) == FALSE ||
+			is_cfu_enabled(cf, NULL) == TRUE)
 		return cf_get_properties_reply(msg, cf);
 
 	if (cf->driver->query == NULL)
@@ -698,6 +723,15 @@ static void set_property_callback(const struct ofono_error *error, void *data)
 		return;
 	}
 
+	if (cf->query_next != CALL_FORWARDING_TYPE_UNCONDITIONAL &&
+			is_cfu_enabled(cf, NULL) == TRUE) {
+		DBusMessage *reply;
+		cf->flags &= ~CALL_FORWARDING_FLAG_CACHED;
+		reply = dbus_message_new_method_return(cf->pending);
+		__ofono_dbus_pending_reply(&cf->pending, reply);
+		return;
+	}
+
 	/* Successfully set, query the entire set just in case */
 	set_query_next_cf_cond(cf);
 }
-- 
1.7.1

Re: [PATCH] call-forward: Call forwarding state handling

[Denis Kenzior]

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

Hi Nicolas,

On 06/16/2011 09:31 AM, Nicolas Bertrand wrote:
> When CFU is active be cautious with conditional
> call-forward activation/deactivation
> ---
>  src/call-forwarding.c |   46 ++++++++++++++++++++++++++++++++++++++++------
>  1 files changed, 40 insertions(+), 6 deletions(-)
> 

So I had to go back and re-read the thread that culminated in the need
for these changes.  I include it here for reference:
http://lists.ofono.org/pipermail/ofono/2011-January/007234.html

In particular see my previous 'todo' thoughts:
http://lists.ofono.org/pipermail/ofono/2011-January/007721.html

> diff --git a/src/call-forwarding.c b/src/call-forwarding.c
> index 73ce433..eff5e9d 100644
> --- a/src/call-forwarding.c
> +++ b/src/call-forwarding.c
> @@ -504,6 +504,7 @@ static DBusMessage *cf_get_properties_reply(DBusMessage *msg,
>  	DBusMessageIter dict;
>  	int i;
>  	dbus_bool_t status;
> +	GSList *hidden = NULL;
>  
>  	reply = dbus_message_new_method_return(msg);
>  	if (reply == NULL)
> @@ -515,17 +516,33 @@ 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],
> -						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, NULL);
>  	else
>  		status = FALSE;
>  
> +	/*
> +	 * If unconditional call-forwarding is enabled,
> +	 * hide conditionnal status
> +	 */
> +	if (status == TRUE) {
> +		struct ofono_call_forwarding_condition cd = {0, 0, {"", 0}, 0};
> +
> +		for (i = 0; i < 4; i++)
> +			hidden = g_slist_prepend(hidden, &cd);
> +	}

Lets not rely on status here, there can be situations where the EFcfis
and EFcff are not present on the SIM, or more likely the device does not
support reading of elementary EFs (e.g. N900).  So relying on
is_cfu_enabled is better here.

> +
> +	for (i = 0; i < 4; i++)
> +		property_append_cf_conditions(&dict, (status &&
> +				i != CALL_FORWARDING_TYPE_UNCONDITIONAL) ?
> +				hidden : cf->cf_conditions[i],
> +				BEARER_CLASS_VOICE,
> +				cf_type_lut[i]);
> +
> +	if (status == TRUE)
> +		g_slist_free(hidden);
> +

This is the right idea, but the code could be a bit more readable,
albeit longer.  Can you do something like:

for () {
	GSList *l;

	if (cfu_enabled && i != CALL_FORWARDING_TYPE_UNCONDITIONAL)
		l = hidden;
	else
		l = cf->cf_conditions[i];

	property_append_cf_conditions(&dict, i, l,
					BEARER_CLASS_VOICE,
					cf_type_lut[i]);
}

>  	ofono_dbus_dict_append(&dict, "ForwardingFlagOnSim", DBUS_TYPE_BOOLEAN,
>  					&status);
>  
> @@ -552,6 +569,13 @@ static void get_query_cf_callback(const struct ofono_error *error, int total,
>  			cf->flags |= CALL_FORWARDING_FLAG_CACHED;
>  	}
>  
> +	if (cf->query_next == CALL_FORWARDING_TYPE_UNCONDITIONAL &&
> +			is_cfu_enabled(cf, NULL) == TRUE) {
> +		DBusMessage *reply = cf_get_properties_reply(cf->pending, cf);
> +		__ofono_dbus_pending_reply(&cf->pending, reply);
> +		return;
> +	}
> +

This looks fine to me.

However, there's another path that needs to be taken care of.  Namely
that we set the cond lists queried via supplementary service operations
in ss_set_query_cf_callback.  You probably need to add extra logic there
to make sure that we don't overwrite / signal settings incorrectly.

In particular things can get funny if one queries conditional / all
settings when cfu is active.  Do note that query all/query conditional
MMI is not valid, but oFono implements it anyway.  If it makes it
easier, feel free to disable these.

>  	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);
> @@ -575,7 +599,8 @@ static DBusMessage *cf_get_properties(DBusConnection *conn, DBusMessage *msg,
>  	struct ofono_modem *modem = __ofono_atom_get_modem(cf->atom);
>  
>  	if ((cf->flags & CALL_FORWARDING_FLAG_CACHED) ||
> -			ofono_modem_get_online(modem) == FALSE)
> +			ofono_modem_get_online(modem) == FALSE ||
> +			is_cfu_enabled(cf, NULL) == TRUE)

I'm not sure about this change, I understand why you're doing it this
way.  There are several places we reset the cached flag due to error
conditions.  So to be on the safe side, perhaps we need to use two flags
for this?  E.g. something telling us that we need to re-query
conditionals once cfu is disabled.

>  		return cf_get_properties_reply(msg, cf);
>  
>  	if (cf->driver->query == NULL)
> @@ -698,6 +723,15 @@ static void set_property_callback(const struct ofono_error *error, void *data)
>  		return;
>  	}
>  
> +	if (cf->query_next != CALL_FORWARDING_TYPE_UNCONDITIONAL &&
> +			is_cfu_enabled(cf, NULL) == TRUE) {
> +		DBusMessage *reply;
> +		cf->flags &= ~CALL_FORWARDING_FLAG_CACHED;
> +		reply = dbus_message_new_method_return(cf->pending);
> +		__ofono_dbus_pending_reply(&cf->pending, reply);
> +		return;
> +	}
> +

This looks fine to me (but see my comment up above).  However, you must
also ensure that PropertyChanged signals are sent accordingly since you
have chosen to make conditional settings empty and vice-versa.

>  	/* Successfully set, query the entire set just in case */
>  	set_query_next_cf_cond(cf);
>  }

One more request, can you implement this 'quiescent' behavior inside
phonesim?  It would make it easier for us to stress-test these changes.

Thanks,
-Denis