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

[rmudgett]

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



/team/group/bridge_construction/bridges/bridge_builtin_interval_features.c
<https://reviewboard.asterisk.org/r/2328/#comment14951>

    Put the unlock at the end of the switch.  It does not need to protect the notify call.



/team/group/bridge_construction/bridges/bridge_builtin_interval_features.c
<https://reviewboard.asterisk.org/r/2328/#comment14952>

    Stray blank crept in.  This code is doing one thing:  Sending the requested audio out.



/team/group/bridge_construction/bridges/bridge_builtin_interval_features.c
<https://reviewboard.asterisk.org/r/2328/#comment14949>

    This is now common code in the then and else block.  That is why I suggested the restructuring.  Move it to after the else block.



/team/group/bridge_construction/main/bridging.c
<https://reviewboard.asterisk.org/r/2328/#comment14950>

    Wrong size.  sizeof(*limits)


- rmudgett


On Feb. 14, 2013, 2:14 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:14 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/bridges/bridge_builtin_interval_features.c PRE-CREATION 
>   /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 
> 
> 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/673391b3/attachment-0001.htm>