[Home]

Summary:ASTERISK-13605: [patch] cleanup bridging from shared variables
Reporter:Kaloyan Kovachev (knk)Labels:
Date Opened:2009-02-19 03:57:50.000-0600Date Closed:2009-04-08 16:02:10
Priority:MinorRegression?No
Status:Closed/CompleteComponents:Core/General
Versions:Frequency of
Occurrence
Related
Issues:
Environment:Attachments:( 0) asterisk-fix.diff
( 1) asterisk-sharedvars_cleanup-v1.diff
( 2) asterisk-sharedvars_cleanup-v2.diff
( 3) asterisk-sharedvars_cleanup-v3.diff
( 4) asterisk-sharedvars_cleanup-v4.diff
( 5) asterisk-sharedvars_cleanup-v5.diff
( 6) asterisk-sharedvars_cleanup-v6.diff
Description:Additional cleanups to the ones from http://reviewboard.digium.com/r/163 as discussed on dev list
Comments:By: Kaloyan Kovachev (knk) 2009-02-19 04:55:43.000-0600

In trunk AST_FEATURE_AUTOMIXMON was inserted as config flag causing the AST_FEATURE_WARNING_ACTIVE to duplicate AST_FEATURE_NO_H_EXTEN

I will try to completely remove AST_FEATURE_WARNING_ACTIVE too in the next version

but it is also the case for 1.6.0 and 1.6.1 where it should became " AST_FEATURE_WARNING_ACTIVE = (1 << 8),"


By: Kaloyan Kovachev (knk) 2009-02-19 10:06:30.000-0600

The attached fix is for merged revisions 176701 to 1.6.0, 1.6.1 and trunk (from the previous message). It's trivial, but it you prefer will file separate bug.

please ignore/delete the v1 patch - it reintroduces bug 14315 ... working on a new version
By: Jeff Peeler (jpeeler) 2009-02-20 11:17:46.000-0600

The merging problems I quickly sorted out. I'll leave this bug open a while for you to post a new version.
By: Kaloyan Kovachev (knk) 2009-02-20 11:33:35.000-0600

it's almost ready and i have discovered another DTMF/warnings related bug (not caused from merging)

will make some more test and will separate it from this patch (and bugid) for 1.4 - 1.6.1
By: Kaloyan Kovachev (knk) 2009-02-21 11:42:54.000-0600

in addition to the bridging code i have included some unrelated trivial cleanups like "should check that cid_num is not NULL" and spacing for a manager event ... should this be separate patch?
By: Jeff Peeler (jpeeler) 2009-02-23 12:08:35.000-0600

I like where this patch is going, but more is to be done. If after the first warning non feature DTMF is dialed, the warnings stop. If after a feature has executed non feature DTMF is dialed, the warning plays for every digit. Beyond that I didn't look over it too much since this is just for cleanup.
By: Kaloyan Kovachev (knk) 2009-02-24 08:50:56.000-0600

wrong nexteventts calculation, sorry.

I have also added a comment why it should be one warning behind
By: Jeff Peeler (jpeeler) 2009-02-25 15:07:30.000-0600

v3 still has the potential problem of the round error that I addressed in my original fix causing the feature timer to not time out as early as it should.
By: Kaloyan Kovachev (knk) 2009-02-26 04:20:44.000-0600

