[asterisk-dev] [Code Review] 2654: Make built-in attended transfer feature complete

rmudgett reviewboard at asterisk.org
Wed Jul 10 15:24:37 CDT 2013 

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



/trunk/include/asterisk/bridging_features.h
<https://reviewboard.asterisk.org/r/2654/#comment17962>

    Use the bit shift definition rather than letting an enum pick the next value.  (1 << 1).



/trunk/include/asterisk/bridging_features.h
<https://reviewboard.asterisk.org/r/2654/#comment17963>

    Remove.  This is not used.



/trunk/include/asterisk/bridging_internal.h
<https://reviewboard.asterisk.org/r/2654/#comment17964>

    Probably should keep the \internal tag for these functions.



/trunk/main/bridging.c
<https://reviewboard.asterisk.org/r/2654/#comment17965>

    Can remove the doxygen for the function here since the prototype is in bridging_internal.h.



/trunk/main/bridging.c
<https://reviewboard.asterisk.org/r/2654/#comment17966>

    Same here with doxygen comment.



/trunk/main/bridging_basic.c
<https://reviewboard.asterisk.org/r/2654/#comment17979>

    Always use isolation parentheses on multi token macros unless there is a compelling reason not to use them.



/trunk/main/bridging_basic.c
<https://reviewboard.asterisk.org/r/2654/#comment17971>

    This is set but not used.  It is just set to the personality v_table name and then not used.



/trunk/main/bridging_basic.c
<https://reviewboard.asterisk.org/r/2654/#comment17967>

    The assert here is not needed.  In fact it should not be here because a bridge creation failure may not have created the personality yet.



/trunk/main/bridging_basic.c
<https://reviewboard.asterisk.org/r/2654/#comment17975>

    Is there a reason this lock is used with the cond and not the ao2 lock?
    
    ao2_object_get_lockaddr() is available so the ao2 lock can be used with cond.



/trunk/main/bridging_basic.c
<https://reviewboard.asterisk.org/r/2654/#comment17977>

    It may be worth using string fields for these.  MAX_PATH is 4k so this struct is quite large.



/trunk/main/bridging_basic.c
<https://reviewboard.asterisk.org/r/2654/#comment17976>

    ast_channel_cleanup() is now available to use with channels.



/trunk/main/bridging_basic.c
<https://reviewboard.asterisk.org/r/2654/#comment17978>

    The values given to the enum names seem odd.
    
    Is TRANSFER_STATE_FLAG_TIMED a bit field mask?  It seems to be from the way it is used.  Maybe it should be defined in terms of the other state timer flags:
    TRANSFER_STATE_FLAG_TIMED = TRANSFER_STATE_FLAG_TIMER_RESET | TRANSFER_STATE_FLAG_TIMER_LOOP_DELAY | TRANSFER_STATE_FLAG_ATXFER_NO_ANSWER
    



/trunk/main/bridging_basic.c
<https://reviewboard.asterisk.org/r/2654/#comment17980>

    Please separate this variable declaration from the struct definition.
    
    There should not be a space between state_properties and the square brackets.
    
    It could be declared const.



/trunk/main/bridging_basic.c
<https://reviewboard.asterisk.org/r/2654/#comment17970>

    You are not handling personality allocation failure here.
    
    You may want to rethink how you do personality changes.  Maybe preallocating the personalities on bridge creation so changing personalities does not risk a critical memory allocation failure.



/trunk/main/bridging_basic.c
<https://reviewboard.asterisk.org/r/2654/#comment17968>

    You are not handling failure of atxfer personality here.  A crash is likely if this happens.



/trunk/main/bridging_basic.c
<https://reviewboard.asterisk.org/r/2654/#comment17969>

    You are not causing the bridge creation to fail if the personality fails to be created.



/trunk/main/bridging_basic.c
<https://reviewboard.asterisk.org/r/2654/#comment17973>

    These DTMF hooks can no longer be attached to any bridge type like the others.  The hooks should check that they are being executed on a basic bridge.



