[asterisk-dev] tilghman: branch 1.6.0 r244072 - in /branches/1.6.0: ./ channels/ main/

[Tilghman Lesher]

On Monday 01 February 2010 12:42:08 Russell Bryant wrote:
> This would have been a good candidate for reviewboard.  This is a
> non-trivial change.
>
> On 02/01/2010 11:59 AM, SVN commits to the Asterisk project wrote:
> > Author: tilghman
> > Date: Mon Feb  1 11:59:37 2010
> > New Revision: 244072
> >
> > URL: http://svnview.digium.com/svn/asterisk?view=rev&rev=244072
> > Log:
> > Merged revisions 244071 via svnmerge from
> > https://origsvn.digium.com/svn/asterisk/trunk
> >
> > ................
> >    r244071 | tilghman | 2010-02-01 11:53:39 -0600 (Mon, 01 Feb 2010) | 22
> > lines
> >
> >    Merged revisions 244070 via svnmerge from
> >    https://origsvn.digium.com/svn/asterisk/branches/1.4
> >
> >    ........
> >      r244070 | tilghman | 2010-02-01 11:46:31 -0600 (Mon, 01 Feb 2010) |
> > 16 lines
> >
> >      Revert previous chan_local fix (r236981) and fix instead by
> > destroying expired frames in the queue.
> >
> >      (closes issue #16525)
> >       Reported by: kobaz
> >       Patches:
> >             20100126__issue16525.diff.txt uploaded by tilghman (license
> > 14) 20100129__issue16525__1.6.0.diff.txt uploaded by tilghman (license
> > 14) Tested by: kobaz, atis
> >
> >      (closes issue #16581)
> >       Reported by: ZX81
> >
> >      (closes issue #16681)
> >       Reported by: alexr1
> >    ........
> > ................
> >
> > Modified:
> >      branches/1.6.0/   (props changed)
> >      branches/1.6.0/channels/chan_local.c
> >      branches/1.6.0/main/channel.c
> >
> > Propchange: branches/1.6.0/
> > -------------------------------------------------------------------------
> >----- Binary property 'trunk-merged' - no diff available.
> >
> > Modified: branches/1.6.0/channels/chan_local.c
> > URL:
> > http://svnview.digium.com/svn/asterisk/branches/1.6.0/channels/chan_local
> >.c?view=diff&rev=244072&r1=244071&r2=244072
> > =========================================================================
> >===== --- branches/1.6.0/channels/chan_local.c (original)
> > +++ branches/1.6.0/channels/chan_local.c Mon Feb  1 11:59:37 2010
> > @@ -217,9 +217,7 @@
> >   	}
> >
> >   	if (other) {
> > -		if (other->pbx || other->_bridge || !ast_strlen_zero(other->appl)) {
> > -			ast_queue_frame(other, f);
> > -		} /* else the frame won't go anywhere */
> > +		ast_queue_frame(other, f);
> >   		ast_channel_unlock(other);
> >   	}
> >
> >
> > Modified: branches/1.6.0/main/channel.c
> > URL:
> > http://svnview.digium.com/svn/asterisk/branches/1.6.0/main/channel.c?view
> >=diff&rev=244072&r1=244071&r2=244072
> > =========================================================================
> >===== --- branches/1.6.0/main/channel.c (original)
> > +++ branches/1.6.0/main/channel.c Mon Feb  1 11:59:37 2010
> > @@ -1028,22 +1028,22 @@
> >   		}
> >   	}
> >
> > -	if ((queued_frames + new_frames)>  128) {
> > -		ast_log(LOG_WARNING, "Exceptionally long queue length queuing to
> > %s\n", chan->name); -		while ((f = AST_LIST_REMOVE_HEAD(&frames,
> > frame_list))) {
> > -			ast_frfree(f);
> > -		}
> > -		ast_channel_unlock(chan);
> > -		return 0;
> > -	}
> > -
> > -	if ((queued_voice_frames + new_voice_frames)>  96) {
> > -		ast_log(LOG_WARNING, "Exceptionally long voice queue length queuing to
> > %s\n", chan->name); -		while ((f = AST_LIST_REMOVE_HEAD(&frames,
> > frame_list))) {
> > -			ast_frfree(f);
> > -		}
> > -		ast_channel_unlock(chan);
> > -		return 0;
> > +	if ((queued_frames + new_frames>  128 || queued_voice_frames +
> > new_voice_frames>  96)) { +		int count = 0;
> > +		ast_log(LOG_WARNING, "Exceptionally long %squeue length queuing to
> > %s\n", queued_frames + new_frames>  128 ? "" : "voice ", chan->name);
> > +		AST_LIST_TRAVERSE_SAFE_BEGIN(&chan->readq, cur, frame_list) { +			/*
> > Save the most recent frame */
> > +			if (!AST_LIST_NEXT(cur, frame_list)) {
> > +				break;
> > +			} else if (cur->frametype == AST_FRAME_VOICE || cur->frametype ==
> > AST_FRAME_VIDEO || cur->frametype == AST_FRAME_NULL) { +				if (++count> 
> > 64) {
> > +					break;
> > +				}
> > +				AST_LIST_REMOVE_CURRENT(frame_list);
> > +				ast_frfree(cur);
> > +			}
> > +		}
> > +		AST_LIST_TRAVERSE_SAFE_END;
> >   	}
> >
> >   	if (after) {
>
> I don't think this is really a fix.  First, it has some coding
> guidelines violations (missing spaces around '>').  However, it puts the
> code back to where that message is going to come up and freak people out
> in the same situations that it did before.  Please fill me in if I am
> missing something.

Well, yes, it will, but the error wasn't entirely removed, either.  Atis
remarked on this and suggested yet another exception to the rule.  This
tends to suggest that with so many exceptions, the previous approach wasn't
the right way to do this.  With the patch above, the error may still occur,
but it will only occur once in a brief while, not constantly, once the limit
has been reached.

-- 
Tilghman Lesher
Digium, Inc. | Senior Software Developer
twitter: Corydon76 | IRC: Corydon76-dig (Freenode)
Check us out at: www.digium.com & www.asterisk.org