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

jrose reviewboard at asterisk.org
Thu Feb 14 14:03:03 CST 2013 


> On Feb. 14, 2013, 12:45 p.m., rmudgett wrote:
> > /team/group/bridge_construction/apps/app_dial.c, line 265
> > <https://reviewboard.asterisk.org/r/2328/diff/3/?file=33440#file33440line265>
> >
> >     This has been documented wrong for quite awhile too.
> >     
> >     Just delete the "If not set..." sentence.
> >     
> >     The documentation changes in app_dial should be fixed in v1.8+

Alright, I'm removing the documentation changes in the review.  I can put them in a separate review later.


> On Feb. 14, 2013, 12:45 p.m., rmudgett wrote:
> > /team/group/bridge_construction/main/bridging.c, line 2048
> > <https://reviewboard.asterisk.org/r/2328/diff/3/?file=33443#file33443line2048>
> >
> >     For completeness, the constructor should memset the struct to zero.  No sense in leaving any member uninitialized to a known value even if it will be overwritten soon anyway.

sure thing


> On Feb. 14, 2013, 12:45 p.m., rmudgett wrote:
> > /team/group/bridge_construction/main/features.c, line 4654
> > <https://reviewboard.asterisk.org/r/2328/diff/3/?file=33444#file33444line4654>
> >
> >     New pig call: "sow e"

> so we
fixed


> On Feb. 14, 2013, 12:45 p.m., rmudgett wrote:
> > /team/group/bridge_construction/bridges/bridge_builtin_interval_features.c, lines 107-110
> > <https://reviewboard.asterisk.org/r/2328/diff/3/?file=33441#file33441line107>
> >
> >     This function could be restructured to make the highlighted code here in a common path by changing the sense of the "if (remaining > 0)" test and returning early.

check


> On Feb. 14, 2013, 12:45 p.m., rmudgett wrote:
> > /team/group/bridge_construction/bridges/bridge_builtin_interval_features.c, line 138
> > <https://reviewboard.asterisk.org/r/2328/diff/3/?file=33441#file33441line138>
> >
> >     Extra parens here.

check


> On Feb. 14, 2013, 12:45 p.m., rmudgett wrote:
> > /team/group/bridge_construction/bridges/bridge_builtin_interval_features.c, line 126
> > <https://reviewboard.asterisk.org/r/2328/diff/3/?file=33441#file33441line126>
> >
> >     Extra parens here.

check


> On Feb. 14, 2013, 12:45 p.m., rmudgett wrote:
> > /team/group/bridge_construction/bridges/bridge_builtin_interval_features.c, line 80
> > <https://reviewboard.asterisk.org/r/2328/diff/3/?file=33441#file33441line80>
> >
> >     Stray blank line.

check


- jrose


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


On Feb. 14, 2013, 2:02 p.m., jrose wrote:
> 
> -----------------------------------------------------------
> 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.
> 
> 
> 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
> -----
> 
>   /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/58c9729a/attachment-0001.htm>

More information about the asterisk-dev mailing list