[PATCH] stk: Clear 'respond_on_exit' flag after sending the terminal response

[PATCH] stk: Clear 'respond_on_exit' flag after sending the terminal response

[Philippe Nunes]

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

---
 src/stk.c |   27 ++-------------------------
 1 files changed, 2 insertions(+), 25 deletions(-)

diff --git a/src/stk.c b/src/stk.c
index c86cbfb..8214b65 100644
--- a/src/stk.c
+++ b/src/stk.c
@@ -128,6 +128,7 @@ static int stk_respond(struct ofono_stk *stk, struct stk_response *rsp,
 	stk_command_free(stk->pending_cmd);
 	stk->pending_cmd = NULL;
 	stk->cancel_cmd = NULL;
+	stk->respond_on_exit = FALSE;
 
 	stk->driver->terminal_response(stk, tlv_len, tlv, cb, stk);
 
@@ -477,10 +478,8 @@ static void user_termination_cb(enum stk_agent_result result, void *user_data)
 {
 	struct ofono_stk *stk = user_data;
 
-	if (result == STK_AGENT_RESULT_TERMINATE) {
-		stk->respond_on_exit = FALSE;
+	if (result == STK_AGENT_RESULT_TERMINATE)
 		send_simple_response(stk, STK_RESULT_TYPE_USER_TERMINATED);
-	}
 }
 
 static void stk_alpha_id_set(struct ofono_stk *stk,
@@ -598,7 +597,6 @@ static void default_agent_notify(gpointer user_data)
 
 	stk->default_agent = NULL;
 	stk->current_agent = stk->session_agent;
-	stk->respond_on_exit = FALSE;
 }
 
 static void session_agent_notify(gpointer user_data)
@@ -617,7 +615,6 @@ static void session_agent_notify(gpointer user_data)
 
 	stk->session_agent = NULL;
 	stk->current_agent = stk->default_agent;
-	stk->respond_on_exit = FALSE;
 
 	if (stk->remove_agent_source) {
 		g_source_remove(stk->remove_agent_source);
@@ -1159,8 +1156,6 @@ static void request_selection_cb(enum stk_agent_result result, uint8_t id,
 {
 	struct ofono_stk *stk = user_data;
 
-	stk->respond_on_exit = FALSE;
-
 	switch (result) {
 	case STK_AGENT_RESULT_OK:
 	{
@@ -1243,8 +1238,6 @@ static void display_text_cb(enum stk_agent_result result, void *user_data)
 	static unsigned char screen_busy_result[] = { 0x01 };
 	static struct ofono_error error = { .type = OFONO_ERROR_TYPE_FAILURE };
 
-	stk->respond_on_exit = FALSE;
-
 	/*
 	 * There are four possible paths for DisplayText with immediate
 	 * response flag set:
@@ -1386,8 +1379,6 @@ static void request_confirmation_cb(enum stk_agent_result result,
 	struct stk_command_get_inkey *cmd = &stk->pending_cmd->get_inkey;
 	struct stk_response rsp;
 
-	stk->respond_on_exit = FALSE;
-
 	switch (result) {
 	case STK_AGENT_RESULT_OK:
 		memset(&rsp, 0, sizeof(rsp));
@@ -1442,8 +1433,6 @@ static void request_key_cb(enum stk_agent_result result, char *string,
 	struct stk_command_get_inkey *cmd = &stk->pending_cmd->get_inkey;
 	struct stk_response rsp;
 
-	stk->respond_on_exit = FALSE;
-
 	switch (result) {
 	case STK_AGENT_RESULT_OK:
 		memset(&rsp, 0, sizeof(rsp));
@@ -1560,8 +1549,6 @@ static void request_string_cb(enum stk_agent_result result, char *string,
 	gboolean packed = (qualifier & (1 << 3)) != 0;
 	struct stk_response rsp;
 
-	stk->respond_on_exit = FALSE;
-
 	switch (result) {
 	case STK_AGENT_RESULT_OK:
 		memset(&rsp, 0, sizeof(rsp));
@@ -1699,8 +1686,6 @@ static void confirm_call_cb(enum stk_agent_result result, gboolean confirm,
 	struct stk_response rsp;
 	int err;
 
-	stk->respond_on_exit = FALSE;
-
 	switch (result) {
 	case STK_AGENT_RESULT_TIMEOUT:
 		confirm = FALSE;
@@ -2283,8 +2268,6 @@ static void send_dtmf_cancel(struct ofono_stk *stk)
 	struct ofono_voicecall *vc = NULL;
 	struct ofono_atom *vc_atom;
 
-	stk->respond_on_exit = FALSE;
-
 	vc_atom = __ofono_modem_find_atom(__ofono_atom_get_modem(stk->atom),
 						OFONO_ATOM_TYPE_VOICECALL);
 	if (vc_atom)
@@ -2300,8 +2283,6 @@ static void dtmf_sent_cb(int error, void *user_data)
 {
 	struct ofono_stk *stk = user_data;
 
-	stk->respond_on_exit = FALSE;
-
 	stk_alpha_id_unset(stk);
 
 	if (error == ENOENT) {
@@ -2413,8 +2394,6 @@ static void play_tone_cb(enum stk_agent_result result, void *user_data)
 {
 	struct ofono_stk *stk = user_data;
 
-	stk->respond_on_exit = FALSE;
-
 	switch (result) {
 	case STK_AGENT_RESULT_OK:
 	case STK_AGENT_RESULT_TIMEOUT:
@@ -2547,8 +2526,6 @@ static void confirm_launch_browser_cb(enum stk_agent_result result,
 	struct ofono_error failure = { .type = OFONO_ERROR_TYPE_FAILURE };
 	struct stk_response rsp;
 
-	stk->respond_on_exit = FALSE;
-
 	switch (result) {
 	case STK_AGENT_RESULT_TIMEOUT:
 		confirm = FALSE;
-- 
1.7.1

Re: [PATCH] stk: Clear 'respond_on_exit' flag after sending the terminal response

[Andrzej Zaborowski]

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

Hi Philippe,

On 14 April 2011 14:40, Philippe Nunes <philippe.nunes@linux.intel.com> wrote:
> ---
>  src/stk.c |   27 ++-------------------------
>  1 files changed, 2 insertions(+), 25 deletions(-)
>
> diff --git a/src/stk.c b/src/stk.c
> index c86cbfb..8214b65 100644
> --- a/src/stk.c
> +++ b/src/stk.c
> @@ -128,6 +128,7 @@ static int stk_respond(struct ofono_stk *stk, struct stk_response *rsp,
>        stk_command_free(stk->pending_cmd);
>        stk->pending_cmd = NULL;
>        stk->cancel_cmd = NULL;
> +       stk->respond_on_exit = FALSE;
>
>        stk->driver->terminal_response(stk, tlv_len, tlv, cb, stk);
>

Overall this looks good.

> @@ -477,10 +478,8 @@ static void user_termination_cb(enum stk_agent_result result, void *user_data)
>  {
>        struct ofono_stk *stk = user_data;
>
> -       if (result == STK_AGENT_RESULT_TERMINATE) {
> -               stk->respond_on_exit = FALSE;
> +       if (result == STK_AGENT_RESULT_TERMINATE)
>                send_simple_response(stk, STK_RESULT_TYPE_USER_TERMINATED);
> -       }
>  }
>
>  static void stk_alpha_id_set(struct ofono_stk *stk,
> @@ -598,7 +597,6 @@ static void default_agent_notify(gpointer user_data)
>
>        stk->default_agent = NULL;
>        stk->current_agent = stk->session_agent;
> -       stk->respond_on_exit = FALSE;
>  }
>
>  static void session_agent_notify(gpointer user_data)
> @@ -617,7 +615,6 @@ static void session_agent_notify(gpointer user_data)
>
>        stk->session_agent = NULL;
>        stk->current_agent = stk->default_agent;
> -       stk->respond_on_exit = FALSE;
>
>        if (stk->remove_agent_source) {
>                g_source_remove(stk->remove_agent_source);
> @@ -1159,8 +1156,6 @@ static void request_selection_cb(enum stk_agent_result result, uint8_t id,
>  {
>        struct ofono_stk *stk = user_data;
>
> -       stk->respond_on_exit = FALSE;
> -
>        switch (result) {
>        case STK_AGENT_RESULT_OK:
>        {
> @@ -1243,8 +1238,6 @@ static void display_text_cb(enum stk_agent_result result, void *user_data)
>        static unsigned char screen_busy_result[] = { 0x01 };
>        static struct ofono_error error = { .type = OFONO_ERROR_TYPE_FAILURE };
>
> -       stk->respond_on_exit = FALSE;
> -
>        /*
>         * There are four possible paths for DisplayText with immediate
>         * response flag set:
> @@ -1386,8 +1379,6 @@ static void request_confirmation_cb(enum stk_agent_result result,
>        struct stk_command_get_inkey *cmd = &stk->pending_cmd->get_inkey;
>        struct stk_response rsp;
>
> -       stk->respond_on_exit = FALSE;
> @@ -1442,8 +1433,6 @@ static void request_key_cb(enum stk_agent_result result, char *string,
>        struct stk_command_get_inkey *cmd = &stk->pending_cmd->get_inkey;
>        struct stk_response rsp;
>
> -       stk->respond_on_exit = FALSE;
> -
>        switch (result) {
>        case STK_AGENT_RESULT_OK:
>                memset(&rsp, 0, sizeof(rsp));
> @@ -1560,8 +1549,6 @@ static void request_string_cb(enum stk_agent_result result, char *string,
>        gboolean packed = (qualifier & (1 << 3)) != 0;
>        struct stk_response rsp;
>
> -       stk->respond_on_exit = FALSE;
> -
>        switch (result) {
>        case STK_AGENT_RESULT_OK:
>                memset(&rsp, 0, sizeof(rsp));
> @@ -1699,8 +1686,6 @@ static void confirm_call_cb(enum stk_agent_result result, gboolean confirm,
>        struct stk_response rsp;
>        int err;
>
> -       stk->respond_on_exit = FALSE;

However there's a change in behaviour here which I think is incorrect.
 We need to wait for the call to be cancelled or connected before
responding, so I'd just leave this line as is.

Best regards

Re: [PATCH] stk: Clear 'respond_on_exit' flag after sending the terminal response

[Philippe Nunes]

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

Hi Andrew,

Thank you for your review.

>> @@ -1699,8 +1686,6 @@ static void confirm_call_cb(enum stk_agent_result result, gboolean confirm,
>>         struct stk_response rsp;
>>         int err;
>>
>> -       stk->respond_on_exit = FALSE;
>
> However there's a change in behaviour here which I think is incorrect.
>   We need to wait for the call to be cancelled or connected before
> responding, so I'd just leave this line as is.
>



According to me, this change doesn't impact your expectation.
It offers only the possibility for the user to end properly the session 
when exiting the on screen STK agent and before the dialer App becomes 
foreground. I know, this could be unlikely to happen due to timing 
consideration but still, I think this change is more safe.
Indeed, when the STK agent quits, normally the callback 
"session_agent_notify" is called. If the flag respond_on exit is set to 
FALSE, no chance to cancel the command neither to send the appropriate 
terminal response with the result "user ends the session".

Now, indeed, the user is still able to cancel the call in the dialer App 
(and a terminal response will be sent at the end) but I feel we missed 
to consider first the STK agent exit.

Also, you mentioned:
"the spec only mentions the "session terminated by the user" response in 
the "confirmation phase" and not in the "setup phase".

According to me, the session still exists during the setup phase, 
therefore, the result "session terminated by the user" should be possible.
Also, it's more logical to return "use cleared down call before 
connection" when the user decides to cancel the call, this way the SAP 
application can propose alternatively the previous sub-menu and still 
keep active the session.
Here, the proposed change is precisely linked with the STK agent exit, 
it means that the user wants indeed to cancel the call but he wants 
first to end the session. That's why I think the result "session 
terminated by the user" is also relevant during the setup phase.

Regards,

Philippe.

Re: [PATCH] stk: Clear 'respond_on_exit' flag after sending the terminal response

[Andrzej Zaborowski]

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

Hi,

On 20 April 2011 18:06, Philippe Nunes <philippe.nunes@linux.intel.com> wrote:
>>> @@ -1699,8 +1686,6 @@ static void confirm_call_cb(enum stk_agent_result
>>> result, gboolean confirm,
>>>        struct stk_response rsp;
>>>        int err;
>>>
>>> -       stk->respond_on_exit = FALSE;
>>
>> However there's a change in behaviour here which I think is incorrect.
>>  We need to wait for the call to be cancelled or connected before
>> responding, so I'd just leave this line as is.
>>
>
> According to me, this change doesn't impact your expectation.
> It offers only the possibility for the user to end properly the session when
> exiting the on screen STK agent and before the dialer App becomes
> foreground. I know, this could be unlikely to happen due to timing
> consideration but still, I think this change is more safe.

The problem is not that it's unlikely, but by my reading of the
specification the user should be able to quit the STK session without
disturbing the call setup.

> Indeed, when the STK agent quits, normally the callback
> "session_agent_notify" is called. If the flag respond_on exit is set to
> FALSE, no chance to cancel the command neither to send the appropriate
> terminal response with the result "user ends the session".
>
> Now, indeed, the user is still able to cancel the call in the dialer App
> (and a terminal response will be sent at the end) but I feel we missed to
> consider first the STK agent exit.
>
> Also, you mentioned:
> "the spec only mentions the "session terminated by the user" response in the
> "confirmation phase" and not in the "setup phase".
>
> According to me, the session still exists during the setup phase, therefore,

Yes it exists, and if the user closes the session then in this case
nothing should happen (same as during Send DTMF or other commands).
TS 102.223 6.4 lists the allowed responses for every scenario and if
we want to follow it, then the current behaviour is more correct than
the proposed one.

All in all I'm okay with this patch because the change is minor but
whenever there's a behaviour change the reasoning needs to be
documented in the commit message optimally with reference to the
specification. (possibly in the code too)

Best regards

Re: [PATCH] stk: Clear 'respond_on_exit' flag after sending the terminal response

[Philippe Nunes]

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

Hi Andrew,


On 04/20/2011 09:11 PM, Andrzej Zaborowski wrote:
> Hi,
>
> On 20 April 2011 18:06, Philippe Nunes<philippe.nunes@linux.intel.com>  wrote:
>>>> @@ -1699,8 +1686,6 @@ static void confirm_call_cb(enum stk_agent_result
>>>> result, gboolean confirm,
>>>>         struct stk_response rsp;
>>>>         int err;
>>>>
>>>> -       stk->respond_on_exit = FALSE;
>>>
>>> However there's a change in behaviour here which I think is incorrect.
>>>   We need to wait for the call to be cancelled or connected before
>>> responding, so I'd just leave this line as is.
>>>
>>
>> According to me, this change doesn't impact your expectation.
>> It offers only the possibility for the user to end properly the session when
>> exiting the on screen STK agent and before the dialer App becomes
>> foreground. I know, this could be unlikely to happen due to timing
>> consideration but still, I think this change is more safe.
>
> The problem is not that it's unlikely, but by my reading of the
> specification the user should be able to quit the STK session without
> disturbing the call setup.
>
>> Indeed, when the STK agent quits, normally the callback
>> "session_agent_notify" is called. If the flag respond_on exit is set to
>> FALSE, no chance to cancel the command neither to send the appropriate
>> terminal response with the result "user ends the session".
>>
>> Now, indeed, the user is still able to cancel the call in the dialer App
>> (and a terminal response will be sent at the end) but I feel we missed to
>> consider first the STK agent exit.
>>
>> Also, you mentioned:
>> "the spec only mentions the "session terminated by the user" response in the
>> "confirmation phase" and not in the "setup phase".
>>
>> According to me, the session still exists during the setup phase, therefore,
>
> Yes it exists, and if the user closes the session then in this case
> nothing should happen (same as during Send DTMF or other commands).
> TS 102.223 6.4 lists the allowed responses for every scenario and if
> we want to follow it, then the current behaviour is more correct than
> the proposed one.
>

For Send DTMF proactive command, I can read "If the user indicates the 
need to end the proactive UICC session whilst the terminal is sending 
the DTMF string, the terminal shall stop sending the DTMF string and 
shall send a terminal response with "proactive UICC session terminated 
by the user" result value. That's precisely what is allowing this change.

For setup call, the specification is not so clear but since this result 
code is allowed for SETUP CALL, I guess that the spec should have 
pointed that it was however not possible to send this result code during 
the call setup.

For me, this change brings at least an homogeneous behavior.


> All in all I'm okay with this patch because the change is minor but
> whenever there's a behaviour change the reasoning needs to be
> documented in the commit message optimally with reference to the
> specification. (possibly in the code too)
>

I agree. I didn't realize that this change could be problematic.

Regards,

Philippe.