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

[Denis Kenzior <denkenz@gmail.com>]

[-- 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