[PATCH 1/8] ussd: Recover idle state in case of response sending failure

[PATCH 1/8] ussd: Recover idle state in case of response sending failure

[Philippe Nunes]

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

GCF test case 31.8.1.2.3 is rejecting the user response.
Any subsequent USSD notification are not handled because USSD is always in
state user-action.
---
 src/ussd.c |    8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/src/ussd.c b/src/ussd.c
index 74888b2..0ad4f61 100644
--- a/src/ussd.c
+++ b/src/ussd.c
@@ -414,6 +414,10 @@ void ofono_ussd_notify(struct ofono_ussd *ussd, int status, int dcs,
 		return;
 	}
 
+	if (status == OFONO_USSD_STATUS_TERMINATED &&
+			ussd->state == USSD_STATE_IDLE)
+		return;
+
 	if (status == OFONO_USSD_STATUS_NOT_SUPPORTED) {
 		ussd_change_state(ussd, USSD_STATE_IDLE);
 
@@ -595,9 +599,11 @@ static void ussd_response_callback(const struct ofono_error *error, void *data)
 	struct ofono_ussd *ussd = data;
 	DBusMessage *reply;
 
-	if (error->type != OFONO_ERROR_TYPE_NO_ERROR)
+	if (error->type != OFONO_ERROR_TYPE_NO_ERROR) {
 		DBG("ussd response failed with error: %s",
 				telephony_error_to_str(error));
+		ussd_change_state(ussd, USSD_STATE_IDLE);
+	}
 
 	if (error->type == OFONO_ERROR_TYPE_NO_ERROR) {
 		ussd_change_state(ussd, USSD_STATE_RESPONSE_SENT);
-- 
1.7.9.5

[PATCH 2/8] stkagent: '+' is considered as a digit

[Philippe Nunes]

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

The function 'valid_phone_number_format' does not comply with GCF test cases
expectation. For STK, the character '+' should be considered as a whole digit.
---
 src/stkagent.c |   26 ++++++++++++++++++++++++--
 1 file changed, 24 insertions(+), 2 deletions(-)

diff --git a/src/stkagent.c b/src/stkagent.c
index 7c3f697..392a2b2 100644
--- a/src/stkagent.c
+++ b/src/stkagent.c
@@ -69,6 +69,27 @@ struct stk_agent {
 #define TERMINATE_ERROR ERROR_PREFIX ".EndSession"
 #define BUSY_ERROR ERROR_PREFIX ".Busy"
 
+static gboolean check_digit(const char *digit)
+{
+	int len = strlen(digit);
+	int i;
+
+	if (!len)
+		return FALSE;
+
+	for (i = 0; i < len; i++) {
+		if (digit[i] >= '0' && digit[i] <= '9')
+			continue;
+
+		if (digit[i] == '*' || digit[i] == '#' || digit[i] == '+')
+			continue;
+
+		return FALSE;
+	}
+
+	return TRUE;
+}
+
 static void stk_agent_send_noreply(struct stk_agent *agent, const char *method)
 {
 	DBusConnection *conn = ofono_dbus_get_connection();
@@ -541,7 +562,7 @@ static void get_digit_cb(DBusPendingCall *call, void *data)
 					DBUS_TYPE_STRING, &digit,
 					DBUS_TYPE_INVALID) == FALSE ||
 			strlen(digit) != 1 ||
-			!valid_phone_number_format(digit)) {
+			!check_digit(digit)) {
 		ofono_error("Can't parse the reply to GetDigit()");
 		remove_agent = TRUE;
 		goto error;
@@ -675,7 +696,8 @@ static void get_digits_cb(DBusPendingCall *call, void *data)
 
 	if (dbus_message_get_args(reply, NULL,
 					DBUS_TYPE_STRING, &string,
-					DBUS_TYPE_INVALID) == FALSE) {
+					DBUS_TYPE_INVALID) == FALSE ||
+					!check_digit(string)) {
 		ofono_error("Can't parse the reply to GetDigits()");
 		remove_agent = TRUE;
 		goto error;
-- 
1.7.9.5

[PATCH 3/8] call-forwarding: return specific errors for SS query

[Philippe Nunes]

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

Trace is modified as a CF query is not necessarily done after a CF setting
via SS.
---
 src/call-forwarding.c |    5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/src/call-forwarding.c b/src/call-forwarding.c
index 5acbd67..91e34c6 100644
--- a/src/call-forwarding.c
+++ b/src/call-forwarding.c
@@ -981,9 +981,10 @@ static void ss_set_query_cf_callback(const struct ofono_error *error, int total,
 	DBusMessage *reply;
 
 	if (error->type != OFONO_ERROR_TYPE_NO_ERROR) {
-		ofono_error("Setting succeeded, but query failed");
+		ofono_error("Query failed with error: %s",
+						telephony_error_to_str(error));
 		cf->flags &= ~CALL_FORWARDING_FLAG_CACHED;
-		reply = __ofono_error_failed(cf->pending);
+		reply = __ofono_error_from_error(error, cf->pending);
 		__ofono_dbus_pending_reply(&cf->pending, reply);
 		return;
 	}
-- 
1.7.9.5

[PATCH 4/8] call-forwarding: class applied is the class given by SS code

[Philippe Nunes]

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

GCF test cases 31.2.1.6.1/2 are asking to make a query according a specific
class.
The default class is applied when no class is specified in the SS code.
---
 src/call-forwarding.c |   22 +++++++++++-----------
 1 file changed, 11 insertions(+), 11 deletions(-)

diff --git a/src/call-forwarding.c b/src/call-forwarding.c
index 91e34c6..7531f07 100644
--- a/src/call-forwarding.c
+++ b/src/call-forwarding.c
@@ -1012,7 +1012,7 @@ 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,
+	cf->driver->query(cf, cf->query_next, cf->ss_req->cls,
 			ss_set_query_cf_callback, cf);
 }
 
@@ -1167,6 +1167,16 @@ static gboolean cf_ss_control(int type, const char *sc,
 		return TRUE;
 	}
 
+	/*
+	 * Some modems don't understand all classes very well, particularly
+	 * the older models.  So if the bearer class is the default, we
+	 * just use the more commonly understood value of 7 since BEARER_SMS
+	 * is not applicable to CallForwarding conditions according to 22.004
+	 * Annex A
+	 */
+	if (cls == BEARER_CLASS_SS_DEFAULT)
+		cls = BEARER_CLASS_DEFAULT;
+
 	cf->ss_req->ss_type = type;
 	cf->ss_req->cf_type = cf_type;
 	cf->ss_req->cls = cls;
@@ -1188,16 +1198,6 @@ static gboolean cf_ss_control(int type, const char *sc,
 		break;
 	}
 
-	/*
-	 * Some modems don't understand all classes very well, particularly
-	 * the older models.  So if the bearer class is the default, we
-	 * just use the more commonly understood value of 7 since BEARER_SMS
-	 * is not applicable to CallForwarding conditions according to 22.004
-	 * Annex A
-	 */
-	if (cls == BEARER_CLASS_SS_DEFAULT)
-		cls = BEARER_CLASS_DEFAULT;
-
 	switch (cf->ss_req->ss_type) {
 	case SS_CONTROL_TYPE_REGISTRATION:
 		string_to_phone_number(sia, &ph);
-- 
1.7.9.5

[PATCH 5/8] call-barring: Return specific errors for SS query

[Philippe Nunes]

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

Trace is modified as a CB query is not necessarily done after a CB setting
via SS.
---
 src/call-barring.c |    7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/src/call-barring.c b/src/call-barring.c
index 53847fb..68533c2 100644
--- a/src/call-barring.c
+++ b/src/call-barring.c
@@ -284,14 +284,13 @@ static void cb_ss_query_next_lock_callback(const struct ofono_error *error,
 	struct ofono_call_barring *cb = data;
 
 	if (error->type != OFONO_ERROR_TYPE_NO_ERROR) {
-		if (cb->ss_req_type != SS_CONTROL_TYPE_QUERY)
-			ofono_error("Enabling/disabling Call Barring via SS "
-					"successful, but query was not");
+		ofono_error("Query failed with error: %s",
+						telephony_error_to_str(error));
 
 		cb->flags &= ~CALL_BARRING_FLAG_CACHED;
 
 		__ofono_dbus_pending_reply(&cb->pending,
-					__ofono_error_failed(cb->pending));
+				__ofono_error_from_error(error, cb->pending));
 		return;
 	}
 
-- 
1.7.9.5

[PATCH 6/8] call-barring: class applied is the class given by SS code

[Philippe Nunes]

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

The default class is applied when no class is specified in the SS code.
---
 src/call-barring.c |    6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/src/call-barring.c b/src/call-barring.c
index 68533c2..a372792 100644
--- a/src/call-barring.c
+++ b/src/call-barring.c
@@ -308,11 +308,7 @@ static void cb_ss_query_next_lock_callback(const struct ofono_error *error,
 
 static void cb_ss_query_next_lock(struct ofono_call_barring *cb)
 {
-	int cls;
-
-	cls = cb->ss_req_cls | BEARER_CLASS_DEFAULT;
-
-	cb->driver->query(cb, cb_locks[cb->query_next].fac, cls,
+	cb->driver->query(cb, cb_locks[cb->query_next].fac, cb->ss_req_cls,
 			cb_ss_query_next_lock_callback, cb);
 }
 
-- 
1.7.9.5

[PATCH 7/8] call-settings: Return specific errors for SS query

[Philippe Nunes]

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

Trace is modified as clir query is not necessarily done after a clir
setting via SS.
---
 src/call-settings.c |    5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/src/call-settings.c b/src/call-settings.c
index 4bfb561..51f96bf 100644
--- a/src/call-settings.c
+++ b/src/call-settings.c
@@ -726,9 +726,10 @@ static void clir_ss_query_callback(const struct ofono_error *error,
 	const char *value;
 
 	if (error->type != OFONO_ERROR_TYPE_NO_ERROR) {
-		DBG("setting clir via SS failed");
+		DBG("clir query via SS failed with error: %s",
+					telephony_error_to_str(error));
 		__ofono_dbus_pending_reply(&cs->pending,
-					__ofono_error_failed(cs->pending));
+				__ofono_error_from_error(error, cs->pending));
 
 		return;
 	}
-- 
1.7.9.5

[PATCH 8/8] call-settings: class applied is the class given by SS code.

[Philippe Nunes]

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

The default class is applied when no class is specified by SS code.
---
 src/call-settings.c |   11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/src/call-settings.c b/src/call-settings.c
index 51f96bf..d63d9d8 100644
--- a/src/call-settings.c
+++ b/src/call-settings.c
@@ -485,7 +485,7 @@ static void cw_ss_set_callback(const struct ofono_error *error, void *data)
 		return;
 	}
 
-	cs->driver->cw_query(cs, BEARER_CLASS_DEFAULT,
+	cs->driver->cw_query(cs, cs->ss_req_cls,
 				cw_ss_query_callback, cs);
 }
 
@@ -533,13 +533,13 @@ static gboolean cw_ss_control(int type,
 			goto bad_format;
 	}
 
-	cs->ss_req_cls = cls;
-	cs->pending = dbus_message_ref(msg);
-
 	/* For the default case use the more readily accepted value */
 	if (cls == BEARER_CLASS_SS_DEFAULT)
 		cls = BEARER_CLASS_DEFAULT;
 
+	cs->ss_req_cls = cls;
+	cs->pending = dbus_message_ref(msg);
+
 	switch (type) {
 	case SS_CONTROL_TYPE_REGISTRATION:
 	case SS_CONTROL_TYPE_ACTIVATION:
@@ -554,8 +554,7 @@ static gboolean cw_ss_control(int type,
 		 * according to 22.004 Appendix A, so CLASS_DEFAULT
 		 * is safe to use here
 		 */
-		cs->driver->cw_query(cs, BEARER_CLASS_DEFAULT,
-					cw_ss_query_callback, cs);
+		cs->driver->cw_query(cs, cls, cw_ss_query_callback, cs);
 		break;
 
 	case SS_CONTROL_TYPE_DEACTIVATION:
-- 
1.7.9.5

Re: [PATCH 2/8] stkagent: '+' is considered as a digit

[Denis Kenzior]

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

Hi Philippe,

On 08/22/2012 11:18 AM, Philippe Nunes wrote:
> The function 'valid_phone_number_format' does not comply with GCF test cases
> expectation. For STK, the character '+' should be considered as a whole digit.
> ---
>   src/stkagent.c |   26 ++++++++++++++++++++++++--
>   1 file changed, 24 insertions(+), 2 deletions(-)
>
> diff --git a/src/stkagent.c b/src/stkagent.c
> index 7c3f697..392a2b2 100644
> --- a/src/stkagent.c
> +++ b/src/stkagent.c
> @@ -69,6 +69,27 @@ struct stk_agent {
>   #define TERMINATE_ERROR ERROR_PREFIX ".EndSession"
>   #define BUSY_ERROR ERROR_PREFIX ".Busy"
>
> +static gboolean check_digit(const char *digit)

I really don't like the name...

> +{
> +	int len = strlen(digit);
> +	int i;
> +
> +	if (!len)
> +		return FALSE;
> +
> +	for (i = 0; i<  len; i++) {
> +		if (digit[i]>= '0'&&  digit[i]<= '9')
> +			continue;
> +
> +		if (digit[i] == '*' || digit[i] == '#' || digit[i] == '+')
> +			continue;
> +
> +		return FALSE;
> +	}
> +
> +	return TRUE;

And this function is entirely too long-winded for what it is doing. 
Please refer to 'man strspn'.  It might be easier to simply use strspn 
below...

> +}
> +
>   static void stk_agent_send_noreply(struct stk_agent *agent, const char *method)
>   {
>   	DBusConnection *conn = ofono_dbus_get_connection();
> @@ -541,7 +562,7 @@ static void get_digit_cb(DBusPendingCall *call, void *data)
>   					DBUS_TYPE_STRING,&digit,
>   					DBUS_TYPE_INVALID) == FALSE ||
>   			strlen(digit) != 1 ||
> -			!valid_phone_number_format(digit)) {
> +			!check_digit(digit)) {

This is still wrong as it also needs to take care of the hidden_input 
case where the '+' is not allowed.

>   		ofono_error("Can't parse the reply to GetDigit()");
>   		remove_agent = TRUE;
>   		goto error;
> @@ -675,7 +696,8 @@ static void get_digits_cb(DBusPendingCall *call, void *data)
>
>   	if (dbus_message_get_args(reply, NULL,
>   					DBUS_TYPE_STRING,&string,
> -					DBUS_TYPE_INVALID) == FALSE) {
> +					DBUS_TYPE_INVALID) == FALSE ||
> +					!check_digit(string)) {

As above

>   		ofono_error("Can't parse the reply to GetDigits()");
>   		remove_agent = TRUE;
>   		goto error;

Regards,
-Denis

Re: [PATCH 1/8] ussd: Recover idle state in case of response sending failure

[Denis Kenzior]

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

Hi Philippe,

On 08/22/2012 11:18 AM, Philippe Nunes wrote:
> GCF test case 31.8.1.2.3 is rejecting the user response.
> Any subsequent USSD notification are not handled because USSD is always in
> state user-action.
> ---
>   src/ussd.c |    8 +++++++-
>   1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/src/ussd.c b/src/ussd.c
> index 74888b2..0ad4f61 100644
> --- a/src/ussd.c
> +++ b/src/ussd.c
> @@ -414,6 +414,10 @@ void ofono_ussd_notify(struct ofono_ussd *ussd, int status, int dcs,
>   		return;
>   	}
>
> +	if (status == OFONO_USSD_STATUS_TERMINATED&&
> +			ussd->state == USSD_STATE_IDLE)
> +		return;
> +
>   	if (status == OFONO_USSD_STATUS_NOT_SUPPORTED) {
>   		ussd_change_state(ussd, USSD_STATE_IDLE);
>

How about you simply handle the TERMINATED notification properly here and

> @@ -595,9 +599,11 @@ static void ussd_response_callback(const struct ofono_error *error, void *data)
>   	struct ofono_ussd *ussd = data;
>   	DBusMessage *reply;
>
> -	if (error->type != OFONO_ERROR_TYPE_NO_ERROR)
> +	if (error->type != OFONO_ERROR_TYPE_NO_ERROR) {
>   		DBG("ussd response failed with error: %s",
>   				telephony_error_to_str(error));
> +		ussd_change_state(ussd, USSD_STATE_IDLE);
> +	}
>
>   	if (error->type == OFONO_ERROR_TYPE_NO_ERROR) {
>   		ussd_change_state(ussd, USSD_STATE_RESPONSE_SENT);

Take out this chunk.

Regards,
-Denis

Re: [PATCH 3/8] call-forwarding: return specific errors for SS query

[Denis Kenzior]

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

Hi Philippe,

On 08/22/2012 11:18 AM, Philippe Nunes wrote:
> Trace is modified as a CF query is not necessarily done after a CF setting
> via SS.
> ---
>   src/call-forwarding.c |    5 +++--
>   1 file changed, 3 insertions(+), 2 deletions(-)
>

Patch has been applied, thanks.

Regards,
-Denis

Re: [PATCH 5/8] call-barring: Return specific errors for SS query

[Denis Kenzior]

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

Hi Philippe,

On 08/22/2012 11:18 AM, Philippe Nunes wrote:
> Trace is modified as a CB query is not necessarily done after a CB setting
> via SS.
> ---
>   src/call-barring.c |    7 +++----
>   1 file changed, 3 insertions(+), 4 deletions(-)
>

Patch has been applied, thanks.

Regards,
-Denis

Re: [PATCH 7/8] call-settings: Return specific errors for SS query

[Denis Kenzior]

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

Hi Philippe,

On 08/22/2012 11:18 AM, Philippe Nunes wrote:
> Trace is modified as clir query is not necessarily done after a clir
> setting via SS.
> ---
>   src/call-settings.c |    5 +++--
>   1 file changed, 3 insertions(+), 2 deletions(-)
>

Patch has been applied, thanks.

Regards,
-Denis

Re: [PATCH 4/8] call-forwarding: class applied is the class given by SS code

[Denis Kenzior]

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

Hi Philippe,

On 08/22/2012 11:18 AM, Philippe Nunes wrote:
> GCF test cases 31.2.1.6.1/2 are asking to make a query according a specific
> class.
> The default class is applied when no class is specified in the SS code.
> ---
>   src/call-forwarding.c |   22 +++++++++++-----------
>   1 file changed, 11 insertions(+), 11 deletions(-)
>
> diff --git a/src/call-forwarding.c b/src/call-forwarding.c
> index 91e34c6..7531f07 100644
> --- a/src/call-forwarding.c
> +++ b/src/call-forwarding.c
> @@ -1012,7 +1012,7 @@ 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,
> +	cf->driver->query(cf, cf->query_next, cf->ss_req->cls,
>   			ss_set_query_cf_callback, cf);
>   }
>
> @@ -1167,6 +1167,16 @@ static gboolean cf_ss_control(int type, const char *sc,
>   		return TRUE;
>   	}
>
> +	/*
> +	 * Some modems don't understand all classes very well, particularly
> +	 * the older models.  So if the bearer class is the default, we
> +	 * just use the more commonly understood value of 7 since BEARER_SMS
> +	 * is not applicable to CallForwarding conditions according to 22.004
> +	 * Annex A
> +	 */
> +	if (cls == BEARER_CLASS_SS_DEFAULT)
> +		cls = BEARER_CLASS_DEFAULT;
> +

Strictly speaking this is wrong.  You cannot modify the cls here as it 
is used to generate the reply in cf_ss_control_reply.  Since 
BEARER_CLASS_SS_DEFAULT != BEARER_CLASS_DEFAULT you will generate the 
wrong result.

>   	cf->ss_req->ss_type = type;
>   	cf->ss_req->cf_type = cf_type;
>   	cf->ss_req->cls = cls;
> @@ -1188,16 +1198,6 @@ static gboolean cf_ss_control(int type, const char *sc,
>   		break;
>   	}
>
> -	/*
> -	 * Some modems don't understand all classes very well, particularly
> -	 * the older models.  So if the bearer class is the default, we
> -	 * just use the more commonly understood value of 7 since BEARER_SMS
> -	 * is not applicable to CallForwarding conditions according to 22.004
> -	 * Annex A
> -	 */
> -	if (cls == BEARER_CLASS_SS_DEFAULT)
> -		cls = BEARER_CLASS_DEFAULT;
> -
>   	switch (cf->ss_req->ss_type) {
>   	case SS_CONTROL_TYPE_REGISTRATION:
>   		string_to_phone_number(sia,&ph);

Regards,
-Denis

Re: [PATCH 8/8] call-settings: class applied is the class given by SS code.

[Denis Kenzior]

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

Hi Philippe,

On 08/22/2012 11:18 AM, Philippe Nunes wrote:
> The default class is applied when no class is specified by SS code.
> ---
>   src/call-settings.c |   11 +++++------
>   1 file changed, 5 insertions(+), 6 deletions(-)
>
> diff --git a/src/call-settings.c b/src/call-settings.c
> index 51f96bf..d63d9d8 100644
> --- a/src/call-settings.c
> +++ b/src/call-settings.c
> @@ -485,7 +485,7 @@ static void cw_ss_set_callback(const struct ofono_error *error, void *data)
>   		return;
>   	}
>
> -	cs->driver->cw_query(cs, BEARER_CLASS_DEFAULT,
> +	cs->driver->cw_query(cs, cs->ss_req_cls,
>   				cw_ss_query_callback, cs);

This is only used to query after the set, why are you touching this?

>   }
>
> @@ -533,13 +533,13 @@ static gboolean cw_ss_control(int type,
>   			goto bad_format;
>   	}
>
> -	cs->ss_req_cls = cls;
> -	cs->pending = dbus_message_ref(msg);
> -
>   	/* For the default case use the more readily accepted value */
>   	if (cls == BEARER_CLASS_SS_DEFAULT)
>   		cls = BEARER_CLASS_DEFAULT;
>
> +	cs->ss_req_cls = cls;
> +	cs->pending = dbus_message_ref(msg);
> +

And again, this part is wrong since cls is used to generate the reply.

>   	switch (type) {
>   	case SS_CONTROL_TYPE_REGISTRATION:
>   	case SS_CONTROL_TYPE_ACTIVATION:
> @@ -554,8 +554,7 @@ static gboolean cw_ss_control(int type,
>   		 * according to 22.004 Appendix A, so CLASS_DEFAULT
>   		 * is safe to use here
>   		 */
> -		cs->driver->cw_query(cs, BEARER_CLASS_DEFAULT,
> -					cw_ss_query_callback, cs);
> +		cs->driver->cw_query(cs, cls, cw_ss_query_callback, cs);
>   		break;
>
>   	case SS_CONTROL_TYPE_DEACTIVATION:

Regards,
-Denis

Re: [PATCH 1/8] ussd: Recover idle state in case of response sending failure

[Philippe Nunes]

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

Hi Denis,

On 08/23/2012 01:02 AM, Denis Kenzior wrote:
> Hi Philippe,
>
> On 08/22/2012 11:18 AM, Philippe Nunes wrote:
>> GCF test case 31.8.1.2.3 is rejecting the user response.
>> Any subsequent USSD notification are not handled because USSD is
>> always in
>> state user-action.
>> ---
>> src/ussd.c | 8 +++++++-
>> 1 file changed, 7 insertions(+), 1 deletion(-)
>>
>> diff --git a/src/ussd.c b/src/ussd.c
>> index 74888b2..0ad4f61 100644
>> --- a/src/ussd.c
>> +++ b/src/ussd.c
>> @@ -414,6 +414,10 @@ void ofono_ussd_notify(struct ofono_ussd *ussd,
>> int status, int dcs,
>> return;
>> }
>>
>> + if (status == OFONO_USSD_STATUS_TERMINATED&&
>> + ussd->state == USSD_STATE_IDLE)
>> + return;
>> +
>> if (status == OFONO_USSD_STATUS_NOT_SUPPORTED) {
>> ussd_change_state(ussd, USSD_STATE_IDLE);
>>
>
> How about you simply handle the TERMINATED notification properly here and
>
>> @@ -595,9 +599,11 @@ static void ussd_response_callback(const struct
>> ofono_error *error, void *data)
>> struct ofono_ussd *ussd = data;
>> DBusMessage *reply;
>>
>> - if (error->type != OFONO_ERROR_TYPE_NO_ERROR)
>> + if (error->type != OFONO_ERROR_TYPE_NO_ERROR) {
>> DBG("ussd response failed with error: %s",
>> telephony_error_to_str(error));
>> + ussd_change_state(ussd, USSD_STATE_IDLE);
>> + }
>>
>> if (error->type == OFONO_ERROR_TYPE_NO_ERROR) {
>> ussd_change_state(ussd, USSD_STATE_RESPONSE_SENT);
>
> Take out this chunk.
>

In fact, that's what I did first.
But then, I moved to this version as it looked more logical for me.
In practice, the ussd_response_callback is handled before the TERMINATED 
notification and it is more meaningful to link the error to the 'idle' 
state recovery.
Anyway, I send you a new patch.

Regards,

Philippe.

Re: [PATCH 4/8] call-forwarding: class applied is the class given by SS code

[Philippe Nunes]

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

Hi Denis,

On 08/23/2012 01:36 AM, Denis Kenzior wrote:
> Hi Philippe,
>
> On 08/22/2012 11:18 AM, Philippe Nunes wrote:
>> GCF test cases 31.2.1.6.1/2 are asking to make a query according a
>> specific
>> class.
>> The default class is applied when no class is specified in the SS code.
>> ---
>> src/call-forwarding.c | 22 +++++++++++-----------
>> 1 file changed, 11 insertions(+), 11 deletions(-)
>>
>> diff --git a/src/call-forwarding.c b/src/call-forwarding.c
>> index 91e34c6..7531f07 100644
>> --- a/src/call-forwarding.c
>> +++ b/src/call-forwarding.c
>> @@ -1012,7 +1012,7 @@ 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,
>> + cf->driver->query(cf, cf->query_next, cf->ss_req->cls,
>> ss_set_query_cf_callback, cf);
>> }
>>
>> @@ -1167,6 +1167,16 @@ static gboolean cf_ss_control(int type, const
>> char *sc,
>> return TRUE;
>> }
>>
>> + /*
>> + * Some modems don't understand all classes very well, particularly
>> + * the older models. So if the bearer class is the default, we
>> + * just use the more commonly understood value of 7 since BEARER_SMS
>> + * is not applicable to CallForwarding conditions according to 22.004
>> + * Annex A
>> + */
>> + if (cls == BEARER_CLASS_SS_DEFAULT)
>> + cls = BEARER_CLASS_DEFAULT;
>> +
>
> Strictly speaking this is wrong. You cannot modify the cls here as it is
> used to generate the reply in cf_ss_control_reply. Since
> BEARER_CLASS_SS_DEFAULT != BEARER_CLASS_DEFAULT you will generate the
> wrong result.
>

OK, but I don't understand why we intend to generate a reply including 
classes for which we don't know the status?

With BEARER_CLASS_SS_DEFAULT, we are considering the classes 
DataSync/DataAsync/Fax/Sms/Voice.

But by default, the query is asking the status only for 
BEARER_CLASS_DEFAULT (Data/Fax/Voice).
So, why, don't we match strictly the reply with the query?

Meanwhile, I send you a new patch.

Regards,

Philippe.


>> cf->ss_req->ss_type = type;
>> cf->ss_req->cf_type = cf_type;
>> cf->ss_req->cls = cls;
>> @@ -1188,16 +1198,6 @@ static gboolean cf_ss_control(int type, const
>> char *sc,
>> break;
>> }
>>
>> - /*
>> - * Some modems don't understand all classes very well, particularly
>> - * the older models. So if the bearer class is the default, we
>> - * just use the more commonly understood value of 7 since BEARER_SMS
>> - * is not applicable to CallForwarding conditions according to 22.004
>> - * Annex A
>> - */
>> - if (cls == BEARER_CLASS_SS_DEFAULT)
>> - cls = BEARER_CLASS_DEFAULT;
>> -
>> switch (cf->ss_req->ss_type) {
>> case SS_CONTROL_TYPE_REGISTRATION:
>> string_to_phone_number(sia,&ph);
>
> Regards,
> -Denis
>

Re: [PATCH 2/8] stkagent: '+' is considered as a digit

[Philippe Nunes]

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

Hi Denis,

On 08/23/2012 12:55 AM, Denis Kenzior wrote:
> Hi Philippe,
>
> On 08/22/2012 11:18 AM, Philippe Nunes wrote:
>> The function 'valid_phone_number_format' does not comply with GCF test
>> cases
>> expectation. For STK, the character '+' should be considered as a
>> whole digit.
>> ---
>> src/stkagent.c | 26 ++++++++++++++++++++++++--
>> 1 file changed, 24 insertions(+), 2 deletions(-)
>>
>> diff --git a/src/stkagent.c b/src/stkagent.c
>> index 7c3f697..392a2b2 100644
>> --- a/src/stkagent.c
>> +++ b/src/stkagent.c
>> @@ -69,6 +69,27 @@ struct stk_agent {
>> #define TERMINATE_ERROR ERROR_PREFIX ".EndSession"
>> #define BUSY_ERROR ERROR_PREFIX ".Busy"
>>
>> +static gboolean check_digit(const char *digit)
>
> I really don't like the name...
>
>> +{
>> + int len = strlen(digit);
>> + int i;
>> +
>> + if (!len)
>> + return FALSE;
>> +
>> + for (i = 0; i< len; i++) {
>> + if (digit[i]>= '0'&& digit[i]<= '9')
>> + continue;
>> +
>> + if (digit[i] == '*' || digit[i] == '#' || digit[i] == '+')
>> + continue;
>> +
>> + return FALSE;
>> + }
>> +
>> + return TRUE;
>
> And this function is entirely too long-winded for what it is doing.
> Please refer to 'man strspn'. It might be easier to simply use strspn
> below...
>
>> +}
>> +
>> static void stk_agent_send_noreply(struct stk_agent *agent, const char
>> *method)
>> {
>> DBusConnection *conn = ofono_dbus_get_connection();
>> @@ -541,7 +562,7 @@ static void get_digit_cb(DBusPendingCall *call,
>> void *data)
>> DBUS_TYPE_STRING,&digit,
>> DBUS_TYPE_INVALID) == FALSE ||
>> strlen(digit) != 1 ||
>> - !valid_phone_number_format(digit)) {
>> + !check_digit(digit)) {
>
> This is still wrong as it also needs to take care of the hidden_input
> case where the '+' is not allowed.

Good point. But then, it requires to retrieve the command qualifier (or 
at least the hidden_val property from the agent->msg).

I think this is more complicated to perform this checking here. It 
should be more convenient to place this in the callback (here 
'request_key_cb').
But we can think also to simply remove any checking. For GET_INPUT, we 
are not checking the min/max length ;o)

Note that apparently, we missed to consider the hidden property in 
'handle_command_get_inkey'. Therefore, we are not sending this 
information to the STK agent. I'm willing to correct this.

Regards,

Philippe.




>
>> ofono_error("Can't parse the reply to GetDigit()");
>> remove_agent = TRUE;
>> goto error;
>> @@ -675,7 +696,8 @@ static void get_digits_cb(DBusPendingCall *call,
>> void *data)
>>
>> if (dbus_message_get_args(reply, NULL,
>> DBUS_TYPE_STRING,&string,
>> - DBUS_TYPE_INVALID) == FALSE) {
>> + DBUS_TYPE_INVALID) == FALSE ||
>> + !check_digit(string)) {
>
> As above
>
>> ofono_error("Can't parse the reply to GetDigits()");
>> remove_agent = TRUE;
>> goto error;
>
> Regards,
> -Denis
>

Re: [PATCH 2/8] stkagent: '+' is considered as a digit

[Denis Kenzior]

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

Hi Philippe,

>>> +}
>>> +
>>> static void stk_agent_send_noreply(struct stk_agent *agent, const char
>>> *method)
>>> {
>>> DBusConnection *conn = ofono_dbus_get_connection();
>>> @@ -541,7 +562,7 @@ static void get_digit_cb(DBusPendingCall *call,
>>> void *data)
>>> DBUS_TYPE_STRING,&digit,
>>> DBUS_TYPE_INVALID) == FALSE ||
>>> strlen(digit) != 1 ||
>>> - !valid_phone_number_format(digit)) {
>>> + !check_digit(digit)) {
>>
>> This is still wrong as it also needs to take care of the hidden_input
>> case where the '+' is not allowed.
>
> Good point. But then, it requires to retrieve the command qualifier (or
> at least the hidden_val property from the agent->msg).
>
> I think this is more complicated to perform this checking here. It
> should be more convenient to place this in the callback (here
> 'request_key_cb').
> But we can think also to simply remove any checking. For GET_INPUT, we
> are not checking the min/max length ;o)

No, we do need to sanitize any output from the agent.  We do this in a 
half assed way right now and that needs to be fixed.

>
> Note that apparently, we missed to consider the hidden property in
> 'handle_command_get_inkey'. Therefore, we are not sending this
> information to the STK agent. I'm willing to correct this.

Please do.

Regards,
-Denis

Re: [PATCH 4/8] call-forwarding: class applied is the class given by SS code

[Denis Kenzior]

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

Hi Philippe,

<snip>

>>> + /*
>>> + * Some modems don't understand all classes very well, particularly
>>> + * the older models. So if the bearer class is the default, we
>>> + * just use the more commonly understood value of 7 since BEARER_SMS
>>> + * is not applicable to CallForwarding conditions according to 22.004
>>> + * Annex A
>>> + */
>>> + if (cls == BEARER_CLASS_SS_DEFAULT)
>>> + cls = BEARER_CLASS_DEFAULT;
>>> +
>>
>> Strictly speaking this is wrong. You cannot modify the cls here as it is
>> used to generate the reply in cf_ss_control_reply. Since
>> BEARER_CLASS_SS_DEFAULT != BEARER_CLASS_DEFAULT you will generate the
>> wrong result.
>>
>
> OK, but I don't understand why we intend to generate a reply including
> classes for which we don't know the status?
>
> With BEARER_CLASS_SS_DEFAULT, we are considering the classes
> DataSync/DataAsync/Fax/Sms/Voice.

For some bizarre reason 22.030 specifies that all bearers are queried in 
the case that the mmi service code is omitted.  That means SMS as well. 
  However, half the bearers are not even applicable to call forwarding.

When reporting the results we report all bearers that were queried, even 
if some of them are not applicable or not known.  This is because we do 
not want to require the calling application to have any intelligence in 
this area.  It should simply display the results as given by oFono.

>
> But by default, the query is asking the status only for
> BEARER_CLASS_DEFAULT (Data/Fax/Voice).
> So, why, don't we match strictly the reply with the query?

We do, but querying with class is broken on many modems, hence the 
comment and why we optimize this case.  If we pass in an explicit mmi 
service code then that is what will be used instead.

If your question is why we always query with the default, the answer is 
why not?  Default literally means Voice, Data, Fax.  Where Data is 
equivalent to DataAsync | DataSync in 27.007.  So in effect we get a 
full set of results and we return only the applicable set to the 
application.  Just because the GCF testing methodology is broken in this 
area does not mean that this approach is incorrect.

Regards,
-Denis