[asterisk-dev] [Code Review] res_agi: Review improvement for get_agi_cmd to differentiate between an error and an empty command list

[jrose]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/2117/#review7081
-----------------------------------------------------------



/trunk/res/res_agi.c
<https://reviewboard.asterisk.org/r/2117/#comment13688>

    The primary problem I noticed with this patch were the conditions of the while loop and if statement here.
    
    In order for the while loop to be entered, the res value from get_agi_cmd must be 0, so unconditionally the if (!res) condition will also be true. This means everything outside of this failure condition has been rendered unreachable.
    
    I thought the if condition might have been intended to be resolved with if (!cmd). In retrospect I see why this might not be the case. Instead perhaps the while loop should be changed to just use the (!hungup) condition and then res = get_agi_cmd(chan, &cmd) should be executed within the loop. It also looks to me though like having a NULL value for cmd would also likely cause a segfault if it entered that loop. Perhaps the if (!res) should remain, but also add:
    if (!cmd) { break; }
    
    or something similar to that anyway.



/trunk/res/res_agi.c
<https://reviewboard.asterisk.org/r/2117/#comment13687>

    This line is changed from the original patch to account for the channel opacification changes present in 11/trunk that weren't in 1.8.


- jrose


On Sept. 18, 2012, 12:26 p.m., jrose wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/2117/
> -----------------------------------------------------------
> 
> (Updated Sept. 18, 2012, 12:26 p.m.)
> 
> 
> Review request for Asterisk Developers, Mark Michelson and Matt Jordan.
> 
> 
> Summary
> -------
> 
> This patch has a couple problems, but the aim is to improve the performance/responsiveness of launch_asyncagi when it calls the get_agi_cmd function. The reporter explained that prior to the patch an empty command list would result in a channel hanging for approximately 30 seconds with broken pipe errors.
> 
> There appear to be some problems with this patch which need to be worked out.
> 
> 
> This addresses bug ASTERISK-20109.
>     https://issues.asterisk.org/jira/browse/ASTERISK-20109
> 
> 
> Diffs
> -----
> 
>   /trunk/res/res_agi.c 373118 
> 
> Diff: https://reviewboard.asterisk.org/r/2117/diff
> 
> 
> Testing
> -------
> 
> Untested on my end. From what the patcher had written and based on what I've looked over of the code so far, I'm guessing he had probably tested what happens when no agi commands are available, but not much else.
> 
> 
> Thanks,
> 
> jrose
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20120918/043b6189/attachment-0001.htm>