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

Hi Denis, On 08/23/2012 12:55 AM, Denis Kenzior wrote: 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.

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

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

Hi Denis, On 08/23/2012 01:36 AM, Denis Kenzior wrote: 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.

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

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

Hi Philippe, On 08/22/2012 11:18 AM, Philippe Nunes wrote: This is only used to query after the set, why are you touching this? And again, this part is wrong since cls is used to generate the reply. Regards, -Denis

Hi Denis, On 08/23/2012 01:02 AM, Denis Kenzior wrote: 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.