/trunk/main/features_config.c
<https://reviewboard.asterisk.org/r/2654/#comment17972>

    Instead of using the word change, how about using the word toggle.



/trunk/main/stasis_bridging.c
<https://reviewboard.asterisk.org/r/2654/#comment17974>

    Missing a break here?


- rmudgett


On July 10, 2013, 6:20 p.m., Mark Michelson wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/2654/
> -----------------------------------------------------------
> 
> (Updated July 10, 2013, 6:20 p.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Bugs: ASTERISK-21543
>     https://issues.asterisk.org/jira/browse/ASTERISK-21543
> 
> 
> Repository: Asterisk
> 
> 
> Description
> -------
> 
> This review contains the content necessary to get DTMF attended transfer feature complete.
> 
> In order to facilitate this, there are some background tasks that needed to be done first. First, when an attended transfer is initiated, it was clear that the bridge that the parties were in would need to behave differently from how it normally did. In order to allow for this to happen, the concept of bridge "personalities" was added. When a bridge is being used in an attended transfer, it has its personality changed to be that of an attended transfer basic bridge instead of a normal basic bridge. With this personality, the following changes are present:
> 
> * The bridge will never dissolve on its own. It must be destroyed by an outside influence.
> * The bridge recognizes when a channel with the "transferer" role enters. This channel is bestowed with DTMF hooks and a hangup hook that other channels do not get.
> * The normal DTMF hooks are not present for channels. This makes compounding transfers impossible.
> 
> In order to make use of an alternate personality for basic bridging, attended transfer functionality was moved from bridges/bridge_builtin_features.c to main/bridging_basic.c. While performing this move, it became clear that it made sense to move blind transfer functionality as well since attended and blind transfers share some common code.
> 
> Attended transfer operation has been completely overhauled from its old form. In its old form, the attended transfer DTMF hook would not return until a conclusive result of the transfer could be determined. With this set of changes, the attended transfer DTMF hook sets up an attended_transfer_properties structure that has all the relevant pieces of information for the attended transfer. After setting up the properties, the feature hook launches a monitoring thread and returns.
> 
> This monitor thread then runs a state machine that responds to stimuli from various hooks and callbacks. The state machine allows for orderly operation since stimuli may be coming from multiple other threads.
> 
> In addition to the normal feedback for reviews (i.e. memory leaks, potential deadlocks, etc.), let me know if you have alternate suggestions for names of things. I struggled to come up with cogent state names for the state machine, so if you have better suggestions for the names, please let me know.
> 
> 
> Diffs
> -----
> 
>   /trunk/include/asterisk/stasis_bridging.h 393998 
>   /trunk/include/asterisk/features_config.h 393998 
>   /trunk/include/asterisk/bridging_roles.h 393998 
>   /trunk/include/asterisk/bridging_internal.h PRE-CREATION 
>   /trunk/include/asterisk/bridging_features.h 393998 
>   /trunk/include/asterisk/bridging.h 393998 
>   /trunk/bridges/bridge_builtin_features.c 393998 
>   /trunk/main/bridging.c 393998 
>   /trunk/main/bridging_basic.c 393998 
>   /trunk/main/bridging_roles.c 393998 
>   /trunk/main/features.c 393998 
>   /trunk/main/features_config.c 393998 
>   /trunk/main/stasis_bridging.c 393998 
> 
> Diff: https://reviewboard.asterisk.org/r/2654/diff/
> 
> 
> Testing
> -------
> 
> Using the state machine as a guide, I ran through every transition that can be reliably tested (for instance, I couldn't easily test transfer failures that rely on memory allocation failures). In all cases, I got the result I expected.
> 
> 
> Thanks,
> 
> Mark Michelson
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20130710/f7499c77/attachment-0001.htm>

More information about the asterisk-dev mailing list