Re: [PATCH v3 2/7] stk: Introduce BIP command handlers

[Philippe Nunes <philippe.nunes@linux.intel.com>]

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


Hi Andrew,

>> +
>> +       stk->tx_buffer.data.array = g_try_malloc(oc->buf_size);
>> +       if (stk->tx_buffer.data.array == NULL) {
>> +               rsp.result.type = STK_RESULT_TYPE_NOT_CAPABLE;
>> +               goto out;
>> +       }
>
> A minor nitpick: we tend to return one of the temporary error codes
> when out of memory.
>

OK, I will return in this case the general result 
"STK_RESULT_TYPE_TERMINAL_BUSY" with the additional info "no specific 
cause".

>> +static void confirm_open_channel_cb(enum stk_agent_result result,
>> +                                       gboolean confirm,
>> +                                       void *user_data)
>> +{
>> +       struct ofono_stk *stk = user_data;
>> +
>> +       switch (result) {
>> +       case STK_AGENT_RESULT_TERMINATE:
>> +               send_simple_response(stk, STK_RESULT_TYPE_USER_TERMINATED);
>> +               return;
>> +
>> +       case STK_AGENT_RESULT_TIMEOUT:
>> +               confirm = FALSE;
>> +               /* Fall through */
>> +
>> +       case STK_AGENT_RESULT_OK:
>> +               if (confirm)
>> +                       break;
>> +               /* Fall through */
>
> According to 6.4.27 we should return STK_RESULT_TYPE_USER_REJECT if
> the user does not accept the channel setup.
>
>> +
>> +       default:
>> +               send_simple_response(stk, STK_RESULT_TYPE_TERMINAL_BUSY);
>> +               return;
>> +       }
>
> I think Jeevaka determined that this needs an additional info field.


In practice, I applied the same code than "confirm_launch_browser_cb". 
But you're right, for Open channel, we should return the terminal 
response "User did not accept the proactive command".

For the additional info field, I just simplified the code seen in 
"confirm_launch_browser_cb" as in this case, no specific cause is given.
But your remark is true, it is mandatory to return an additional 
information in case of general result "STK_RESULT_TYPE_TERMINAL_BUSY".

To clean both issues you raised and for an homogeneous behavior, I 
propose to follow the code written in "confirm_call_cb".Thus, in case of 
no answer, I will return STK_RESULT_TYPE_USER_TERMINATED and not 
STK_RESULT_TYPE_TERMINAL_BUSY.

Thank you for your review.
Best regards,

Philippe.