[PATCH] call-forwarding: fix for showing call forwarding states

[PATCH] call-forwarding: fix for showing call forwarding states

[Jussi Kangas]

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

---
Hi,

Here is a fix for call forwarding issue Jarko found a while back. ( TODO
issue "Call forwarding state handling change" and mailing list
conversation "Ofono CF states not always correct" ).

Br,
Jussi

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

diff --git a/src/call-forwarding.c b/src/call-forwarding.c
index d13f990..6f81296 100644
--- a/src/call-forwarding.c
+++ b/src/call-forwarding.c
@@ -514,10 +514,24 @@ 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],
+	if (cf->cf_conditions[CALL_FORWARDING_TYPE_UNCONDITIONAL] == 0) {
+		DBG("Append all");
+		for (i = 0; i < 4; i++)
+			property_append_cf_conditions(&dict,
+				cf->cf_conditions[i],
+					BEARER_CLASS_VOICE,
+						cf_type_lut[i]);
+	} else {
+		DBG("append only unconditional");
+		property_append_cf_conditions(&dict,
+			cf->cf_conditions[CALL_FORWARDING_TYPE_UNCONDITIONAL],
+				BEARER_CLASS_VOICE,
+					cf_type_lut[0]);
+		for (i = 1; i < 4; i++)
+			property_append_cf_conditions(&dict, NULL,
 						BEARER_CLASS_VOICE,
 						cf_type_lut[i]);
+	}
 
 	if ((cf->flags & CALL_FORWARDING_FLAG_CPHS_CFF) ||
 			cf->cfis_record_id > 0)
