[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/#review7865
-----------------------------------------------------------



/team/group/bridge_construction/apps/app_dial.c
<https://reviewboard.asterisk.org/r/2328/#comment14927>

    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+



/team/group/bridge_construction/apps/app_dial.c
<https://reviewboard.asterisk.org/r/2328/#comment14928>

    ...a warning when y milliseconds before x is reached.



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

    Looks like we really need to make ast_bridge_change_state_nolock() a semi-private API call with the prototype put into bridging.h.



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

    Stray blank line.



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

    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.



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

    Extra parens here.



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

    Extra parens here.



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

    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.



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

    New pig call: "sow e"


- rmudgett


On Feb. 14, 2013, 9:29 a.m., jrose wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/2328/
> -----------------------------------------------------------
> 
> (Updated Feb. 14, 2013, 9:29 a.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/apps/app_dial.c 381343 
>   /team/group/bridge_construction/include/asterisk/bridging_features.h 381343 
>   /team/group/bridge_construction/main/bridging.c 381343 
>   /team/group/bridge_construction/main/features.c 381343 
> 
> 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/7b786435/attachment-0001.htm>