I have removed it when started and forgot to add it later :(
By: Jeff Peeler (jpeeler) 2009-02-26 13:51:35.000-0600

I think it's ready to go now, but I would like for others to take a look at it. Can you post v4 on reviewboard with your changes described?
By: Kaloyan Kovachev (knk) 2009-02-26 15:19:21.000-0600

One more thing I am not sure of ... in the:
} else if (config->feature_timer) {
res = AST_BRIDGE_RETRY;
block probably there should be explicit clear of the AST_FEATURE_WARNING_ACTIVE flag, but there is a dependancy on backup_feature timer, which i don't know how to triger and test

Edit: it is actually impossible to reach the backup point with a nonzero timer ... introduced probably with revision 4681 ... there is no way that we have backup before it was made ...


By: Kaloyan Kovachev (knk) 2009-02-27 10:53:23.000-0600

After i realized that backup_feature_timer is useless there is much more to remove - posting v5 here and to reviewboard
By: Jeff Peeler (jpeeler) 2009-03-02 14:44:26.000-0600

I noticed while testing your code to see if it does not contain the regression my code has for 14315 that it doesn't work properly as well, but only for the case when warnings are enabled.

Test scenario from 14315:

Pick your favorite 3 phones. Call them A, B and C.

1. A calls B
2. B answers
3. A dials *2# (or whatever the feature code is for attended xfer).
  (B will get MOH; A will get "transfer played)
4. A dials C
5. Before C answers, A hangs up.
By: Kaloyan Kovachev (knk) 2009-03-03 04:14:59.000-0600

If it is with v5 (diff 1 on reviewboard), please see the latest version from reviewboard there was a missing feature_timer reset http://reviewboard.digium.com/r/179/diff/2-3/#index_header which caused the call to be hangup after the transfer is complete. I was waiting for comments there before updating here sorry.

Or is it something else i don't get here? What happens after 5?
By: Jeff Peeler (jpeeler) 2009-03-03 11:29:03.000-0600

Unfortunately, yes. The hangup doesn't occur which is good, but the warnings are lost after the transfer. It's also a problem with the current bridging code as well.
By: Jeff Peeler (jpeeler) 2009-03-03 14:04:19.000-0600

KNK: I misunderstood what should be happening here. The bridge config shouldn't be copied after a masquerade, so there is no problem here. Sorry for the noise.
By: Kaloyan Kovachev (knk) 2009-03-04 02:18:10.000-0600

Well i have tested with L() and the new call was also getting its own warnings at the right time. Right they should not be the ones from the first bridge.
i think there is nothing left that i have missed, so this is ready to go ... the changes are really quite a lot, but i still think its worth backporting it
By: Kaloyan Kovachev (knk) 2009-03-04 02:30:21.000-0600

And here is the latest version v6 against latest trunk
By: Digium Subversion (svnbot) 2009-04-08 16:00:40

Repository: asterisk
Revision: 187211

U   trunk/include/asterisk/channel.h
U   trunk/main/channel.c
U   trunk/main/features.c

------------------------------------------------------------------------
r187211 | jpeeler | 2009-04-08 16:00:39 -0500 (Wed, 08 Apr 2009) | 20 lines

Add timer for features so that backup bridge config can go away

The biggest change done here was elimination of the backup_config for use with
features. Previously, the bridging code upon detecting a feature would set the
start time of the bridge to the start time of the feature. Then after the
feature had either expired or timed out the start time would be reset to the
true bridge start time from the backup_config. Now, the time differences are
calculated with respect to the newly added feature_start_time timeval instead.

There should be no behavior changes from the previous functionality aside from
the bridge timing being unaffected by either valid or partial feature matches.
Previously the timing would be increased by the length of time configured for
featuredigittimeout, which was probably never noticed.

(closes issue ASTERISK-13605)
Reported by: KNK
Tested by: jpeeler

Review: http://reviewboard.digium.com/r/179/

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

http://svn.digium.com/view/asterisk?view=rev&revision=187211
By: Digium Subversion (svnbot) 2009-04-08 16:01:33

Repository: asterisk
Revision: 187212

_U  branches/1.6.0/

------------------------------------------------------------------------
r187212 | jpeeler | 2009-04-08 16:01:33 -0500 (Wed, 08 Apr 2009) | 26 lines

Blocked revisions 187211 via svnmerge

........
 r187211 | jpeeler | 2009-04-08 16:00:39 -0500 (Wed, 08 Apr 2009) | 20 lines
 
 Add timer for features so that backup bridge config can go away
 
 The biggest change done here was elimination of the backup_config for use with
 features. Previously, the bridging code upon detecting a feature would set the
 start time of the bridge to the start time of the feature. Then after the
 feature had either expired or timed out the start time would be reset to the
 true bridge start time from the backup_config. Now, the time differences are
 calculated with respect to the newly added feature_start_time timeval instead.
 
 There should be no behavior changes from the previous functionality aside from
 the bridge timing being unaffected by either valid or partial feature matches.
 Previously the timing would be increased by the length of time configured for
 featuredigittimeout, which was probably never noticed.
 
 (closes issue ASTERISK-13605)
 Reported by: KNK
 Tested by: jpeeler
 
 Review: http://reviewboard.digium.com/r/179/
........

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

http://svn.digium.com/view/asterisk?view=rev&revision=187212
By: Digium Subversion (svnbot) 2009-04-08 16:01:52

Repository: asterisk
Revision: 187213

_U  branches/1.6.1/

------------------------------------------------------------------------
r187213 | jpeeler | 2009-04-08 16:01:52 -0500 (Wed, 08 Apr 2009) | 26 lines

Blocked revisions 187211 via svnmerge

........
 r187211 | jpeeler | 2009-04-08 16:00:39 -0500 (Wed, 08 Apr 2009) | 20 lines
 
 Add timer for features so that backup bridge config can go away
 
 The biggest change done here was elimination of the backup_config for use with
 features. Previously, the bridging code upon detecting a feature would set the
 start time of the bridge to the start time of the feature. Then after the
 feature had either expired or timed out the start time would be reset to the
 true bridge start time from the backup_config. Now, the time differences are
 calculated with respect to the newly added feature_start_time timeval instead.
 
 There should be no behavior changes from the previous functionality aside from
 the bridge timing being unaffected by either valid or partial feature matches.
 Previously the timing would be increased by the length of time configured for
 featuredigittimeout, which was probably never noticed.
 
 (closes issue ASTERISK-13605)
 Reported by: KNK
 Tested by: jpeeler
 
 Review: http://reviewboard.digium.com/r/179/
........

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

http://svn.digium.com/view/asterisk?view=rev&revision=187213
By: Digium Subversion (svnbot) 2009-04-08 16:02:09

Repository: asterisk
Revision: 187214

_U  branches/1.6.2/

------------------------------------------------------------------------
r187214 | jpeeler | 2009-04-08 16:02:09 -0500 (Wed, 08 Apr 2009) | 26 lines

Blocked revisions 187211 via svnmerge

........
 r187211 | jpeeler | 2009-04-08 16:00:39 -0500 (Wed, 08 Apr 2009) | 20 lines
 
 Add timer for features so that backup bridge config can go away
 
 The biggest change done here was elimination of the backup_config for use with
 features. Previously, the bridging code upon detecting a feature would set the
 start time of the bridge to the start time of the feature. Then after the
 feature had either expired or timed out the start time would be reset to the
 true bridge start time from the backup_config. Now, the time differences are
 calculated with respect to the newly added feature_start_time timeval instead.
 
 There should be no behavior changes from the previous functionality aside from
 the bridge timing being unaffected by either valid or partial feature matches.
 Previously the timing would be increased by the length of time configured for
 featuredigittimeout, which was probably never noticed.
 
 (closes issue ASTERISK-13605)
 Reported by: KNK
 Tested by: jpeeler
 
 Review: http://reviewboard.digium.com/r/179/
........

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

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