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

[Matt Jordan]

> On Sept. 18, 2012, 12:34 p.m., jrose wrote:
> > /trunk/res/res_agi.c, lines 1326-1331
> > <https://reviewboard.asterisk.org/r/2117/diff/1/?file=31278#file31278line1326>
> >
> >     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.

So, yes.  We shouldn't need to change get_agi_cmd.

If get_agi_cmd returns NULL, then we want to handle the lack of a command before we check whether or not we got hungup.  So the check should be removed out of the while() loop and instead be:

if (!cmd) {
  ...
  returnstatus = AGI_RESULT_FAILURE;
  goto async_agi_done;
} else if (!hungup) {
  /* Wait a bit for a frame to read or to poll for a new command. */
  res = ast_waitfor(chan, timeout);
  ...
}


- Matt


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


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/864d328f/attachment.htm>