@@ -682,8 +696,23 @@ static void set_query_cf_callback(const struct ofono_error *error, int total,
 
 static void set_query_next_cf_cond(struct ofono_call_forwarding *cf)
 {
-	cf->driver->query(cf, cf->query_next, BEARER_CLASS_DEFAULT,
-			set_query_cf_callback, cf);
+	DBusMessage *reply;
+
+	while (cf->query_next != CALL_FORWARDING_TYPE_NOT_REACHABLE) {
+		if (!cf->cf_conditions[cf->query_next]) {
+			cf->driver->query(cf, cf->query_next,
+				BEARER_CLASS_DEFAULT,
+				set_query_cf_callback, cf);
+				return;
+		} else {
+			cf->query_next++;
+			if (cf->query_next == cf->query_end)
+				break;
+		}
+	}
+
+	reply = dbus_message_new_method_return(cf->pending);
+	__ofono_dbus_pending_reply(&cf->pending, reply);
 }
 
 static void set_property_callback(const struct ofono_error *error, void *data)
@@ -836,6 +865,7 @@ static void disable_conditional_callback(const struct ofono_error *error,
 static void disable_all_callback(const struct ofono_error *error, void *data)
 {
 	struct ofono_call_forwarding *cf = data;
+	int i;
 
 	if (error->type != OFONO_ERROR_TYPE_NO_ERROR) {
 		DBG("Error occurred during erasure of all");
@@ -848,6 +878,10 @@ static void disable_all_callback(const struct ofono_error *error, void *data)
 	/* Query all cf types */
 	cf->query_next = CALL_FORWARDING_TYPE_UNCONDITIONAL;
 	cf->query_end = CALL_FORWARDING_TYPE_NOT_REACHABLE;
+
+	for (i = 0; i < CALL_FORWARDING_TYPE_ALL; i++)
+		cf->cf_conditions[i] = 0;
+
 	set_query_next_cf_cond(cf);
 }
 
@@ -1014,8 +1048,25 @@ static void ss_set_query_cf_callback(const struct ofono_error *error, int total,
 
 static void ss_set_query_next_cf_cond(struct ofono_call_forwarding *cf)
 {
-	cf->driver->query(cf, cf->query_next, BEARER_CLASS_DEFAULT,
-			ss_set_query_cf_callback, cf);
+	DBusMessage *reply;
+
+	while (cf->query_next != CALL_FORWARDING_TYPE_NOT_REACHABLE) {
+		if (!cf->cf_conditions[cf->query_next]) {
+			cf->driver->query(cf, cf->query_next,
+				BEARER_CLASS_DEFAULT,
+				ss_set_query_cf_callback, cf);
+				return;
+		} else {
+			cf->query_next++;
+			if (cf->query_next == cf->query_end)
+				break;
+		}
+	}
+
+	reply = cf_ss_control_reply(cf, cf->ss_req);
+	__ofono_dbus_pending_reply(&cf->pending, reply);
+	g_free(cf->ss_req);
+	cf->ss_req = NULL;
 }
 
 static void cf_ss_control_callback(const struct ofono_error *error, void *data)
@@ -1032,6 +1083,11 @@ static void cf_ss_control_callback(const struct ofono_error *error, void *data)
 		return;
 	}
 
+	if (cf->ss_req) {
+		if (cf->ss_req->ss_type == SS_CONTROL_TYPE_ERASURE)
+			cf->cf_conditions[cf->ss_req->cf_type] = 0;
+	}
+
 	ss_set_query_next_cf_cond(cf);
 }
 
@@ -1180,8 +1236,14 @@ static gboolean cf_ss_control(int type, const char *sc,
 		cf->query_end = CALL_FORWARDING_TYPE_NOT_REACHABLE;
 		break;
 	case CALL_FORWARDING_TYPE_ALL_CONDITIONAL:
-		cf->query_next = CALL_FORWARDING_TYPE_BUSY;
-		cf->query_end = CALL_FORWARDING_TYPE_NOT_REACHABLE;
+	case CALL_FORWARDING_TYPE_UNCONDITIONAL:
+		if (type == SS_CONTROL_TYPE_REGISTRATION) {
+			cf->query_next = cf->ss_req->cf_type;
+			cf->query_end = cf->ss_req->cf_type;
+		} else {
+			cf->query_next = cf->ss_req->cf_type;
+			cf->query_end = CALL_FORWARDING_TYPE_NOT_REACHABLE;
+		}
 		break;
 	default:
 		cf->query_next = cf->ss_req->cf_type;
-- 
1.7.1

Re: [PATCH] call-forwarding: fix for showing call forwarding states

[Denis Kenzior]

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

Hi Jussi,

On 03/09/2011 04:27 AM, Jussi Kangas wrote:
> ---
> Hi,
> 
> Here is a fix for call forwarding issue Jarko found a while back. ( TODO
> issue "Call forwarding state handling change" and mailing list
> conversation "Ofono CF states not always correct" ).
> 
> Br,
> Jussi
> 
>  src/call-forwarding.c |   78 ++++++++++++++++++++++++++++++++++++++++++++-----
>  1 files changed, 70 insertions(+), 8 deletions(-)
> 
> diff --git a/src/call-forwarding.c b/src/call-forwarding.c
> index d13f990..6f81296 100644
> --- a/src/call-forwarding.c
> +++ b/src/call-forwarding.c
> @@ -514,10 +514,24 @@ 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],
> +	if (cf->cf_conditions[CALL_FORWARDING_TYPE_UNCONDITIONAL] == 0) {

Please use a comparison to NULL here instead of 0

> +		DBG("Append all");
> +		for (i = 0; i < 4; i++)
> +			property_append_cf_conditions(&dict,
> +				cf->cf_conditions[i],
> +					BEARER_CLASS_VOICE,
> +						cf_type_lut[i]);

Please fix the indentation levels here, the lines following
property_append... should all be the same indentation level.

Also, we might want to skip the unconditional call forwarding settings here.

> +	} else {
> +		DBG("append only unconditional");
> +		property_append_cf_conditions(&dict,
> +			cf->cf_conditions[CALL_FORWARDING_TYPE_UNCONDITIONAL],
> +				BEARER_CLASS_VOICE,
> +					cf_type_lut[0]);
> +		for (i = 1; i < 4; i++)
> +			property_append_cf_conditions(&dict, NULL,
>  						BEARER_CLASS_VOICE,
>  						cf_type_lut[i]);

So I'm confused, the TODO entry says we should not return conditional
entries if the unconditional call forwarding is set as they are now
quiescent.

Are you proposing we handle this a bit differently?

> +	}
>  
>  	if ((cf->flags & CALL_FORWARDING_FLAG_CPHS_CFF) ||
>  			cf->cfis_record_id > 0)
> @@ -682,8 +696,23 @@ static void set_query_cf_callback(const struct ofono_error *error, int total,
>  
>  static void set_query_next_cf_cond(struct ofono_call_forwarding *cf)
>  {
> -	cf->driver->query(cf, cf->query_next, BEARER_CLASS_DEFAULT,
> -			set_query_cf_callback, cf);
> +	DBusMessage *reply;
> +
> +	while (cf->query_next != CALL_FORWARDING_TYPE_NOT_REACHABLE) {
> +		if (!cf->cf_conditions[cf->query_next]) {
> +			cf->driver->query(cf, cf->query_next,
> +				BEARER_CLASS_DEFAULT,
> +				set_query_cf_callback, cf);
> +				return;
> +		} else {
> +			cf->query_next++;
> +			if (cf->query_next == cf->query_end)
> +				break;
> +		}
> +	}
> +
> +	reply = dbus_message_new_method_return(cf->pending);
> +	__ofono_dbus_pending_reply(&cf->pending, reply);

Really lost here,

>  }
>  
>  static void set_property_callback(const struct ofono_error *error, void *data)
> @@ -836,6 +865,7 @@ static void disable_conditional_callback(const struct ofono_error *error,
>  static void disable_all_callback(const struct ofono_error *error, void *data)
>  {
>  	struct ofono_call_forwarding *cf = data;
> +	int i;
>  
>  	if (error->type != OFONO_ERROR_TYPE_NO_ERROR) {
>  		DBG("Error occurred during erasure of all");
> @@ -848,6 +878,10 @@ static void disable_all_callback(const struct ofono_error *error, void *data)
>  	/* Query all cf types */
>  	cf->query_next = CALL_FORWARDING_TYPE_UNCONDITIONAL;
>  	cf->query_end = CALL_FORWARDING_TYPE_NOT_REACHABLE;
> +
> +	for (i = 0; i < CALL_FORWARDING_TYPE_ALL; i++)
> +		cf->cf_conditions[i] = 0;
> +

For pointers please use NULL instead of 0.

Why are we setting them to NULL here in the first place? And shouldn't
you free the data?

>  	set_query_next_cf_cond(cf);
>  }
>  
> @@ -1014,8 +1048,25 @@ static void ss_set_query_cf_callback(const struct ofono_error *error, int total,
>  
>  static void ss_set_query_next_cf_cond(struct ofono_call_forwarding *cf)
>  {
> -	cf->driver->query(cf, cf->query_next, BEARER_CLASS_DEFAULT,
> -			ss_set_query_cf_callback, cf);
> +	DBusMessage *reply;
> +
> +	while (cf->query_next != CALL_FORWARDING_TYPE_NOT_REACHABLE) {
> +		if (!cf->cf_conditions[cf->query_next]) {
> +			cf->driver->query(cf, cf->query_next,
> +				BEARER_CLASS_DEFAULT,
> +				ss_set_query_cf_callback, cf);
> +				return;
> +		} else {
> +			cf->query_next++;
> +			if (cf->query_next == cf->query_end)
> +				break;
> +		}
> +	}
> +
> +	reply = cf_ss_control_reply(cf, cf->ss_req);
> +	__ofono_dbus_pending_reply(&cf->pending, reply);
> +	g_free(cf->ss_req);
> +	cf->ss_req = NULL;

Again, very lost

>  }
>  
>  static void cf_ss_control_callback(const struct ofono_error *error, void *data)
> @@ -1032,6 +1083,11 @@ static void cf_ss_control_callback(const struct ofono_error *error, void *data)
>  		return;
>  	}
>  
> +	if (cf->ss_req) {
> +		if (cf->ss_req->ss_type == SS_CONTROL_TYPE_ERASURE)
> +			cf->cf_conditions[cf->ss_req->cf_type] = 0;
> +	}
> +
>  	ss_set_query_next_cf_cond(cf);
>  }
>  
> @@ -1180,8 +1236,14 @@ static gboolean cf_ss_control(int type, const char *sc,
>  		cf->query_end = CALL_FORWARDING_TYPE_NOT_REACHABLE;
>  		break;
>  	case CALL_FORWARDING_TYPE_ALL_CONDITIONAL:
> -		cf->query_next = CALL_FORWARDING_TYPE_BUSY;
> -		cf->query_end = CALL_FORWARDING_TYPE_NOT_REACHABLE;

I don't see how handling both these cases with the same code can work...

> +	case CALL_FORWARDING_TYPE_UNCONDITIONAL:
> +		if (type == SS_CONTROL_TYPE_REGISTRATION) {
> +			cf->query_next = cf->ss_req->cf_type;
> +			cf->query_end = cf->ss_req->cf_type;
> +		} else {
> +			cf->query_next = cf->ss_req->cf_type;
> +			cf->query_end = CALL_FORWARDING_TYPE_NOT_REACHABLE;
> +		}

I'm a bit lost here, can you explain some more what you're trying to
accomplish?

>  		break;
>  	default:
>  		cf->query_next = cf->ss_req->cf_type;

Regards,
-Denis

Re: [PATCH] call-forwarding: fix for showing call forwarding states

[Jussi Kangas]

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


Hi Denis,

On Sat, 2011-03-12 at 00:48 +0200, Denis Kenzior wrote: 
> > +	} else {
> > +		DBG("append only unconditional");
> > +		property_append_cf_conditions(&dict,
> > +			cf->cf_conditions[CALL_FORWARDING_TYPE_UNCONDITIONAL],
> > +				BEARER_CLASS_VOICE,
> > +					cf_type_lut[0]);
> > +		for (i = 1; i < 4; i++)
> > +			property_append_cf_conditions(&dict, NULL,
> >  						BEARER_CLASS_VOICE,
> >  						cf_type_lut[i]);
> 
> So I'm confused, the TODO entry says we should not return conditional
> entries if the unconditional call forwarding is set as they are now
> quiescent.
> 
> Are you proposing we handle this a bit differently?

No. I'm setting the unconditional values to NULL in order to clean them
from API. If only unconditional is updated when there has been
unconditional conditions active before, list-modems would show them
still. After this change information about unconditional conditions
remains in oFono, but they are not shown in the API.

> 
> > +	}
> >  
> >  	if ((cf->flags & CALL_FORWARDING_FLAG_CPHS_CFF) ||
> >  			cf->cfis_record_id > 0)
> > @@ -682,8 +696,23 @@ static void set_query_cf_callback(const struct ofono_error *error, int total,
> >  
> >  static void set_query_next_cf_cond(struct ofono_call_forwarding *cf)
> >  {
> > -	cf->driver->query(cf, cf->query_next, BEARER_CLASS_DEFAULT,
> > -			set_query_cf_callback, cf);
> > +	DBusMessage *reply;
> > +
> > +	while (cf->query_next != CALL_FORWARDING_TYPE_NOT_REACHABLE) {
> > +		if (!cf->cf_conditions[cf->query_next]) {
> > +			cf->driver->query(cf, cf->query_next,
> > +				BEARER_CLASS_DEFAULT,
> > +				set_query_cf_callback, cf);
> > +				return;
> > +		} else {
> > +			cf->query_next++;
> > +			if (cf->query_next == cf->query_end)
> > +				break;
> > +		}
> > +	}
> > +
> > +	reply = dbus_message_new_method_return(cf->pending);
> > +	__ofono_dbus_pending_reply(&cf->pending, reply);
> 
> Really lost here,

Query_next and query_end tell only what to ask from query_next to
query_end. This causes pointless query if there is a value in the middle
which is already known. What this implementation does it prevents the
query if the condition is already known. 
  
> >  static void set_property_callback(const struct ofono_error *error, void *data)
> > @@ -836,6 +865,7 @@ static void disable_conditional_callback(const struct ofono_error *error,
> >  static void disable_all_callback(const struct ofono_error *error, void *data)
> >  {
> >  	struct ofono_call_forwarding *cf = data;
> > +	int i;
> >  
> >  	if (error->type != OFONO_ERROR_TYPE_NO_ERROR) {
> >  		DBG("Error occurred during erasure of all");
> > @@ -848,6 +878,10 @@ static void disable_all_callback(const struct ofono_error *error, void *data)
> >  	/* Query all cf types */
> >  	cf->query_next = CALL_FORWARDING_TYPE_UNCONDITIONAL;
> >  	cf->query_end = CALL_FORWARDING_TYPE_NOT_REACHABLE;
> > +
> > +	for (i = 0; i < CALL_FORWARDING_TYPE_ALL; i++)
> > +		cf->cf_conditions[i] = 0;
> > +
> 
> For pointers please use NULL instead of 0.
> 
> Why are we setting them to NULL here in the first place? And shouldn't
> you free the data?

This is the other half of the implementation that prevents the querying
already known conditions. What I'm doing here is that I'm using the
cf_conditions as a flag to tell what is known. And since this is the
response for clearing all conditions, we don't know anything and
everything has to be queried in order to see if the request was
succesful or not. What comes to freeing the data I think you are right.
I think totally new parameter inside cf would be a better solution. That
would be less heavy. I'll check that out. 

> > @@ -1180,8 +1236,14 @@ static gboolean cf_ss_control(int type, const char *sc,
> >  		cf->query_end = CALL_FORWARDING_TYPE_NOT_REACHABLE;
> >  		break;
> >  	case CALL_FORWARDING_TYPE_ALL_CONDITIONAL:
> > -		cf->query_next = CALL_FORWARDING_TYPE_BUSY;
> > -		cf->query_end = CALL_FORWARDING_TYPE_NOT_REACHABLE;
> 
> I don't see how handling both these cases with the same code can work...

Well at least in my testings if there is break and separate query_next
and query_end for CALL_FORWARDING_TYPE_ALL_CONDITIONAL erasing the
condition by using ss API does not remove conditions from API. 

> > +	case CALL_FORWARDING_TYPE_UNCONDITIONAL:
> > +		if (type == SS_CONTROL_TYPE_REGISTRATION) {
> > +			cf->query_next = cf->ss_req->cf_type;
> > +			cf->query_end = cf->ss_req->cf_type;
> > +		} else {
> > +			cf->query_next = cf->ss_req->cf_type;
> > +			cf->query_end = CALL_FORWARDING_TYPE_NOT_REACHABLE;
> > +		}
> 
> I'm a bit lost here, can you explain some more what you're trying to
> accomplish?

I was looking for this: If the user sets call forwarding unconditional
or all conditional on, there is no reason to ask anything else since
implementation in cf_get_properties_reply prevents the showing. If user
clears the call forwarding unconditional all have to be asked. 

Br, 
-Jussi

Re: [PATCH] call-forwarding: fix for showing call forwarding states

[Denis Kenzior]

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

Hi Jussi,

On 03/14/2011 08:47 AM, Jussi Kangas wrote:
> 
> Hi Denis,
> 
> On Sat, 2011-03-12 at 00:48 +0200, Denis Kenzior wrote: 
>>> +	} else {
>>> +		DBG("append only unconditional");
>>> +		property_append_cf_conditions(&dict,
>>> +			cf->cf_conditions[CALL_FORWARDING_TYPE_UNCONDITIONAL],
>>> +				BEARER_CLASS_VOICE,
>>> +					cf_type_lut[0]);
>>> +		for (i = 1; i < 4; i++)
>>> +			property_append_cf_conditions(&dict, NULL,
>>>  						BEARER_CLASS_VOICE,
>>>  						cf_type_lut[i]);
>>
>> So I'm confused, the TODO entry says we should not return conditional
>> entries if the unconditional call forwarding is set as they are now
>> quiescent.
>>
>> Are you proposing we handle this a bit differently?
> 
> No. I'm setting the unconditional values to NULL in order to clean them
> from API. If only unconditional is updated when there has been
> unconditional conditions active before, list-modems would show them
> still. After this change information about unconditional conditions
> remains in oFono, but they are not shown in the API.

Ok, I think I understand now.  You're checking whether any unconditional
entries exist.  If they do, you report them and assume all other entries
are quiescent.  Right?

So correct me if I'm wrong, but I don't think we can do that.  the
current cf_conditions implementation stores all conditions, which could
be unrelated to voice; and if my interpretation of 22.004 is correct you
can have something like this:

Activate CFB for all services to Number 1
Activate CFU for Data services to Number 2

Which results in cf_conditions[CALL_FORWARDING_TYPE_UNCONDITIONAL] not
being NULL and you reporting CFB for voice incorrectly.

You are probably safer using is_cfu_enabled() function.

> 
>>
>>> +	}
>>>  
>>>  	if ((cf->flags & CALL_FORWARDING_FLAG_CPHS_CFF) ||
>>>  			cf->cfis_record_id > 0)
>>> @@ -682,8 +696,23 @@ static void set_query_cf_callback(const struct ofono_error *error, int total,
>>>  
>>>  static void set_query_next_cf_cond(struct ofono_call_forwarding *cf)
>>>  {
>>> -	cf->driver->query(cf, cf->query_next, BEARER_CLASS_DEFAULT,
>>> -			set_query_cf_callback, cf);
>>> +	DBusMessage *reply;
>>> +
>>> +	while (cf->query_next != CALL_FORWARDING_TYPE_NOT_REACHABLE) {
>>> +		if (!cf->cf_conditions[cf->query_next]) {
>>> +			cf->driver->query(cf, cf->query_next,
>>> +				BEARER_CLASS_DEFAULT,
>>> +				set_query_cf_callback, cf);
>>> +				return;
>>> +		} else {
>>> +			cf->query_next++;
>>> +			if (cf->query_next == cf->query_end)
>>> +				break;
>>> +		}
>>> +	}
>>> +
>>> +	reply = dbus_message_new_method_return(cf->pending);
>>> +	__ofono_dbus_pending_reply(&cf->pending, reply);
>>
>> Really lost here,
> 
> Query_next and query_end tell only what to ask from query_next to
> query_end. This causes pointless query if there is a value in the middle
> which is already known. What this implementation does it prevents the
> query if the condition is already known. 
>   

So the reason it is this way is that if we're querying via SS we want to
force the query to proceed anyway.  Even if the value is already known.
 This is really only a problem for *#002# and *#004# or if the request
of all CF conditions failed as a result of get_properties.  In the
latter case we don't set the cached flag and re-get all the cfs again.

Please note that *#002# and *#004# are not valid MMI strings anyway
according to 22.004 ("Interrogation of groups of Supplementary Services
is not supported.") but I threw them in there since the logic was
already implemented.  If it makes things easier we can limit this
functionality.

Either way this chunk belongs in a separate patch.

>>>  static void set_property_callback(const struct ofono_error *error, void *data)
>>> @@ -836,6 +865,7 @@ static void disable_conditional_callback(const struct ofono_error *error,
>>>  static void disable_all_callback(const struct ofono_error *error, void *data)
>>>  {
>>>  	struct ofono_call_forwarding *cf = data;
>>> +	int i;
>>>  
>>>  	if (error->type != OFONO_ERROR_TYPE_NO_ERROR) {
>>>  		DBG("Error occurred during erasure of all");
>>> @@ -848,6 +878,10 @@ static void disable_all_callback(const struct ofono_error *error, void *data)
>>>  	/* Query all cf types */
>>>  	cf->query_next = CALL_FORWARDING_TYPE_UNCONDITIONAL;
>>>  	cf->query_end = CALL_FORWARDING_TYPE_NOT_REACHABLE;
>>> +
>>> +	for (i = 0; i < CALL_FORWARDING_TYPE_ALL; i++)
>>> +		cf->cf_conditions[i] = 0;
>>> +
>>
>> For pointers please use NULL instead of 0.
>>
>> Why are we setting them to NULL here in the first place? And shouldn't
>> you free the data?
> 
> This is the other half of the implementation that prevents the querying
> already known conditions. What I'm doing here is that I'm using the
> cf_conditions as a flag to tell what is known. And since this is the
> response for clearing all conditions, we don't know anything and
> everything has to be queried in order to see if the request was
> succesful or not. What comes to freeing the data I think you are right.
> I think totally new parameter inside cf would be a better solution. That
> would be less heavy. I'll check that out. 
> 

So see my comment above, this optimization might belong in a separate patch.

>>> @@ -1180,8 +1236,14 @@ static gboolean cf_ss_control(int type, const char *sc,
>>>  		cf->query_end = CALL_FORWARDING_TYPE_NOT_REACHABLE;
>>>  		break;
>>>  	case CALL_FORWARDING_TYPE_ALL_CONDITIONAL:
>>> -		cf->query_next = CALL_FORWARDING_TYPE_BUSY;
>>> -		cf->query_end = CALL_FORWARDING_TYPE_NOT_REACHABLE;
>>
>> I don't see how handling both these cases with the same code can work...
> 
> Well at least in my testings if there is break and separate query_next
> and query_end for CALL_FORWARDING_TYPE_ALL_CONDITIONAL erasing the
> condition by using ss API does not remove conditions from API. 
> 

Sorry I'm still confused with this change.  Maybe a more detailed trace
would help?

>>> +	case CALL_FORWARDING_TYPE_UNCONDITIONAL:
>>> +		if (type == SS_CONTROL_TYPE_REGISTRATION) {
>>> +			cf->query_next = cf->ss_req->cf_type;
>>> +			cf->query_end = cf->ss_req->cf_type;
>>> +		} else {
>>> +			cf->query_next = cf->ss_req->cf_type;
>>> +			cf->query_end = CALL_FORWARDING_TYPE_NOT_REACHABLE;
>>> +		}
>>
>> I'm a bit lost here, can you explain some more what you're trying to
>> accomplish?
> 
> I was looking for this: If the user sets call forwarding unconditional
> or all conditional on, there is no reason to ask anything else since
> implementation in cf_get_properties_reply prevents the showing. If user
> clears the call forwarding unconditional all have to be asked. 
> 

I see, so again I'm not sure this optimization is going to work.
Suppose I run an registration to CFall to Number 1 via USSD.  Something
like:

**002*+12345#

I do want all affected services to show up in my USSD.Initiate return
dictionary with the results that the network is giving us.  Not anything
that is cached by oFono.  Think of MMI codes as completely bypassing the
cache but oFono peeking at them to update its own properties.

It might be helpful to think of this as two caches.  The conditional
cache and the unconditional cache.  If the conditional values are
updated when CFU is on, then we should clear the conditional cache and
re-query them next time they are needed (e.g. when CFU is turned off)

Regards,
-Denis

Re: [PATCH] call-forwarding: fix for showing call forwarding states

[Jussi Kangas]

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

Hi,

On Thu, 2011-03-17 at 05:34 +0200, Denis Kenzior wrote:

> >> So I'm confused, the TODO entry says we should not return conditional
> >> entries if the unconditional call forwarding is set as they are now
> >> quiescent.
> >>
> >> Are you proposing we handle this a bit differently?
> > 
> > No. I'm setting the unconditional values to NULL in order to clean them
> > from API. If only unconditional is updated when there has been
> > unconditional conditions active before, list-modems would show them
> > still. After this change information about unconditional conditions
> > remains in oFono, but they are not shown in the API.
> 
> Ok, I think I understand now.  You're checking whether any unconditional
> entries exist.  If they do, you report them and assume all other entries
> are quiescent.  Right?

Right. And I only clean them from API. When unconditional is removed,
conditional states become visible without any query to network except
checking if the unconditional really disappear. 

> 
> So correct me if I'm wrong, but I don't think we can do that.  the
> current cf_conditions implementation stores all conditions, which could
> be unrelated to voice; and if my interpretation of 22.004 is correct you
> can have something like this:
> 
> Activate CFB for all services to Number 1
> Activate CFU for Data services to Number 2
> 
> Which results in cf_conditions[CALL_FORWARDING_TYPE_UNCONDITIONAL] not
> being NULL and you reporting CFB for voice incorrectly.
> 
> You are probably safer using is_cfu_enabled() function.

As far as I know scenario you describe cannot happen with oFono. There
is no data related conditions in the call forwarding API. Sure, you can
set data forwarding on using SS API, but I don't see how data forwarding
states could be shown in call forwarding API when all states in API
start with word "voice". Also according to comments in code fax and data
are not supported. 

> >>>  
> >>>  	if ((cf->flags & CALL_FORWARDING_FLAG_CPHS_CFF) ||
> >>>  			cf->cfis_record_id > 0)
> >>> @@ -682,8 +696,23 @@ static void set_query_cf_callback(const struct ofono_error *error, int total,
> >>>  
> >>>  static void set_query_next_cf_cond(struct ofono_call_forwarding *cf)
> >>>  {
> >>> -	cf->driver->query(cf, cf->query_next, BEARER_CLASS_DEFAULT,
> >>> -			set_query_cf_callback, cf);
> >>> +	DBusMessage *reply;
> >>> +
> >>> +	while (cf->query_next != CALL_FORWARDING_TYPE_NOT_REACHABLE) {
> >>> +		if (!cf->cf_conditions[cf->query_next]) {
> >>> +			cf->driver->query(cf, cf->query_next,
> >>> +				BEARER_CLASS_DEFAULT,
> >>> +				set_query_cf_callback, cf);
> >>> +				return;
> >>> +		} else {
> >>> +			cf->query_next++;
> >>> +			if (cf->query_next == cf->query_end)
> >>> +				break;
> >>> +		}
> >>> +	}
> >>> +
> >>> +	reply = dbus_message_new_method_return(cf->pending);
> >>> +	__ofono_dbus_pending_reply(&cf->pending, reply);
> >>
> >> Really lost here,
> > 
> > Query_next and query_end tell only what to ask from query_next to
> > query_end. This causes pointless query if there is a value in the middle
> > which is already known. What this implementation does it prevents the
> > query if the condition is already known. 
> >   
> 
> So the reason it is this way is that if we're querying via SS we want to
> force the query to proceed anyway.  Even if the value is already known.
>  This is really only a problem for *#002# and *#004# or if the request
> of all CF conditions failed as a result of get_properties.  In the
> latter case we don't set the cached flag and re-get all the cfs again.

Change is not done for reasons you think here. Fix above is part of
change made mainly for following error situation: 

oFono has conditional and unconditional conditions on. It is rebooted.
Network reports only unconditional status. User removes unconditional
status using SS API. API shows no active call forwardings.  

In order to fix this is I changed the cf_ss_control to ask everything if
unconditional is erased. This causes unnecessary queries if the oFono is
not rebooted after setting the unconditional on and unconditional
conditions exists. This is is prevented with change above. 

> Please note that *#002# and *#004# are not valid MMI strings anyway
> according to 22.004 ("Interrogation of groups of Supplementary Services
> is not supported.") but I threw them in there since the logic was
> already implemented.  If it makes things easier we can limit this
> functionality.
> 
> Either way this chunk belongs in a separate patch.

Well, my opinion is that it's part of the same states showing correctly
change for the reasons explained above, but sure I can separate them. 

> >>> @@ -1180,8 +1236,14 @@ static gboolean cf_ss_control(int type, const char *sc,
> >>>  		cf->query_end = CALL_FORWARDING_TYPE_NOT_REACHABLE;
> >>>  		break;
> >>>  	case CALL_FORWARDING_TYPE_ALL_CONDITIONAL:
> >>> -		cf->query_next = CALL_FORWARDING_TYPE_BUSY;
> >>> -		cf->query_end = CALL_FORWARDING_TYPE_NOT_REACHABLE;
> >>
> >> I don't see how handling both these cases with the same code can work...
> > 
> > Well at least in my testings if there is break and separate query_next
> > and query_end for CALL_FORWARDING_TYPE_ALL_CONDITIONAL erasing the
> > condition by using ss API does not remove conditions from API. 
> > 
> 
> Sorry I'm still confused with this change.  Maybe a more detailed trace
> would help?

I rechecked this and sent the new version to community in Tuesday. I
found out that although in API level it seemed to work nicely it
actually worked so that first query failed and after that oFono asked
all conditions from network when list-modems were run. New version does
not do that anymore. I brought back the separate query_next and
query_end there for ALL_CONDITIONAL. 

> >>> +	case CALL_FORWARDING_TYPE_UNCONDITIONAL:
> >>> +		if (type == SS_CONTROL_TYPE_REGISTRATION) {
> >>> +			cf->query_next = cf->ss_req->cf_type;
> >>> +			cf->query_end = cf->ss_req->cf_type;
> >>> +		} else {
> >>> +			cf->query_next = cf->ss_req->cf_type;
> >>> +			cf->query_end = CALL_FORWARDING_TYPE_NOT_REACHABLE;
> >>> +		}
> >>
> >> I'm a bit lost here, can you explain some more what you're trying to
> >> accomplish?
> > 
> > I was looking for this: If the user sets call forwarding unconditional
> > or all conditional on, there is no reason to ask anything else since
> > implementation in cf_get_properties_reply prevents the showing. If user
> > clears the call forwarding unconditional all have to be asked. 
> > 
> 
> I see, so again I'm not sure this optimization is going to work.
> Suppose I run an registration to CFall to Number 1 via USSD.  Something
> like:
> 
> **002*+12345#
> 
> I do want all affected services to show up in my USSD.Initiate return
> dictionary with the results that the network is giving us.  Not anything
> that is cached by oFono.  Think of MMI codes as completely bypassing the
> cache but oFono peeking at them to update its own properties.
> 
> It might be helpful to think of this as two caches.  The conditional
> cache and the unconditional cache.  If the conditional values are
> updated when CFU is on, then we should clear the conditional cache and
> re-query them next time they are needed (e.g. when CFU is turned off)

Hmm. I'm not sure if I follow. Current functionality works so that no
matter if the conditional conditions are visible or not they are queried
after update. Basically it works as before except states not shown
anymore if unconditional is on. What you describe is chance for further
optimization, not a bug or problem. 

Please check out the new version of the patch. Maybe that clarifies the
case little bit. 

Br,
-Jussi

Re: [PATCH] call-forwarding: fix for showing call forwarding states

[Denis Kenzior]

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

Hi Jussi,

>>
>> So correct me if I'm wrong, but I don't think we can do that.  the
>> current cf_conditions implementation stores all conditions, which could
>> be unrelated to voice; and if my interpretation of 22.004 is correct you
>> can have something like this:
>>
>> Activate CFB for all services to Number 1
>> Activate CFU for Data services to Number 2
>>
>> Which results in cf_conditions[CALL_FORWARDING_TYPE_UNCONDITIONAL] not
>> being NULL and you reporting CFB for voice incorrectly.
>>
>> You are probably safer using is_cfu_enabled() function.
> 
> As far as I know scenario you describe cannot happen with oFono. There
> is no data related conditions in the call forwarding API. Sure, you can
> set data forwarding on using SS API, but I don't see how data forwarding
> states could be shown in call forwarding API when all states in API
> start with word "voice". Also according to comments in code fax and data
> are not supported. 
>

It can.  oFono stores _all_ conditions in its lists.  However, it
filters them when reporting the conditions in get_properties or signals.
 This is why is_cfu_enabled function looks the way it does.  So a
scenario like this is fully possible.  Not to mention that the CFs are
queried for all services by default.

Regards,
-Denis

Re: [PATCH] call-forwarding: fix for showing call forwarding states

[Jussi Kangas]

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

Hi,

On Thu, 2011-03-17 at 17:00 +0200, Denis Kenzior wrote:
> Hi Jussi,
> 
> >>
> >> So correct me if I'm wrong, but I don't think we can do that.  the
> >> current cf_conditions implementation stores all conditions, which could
> >> be unrelated to voice; and if my interpretation of 22.004 is correct you
> >> can have something like this:
> >>
> >> Activate CFB for all services to Number 1
> >> Activate CFU for Data services to Number 2
> >>
> >> Which results in cf_conditions[CALL_FORWARDING_TYPE_UNCONDITIONAL] not
> >> being NULL and you reporting CFB for voice incorrectly.
> >>
> >> You are probably safer using is_cfu_enabled() function.
> > 
> > As far as I know scenario you describe cannot happen with oFono. There
> > is no data related conditions in the call forwarding API. Sure, you can
> > set data forwarding on using SS API, but I don't see how data forwarding
> > states could be shown in call forwarding API when all states in API
> > start with word "voice". Also according to comments in code fax and data
> > are not supported. 
> >
> 
> It can.  oFono stores _all_ conditions in its lists.  However, it
> filters them when reporting the conditions in get_properties or signals.
>  This is why is_cfu_enabled function looks the way it does.  So a
> scenario like this is fully possible.  Not to mention that the CFs are
> queried for all services by default.

All right. I have no access to network that would support data
forwarding or my modem does not support it so I cannot verify this in
practice. But I think problem can be avoided by changing the line 

if (cf->cf_conditions[CALL_FORWARDING_TYPE_UNCONDITIONAL] == NULL) {

to 

if (!is_cfu_enabled(cf, NULL)) {

Of course little bit optimization would be good to avoid second calling
of this method later in function. Moving the setting of status variable
above these and using it when checked if UNCONDITIONAL is on instead of
is_cfu_enabled seems to be working nicely.

Br,
-Jussi

Re: [PATCH] call-forwarding: fix for showing call forwarding states

[Denis Kenzior]

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

Hi Jussi,

On 03/18/2011 07:53 AM, Jussi Kangas wrote:
> Hi,
> 
> On Thu, 2011-03-17 at 17:00 +0200, Denis Kenzior wrote:
>> Hi Jussi,
>>
>>>>
>>>> So correct me if I'm wrong, but I don't think we can do that.  the
>>>> current cf_conditions implementation stores all conditions, which could
>>>> be unrelated to voice; and if my interpretation of 22.004 is correct you
>>>> can have something like this:
>>>>
>>>> Activate CFB for all services to Number 1
>>>> Activate CFU for Data services to Number 2
>>>>
>>>> Which results in cf_conditions[CALL_FORWARDING_TYPE_UNCONDITIONAL] not
>>>> being NULL and you reporting CFB for voice incorrectly.
>>>>
>>>> You are probably safer using is_cfu_enabled() function.
>>>
>>> As far as I know scenario you describe cannot happen with oFono. There
>>> is no data related conditions in the call forwarding API. Sure, you can
>>> set data forwarding on using SS API, but I don't see how data forwarding
>>> states could be shown in call forwarding API when all states in API
>>> start with word "voice". Also according to comments in code fax and data
>>> are not supported. 
>>>
>>
>> It can.  oFono stores _all_ conditions in its lists.  However, it
>> filters them when reporting the conditions in get_properties or signals.
>>  This is why is_cfu_enabled function looks the way it does.  So a
>> scenario like this is fully possible.  Not to mention that the CFs are
>> queried for all services by default.
> 
> All right. I have no access to network that would support data
> forwarding or my modem does not support it so I cannot verify this in
> practice. But I think problem can be avoided by changing the line 
> 
> if (cf->cf_conditions[CALL_FORWARDING_TYPE_UNCONDITIONAL] == NULL) {
> 
> to 
> 
> if (!is_cfu_enabled(cf, NULL)) {
> 
> Of course little bit optimization would be good to avoid second calling
> of this method later in function. Moving the setting of status variable
> above these and using it when checked if UNCONDITIONAL is on instead of
> is_cfu_enabled seems to be working nicely.

Yes, lets do the optimization since its quite easy.

Regards,
-Denis