Re: [isi-voicecall-fix PATCHv3 1/6] voicecall: fix dial result handling

[Denis Kenzior <denkenz@gmail.com>]

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

Hi Pekka,

>>> +     gboolean dial_result_handled;
>>>  };

So I'm finally beginning to understand where you're going with this..

>>>
>>>  struct dial_request {
>>> @@ -1096,9 +1097,20 @@ static struct voicecall *dial_handle_result(struct ofono_voicecall *vc,
>>>               v = l->data;
>>>
>>>               if (v->call->status == CALL_STATUS_DIALING ||
>>> -                             v->call->status == CALL_STATUS_ALERTING ||
>>> -                             v->call->status == CALL_STATUS_ACTIVE)
>>> +                             v->call->status == CALL_STATUS_ALERTING)
>>>                       return v;

So if I understand correctly, you want to set dial_result_handled to
TRUE here as well, right?

>>> +
>>> +             /*
>>> +              * Dial request may return before existing active call
>>> +              * is put on hold or after dialed call has got active
>>> +              */
>>> +             if (v->call->status == CALL_STATUS_ACTIVE &&
>>> +                             v->call->direction ==
>>> +                             CALL_DIRECTION_MOBILE_ORIGINATED &&
>>> +                             !v->dial_result_handled) {
>>> +                     v->dial_result_handled = TRUE;
>>> +                     return v;
>>> +             }
>>
>> I really don't see how this can work, since you never reset
>> dial_result_handled to FALSE anywhere.
> 
> Huh? voicecall_create() uses g_new0().
> 

So that is the piece I was missing.  Now I understand, thanks.

> For each outbound call there should be exactly one dial request, once
> that is returned in dial_handle_result(), the dial_result_handled
> should be TRUE.
> 
> I'll fix the above code.

Are you referring to setting dial_result_handled to TRUE for
ALERTING/DIALING or something else?

> 
>>>       }
>>>
>>>       call = synthesize_outgoing_call(vc, number);
>>> @@ -1119,6 +1131,8 @@ static struct voicecall *dial_handle_result(struct ofono_voicecall *vc,
>>>
>>>       *need_to_emit = TRUE;
>>>
>>> +     v->dial_result_handled = TRUE;
>>> +
>>
>> dial_handle_result is ever called once, why do you need to set this here?
> 
> This prevents mixing this call with the call dialed next.
>

And this one makes sense now as well.  So I'm actually fine with the
fix.  Do you want to resend the patch?

Regards,
-Denis