[asterisk-dev] [Code Review] bridging: Interval hooks continued

[jrose]

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

(Updated Feb. 14, 2013, 2:02 p.m.)


Review request for Asterisk Developers, Mark Michelson, rmudgett, and Matt Jordan.


Changes
-------

Address the above


Summary
-------

Richard had a shopping list of improvements to be made.  Here is the copypasta.

1) Support for ${LIMIT_CONNECT_FILE} is missing.  This file is played at
the beginning of the bridge.  This can be implemented as a one time
interval hook with an immediate timeout that fires as soon as the channel
joins the bridge.  

* Addressed as described.  It's worth noting that contrary to documentation,
normal Asterisk doesn't actually say the remaining time at the beginning of the
conference if no LIMIT_CONNECT_FILE is specified. This patch changes things to
match with the documentation.

2) The interval_hook_time_cmp() needs to compare a sequence number of the
order of hooks added in addition to the interval_trip_time.  This will
ensure that when timeouts happen at the same time, the oldest interval
expiring at that time will execute first.

* Implemented as suggested. 

3) Need to add the test events associated with interval hooks.  Notably
the end bridge timeout event.  There may be other timeout events that I am
not aware of.

* Added to the limits duration callback. The other relevant test event was
for the S option and for L with only the duration argument, and they are
currently executed through a different mechanism that is unaffected by this
work. That might be changed soon.

4) ast_bridge_features_set_limits() must not save a pointer to the limits
struct in the hook.  In this case the peer channel will likely have a
stale pointer when ast_bridge_call() is changed to not expect to get the
peer back.  Also transfers could move the peer channel to another bridge.
ast_bridge_features_set_limits() should copy the limits information into
its own structure so it is independent.

* ast_bridge_features_set_limits now copies the contents of the limits
struct into a freshly allocated struct and applies it to the bridge channel's
features struct. Only one can be applied to a bridge channel at a time right
now and I'm unsure if that should be changed or if a mechanism for overwriting
it should be provided.

5) I think it is a good feature of the interval hooks to know about the
"timeleft" macro sound filename.  This allows the LIMIT_CONNECT_FILE and
LIMIT_WARNING_FILE to be set to "timeleft" and get the time left output.

* timeleft macro sound name is now read by the hook rather than reinterpreted
at the consumer level.

6) I don't particularly like the fact that struct
ast_bridge_features_limits has fixed sized sound filenames.

* Changed to use string fields. It might still be limited from a practical
standpoint by the dial application and/or features.c.

7) I don't think that ast_bridge_features_interval_hook() needs to have a
strict flag.  It should always do that if it is a repeating interval.

* Strict flag removed and all intervals are now treated strictly.


This addresses bug ASTERISK-20872.
    https://issues.asterisk.org/jira/browse/ASTERISK-20872


Diffs (updated)
-----

  /team/group/bridge_construction/apps/app_dial.c 381469 
  /team/group/bridge_construction/include/asterisk/bridging.h 381469 
  /team/group/bridge_construction/include/asterisk/bridging_features.h 381469 
  /team/group/bridge_construction/main/bridging.c 381469 
  /team/group/bridge_construction/main/features.c 381469 
  /team/jrose/bridge_projects/bridges/bridge_builtin_interval_features.c 381469 

Diff: https://reviewboard.asterisk.org/r/2328/diff


Testing
-------

Every single scenario I've been testing all along, which covers everything in the bridge test as well as some extra stuff involving setting of specific sound file variables.


Thanks,

jrose

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20130214/4225296d/attachment.htm>