ASTERISK-14621: [patch] different 'ringt' timeout counting styles througout chan_dahdi/sig

[Home]

Summary: ASTERISK-14621: [patch] different 'ringt' timeout counting styles througout chan_dahdi/sig_analog code

Reporter: Alec Davis (alecdavis) Labels:

Date Opened: 2009-08-09 05:29:02 Date Closed: 2009-10-07 15:12:05

Priority: Trivial Regression? No

Status: Closed/Complete Components: Channels/chan_dahdi

Versions: Frequency of
Occurrence

Related
Issues:

Environment: Attachments: ( 0) chan_dahdi.bug15684.diff2.txt

Description: Below are the three styles I've found.
All trying to achive very close to the same thing, to timeout 'ringt'. <pre>
Style 1:
if (analog_p->ringt) {
analog_p->ringt--;
}
if (analog_p->ringt == 1) {
return -1;
}

Style 2:
if (p->ringt)
p->ringt--;
if (p->ringt == 1) {
res = -1;
break;
}
}

Style 3:
if (p->ringt == 1) {
ast_mutex_unlock(&p->lock);
return NULL;
}
else if (p->ringt > 0)
p->ringt--;
</pre>

****** ADDITIONAL INFORMATION ******

The way I see it:
Style 1 and 2, if for whatever reason 'ringt=1' when invoked the code will bypass the expected code for 'timeout'.

Style 3, if for whatever reason 'ringt=0' when invoked the code will bypass the expected code for 'timeout' and continue to execute code.

It makes more sense in the above cases for ringt to be like below, covering 0,1 and negative numbers as timeout values. <pre>
if (p->ringt > 1) {
p->ringt--;
}
if (p->ringt < 2) {
res = -1;
break;
}</pre>

Comments: By: Alec Davis (alecdavis) 2009-08-09 05:40:01

But if all we are trying to do is 'break' or 'return' if either 'just timedout' or 'not yet set' then the code becomes; <pre>
if (p->ringt > 0) {
p->ringt--;
}
if (p->ringt < 1) {
res = -1;
break;
}</pre>

uploaded chan_dahdi.bug15684.diff.txt to reflect this style (untested!)

By: Alec Davis (alecdavis) 2009-08-10 03:54:50

please remove chan_dahdi.bug15684.diff.txt

uploaded chan_dahdi.bug15684.diff2.txt
uses the style below<pre>
if (p->ringt > 0) {
if (!(--p->ringt)) {
ast_mutex_unlock(&p->lock);
return NULL;
}
}</pre>
Further study of code and I realised we only want to execute the timeout code if we've had a previous ring event.

tested for ringtimeout (aborted inbound calls).
tested callerid is correctly read.
By: Alec Davis (alecdavis) 2009-08-18 16:16:56

please remove chan_dahdi.bug15684.diff.txt

chan_dahdi.bug15684.diff2.txt has been in production for over a week now.
By: Digium Subversion (svnbot) 2009-10-07 15:11:34

Repository: asterisk
Revision: 222652

U trunk/channels/chan_dahdi.c

------------------------------------------------------------------------
r222652 | jpeeler | 2009-10-07 15:11:33 -0500 (Wed, 07 Oct 2009) | 8 lines

Change ringt (ring timeout) styles to be consistent across chan_dahdi.

(closes issue ASTERISK-14621)
Reported by: alecdavis
Patches:
chan_dahdi.bug15684.diff2.txt uploaded by alecdavis (license 585)
Tested by: alecdavis

------------------------------------------------------------------------

http://svn.digium.com/view/asterisk?view=rev&revision=222652
By: Digium Subversion (svnbot) 2009-10-07 15:12:04

Repository: asterisk
Revision: 222653

_U branches/1.6.2/

------------------------------------------------------------------------
r222653 | jpeeler | 2009-10-07 15:12:04 -0500 (Wed, 07 Oct 2009) | 14 lines

Blocked revisions 222652 via svnmerge

........
r222652 | jpeeler | 2009-10-07 15:08:14 -0500 (Wed, 07 Oct 2009) | 8 lines

Change ringt (ring timeout) styles to be consistent across chan_dahdi.

(closes issue ASTERISK-14621)
Reported by: alecdavis
Patches:
chan_dahdi.bug15684.diff2.txt uploaded by alecdavis (license 585)
Tested by: alecdavis
........

------------------------------------------------------------------------

http://svn.digium.com/view/asterisk?view=rev&revision=222653