[Home]

Summary:ASTERISK-20109: get_ast_cmd doesn't differentiate between failure and empty list return
Reporter:Jeremiah Gowdy (jgowdy)Labels:
Date Opened:2012-07-09 16:46:13Date Closed:2012-09-25 09:56:27
Priority:MinorRegression?
Status:Closed/CompleteComponents:Resources/res_agi
Versions:10.5.1 Frequency of
Occurrence
Related
Issues:
Environment:openSuSE 11.4 64bit on Dell M610 bladeAttachments:( 0) jgowdy-7-9-2012.diff
Description:In res_agi, the get_ast_cmd method returns NULL in the case of "Async AGI datastore disappeared" and in the fairly normal case of empty work list.  In the current code, when the datastore disappears (which I've yet to determine why), the at least one of the channels in the associated bridge tend to survive for approximately 30 seconds with repeated broken pipe errors.

By changing the return value to int and taking the struct pointer by pointer, we are able to properly convey the different conditions to the caller, and thus fail much more quickly.  
Comments:By: Jeremiah Gowdy (jgowdy) 2012-07-09 16:47:06.121-0500

Attached is the patch to provide faster failure in this scenario.
By: Rusty Newton (rnewton) 2012-07-10 16:40:11.666-0500

Jeremiah, thanks for finding this. If you are able to provide a patch against trunk that would be great.
By: Jonathan Rose (jrose) 2012-09-18 10:39:36.333-0500

The third segment of this patch has some fundamental wrongness in it making it completely broken. I think the patch is perfectly salvageable though.

the following condition:
 (!hungup && !(res=get_agi_cmd(...))

implies !res, so the only way to enter this while loop is in the way that causes the if statement to occur, which means at a bare minimum that the rest of this loop will never run.

Is what you meant to put there if (!cmd) instead maybe?

From the looks of it, this patch in its current form will break the nominal behavior of this while loop entirely.

Please patch against trunk if/when you decide to pick this back up in the future.

I'm going to go ahead and change the !res condition to !cmd since I'm pretty sure that's what you are aiming for and push this out for some more review.
By: Matt Jordan (mjordan) 2012-09-18 10:46:46.433-0500

No, that's probably not what he meant.  I'll comment on the review.
By: Jeremiah Gowdy (jgowdy) 2012-09-18 12:25:31.252-0500

Ouch, nice catch.  Thank you Jonathan.
By: Matt Jordan (mjordan) 2012-09-18 12:37:47.349-0500

If that's the case, then you probably don't need to change {{get_agi_cmd}} at all.  You probably only need:

{noformat}
+ if (!cmd) {
+ ast_log(LOG_ERROR, "Failed to get AGI command on channel %s: %s\n",chan->name,strerror(errno));
+ returnstatus = AGI_RESULT_FAILURE;
+ goto async_agi_done;
+ }
{noformat}

After the while() loop itself but before the {{if (!hangup)}} check.
By: Jonathan Rose (jrose) 2012-09-18 13:22:18.209-0500

Jeremiah:

Hey, I have an update to this patch that I would like you to evaluate for me. Could you give it a quick test run?

https://reviewboard.asterisk.org/r/2117/diff/raw/

The patch is against trunk and required a minor change to account for the channel opacification patches in 11 and above, so you will either need to use 11 or trunk to test the patch or you'll have to modify the patch to remove the ast_channel_name function and replace it with chan->name.
By: Jonathan Rose (jrose) 2012-09-21 10:45:43.935-0500

* Requesting confirmation that the most recently reviewed patch noted in comments works for the reporter