[asterisk-dev] [Code Review] Enhancements to connected line and redirecting work.
rmudgett at digium.com
rmudgett at digium.com
Wed May 12 17:04:58 CDT 2010
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/652/#review2005
-----------------------------------------------------------
/trunk/apps/app_dial.c
<https://reviewboard.asterisk.org/r/652/#comment4247>
I was going to comment that the second ast_channel_update_redirecting() needed the c lock. However, I think it just needs to be deleted.
/trunk/apps/app_dial.c
<https://reviewboard.asterisk.org/r/652/#comment4238>
Red spot
/trunk/apps/app_dial.c
<https://reviewboard.asterisk.org/r/652/#comment4248>
This cast should not be necessary.
/trunk/channels/chan_dahdi.c
<https://reviewboard.asterisk.org/r/652/#comment4249>
Change the comment to:
/*!
* \brief Caller ID tag from incoming call
* \note The "cid_tag" string read in from chan_dahdi.conf
*/
/trunk/channels/chan_dahdi.c
<https://reviewboard.asterisk.org/r/652/#comment4237>
The cid_tag copy is common to the if and else branches of the test and can be put at the end of the if statement.
/trunk/channels/chan_misdn.c
<https://reviewboard.asterisk.org/r/652/#comment4250>
The incoming_cid_tag[] local variable is not needed. You can call misdn_cfg_get() on bc->incoming_cid_tag directly:
misdn_cfg_get(port, ..._CALLERID_TAG, bc->incoming_cid_tag, sizeof(bc->incoming_cid_tag));
if (!ast_strlen_zero(bc->incoming_cid_tag)) {
chan_misdn_log(1, port, ....);
}
/trunk/channels/chan_misdn.c
<https://reviewboard.asterisk.org/r/652/#comment4251>
Update function comment for cid_tag param.
/trunk/channels/chan_misdn.c
<https://reviewboard.asterisk.org/r/652/#comment4252>
Update function comment for tag param.
/trunk/channels/chan_misdn.c
<https://reviewboard.asterisk.org/r/652/#comment4254>
Change sizeof to sizeof(append_msn)
/trunk/channels/chan_misdn.c
<https://reviewboard.asterisk.org/r/652/#comment4239>
Red spot
/trunk/channels/chan_misdn.c
<https://reviewboard.asterisk.org/r/652/#comment4253>
Change sizeof to sizeof(append_msn).
/trunk/channels/chan_sip.c
<https://reviewboard.asterisk.org/r/652/#comment4240>
Red spot
/trunk/channels/misdn/isdn_lib.h
<https://reviewboard.asterisk.org/r/652/#comment4257>
Remove trailing comment: TAR addon.
/trunk/channels/misdn_config.c
<https://reviewboard.asterisk.org/r/652/#comment4255>
An appropriate location in misdn.conf.sample needs to be added to document these new configuration parameters.
/trunk/channels/misdn_config.c
<https://reviewboard.asterisk.org/r/652/#comment4256>
What is going on with this sentence?
"incoming resp. outgoing msn"
Incoming calls have the dialed number appended to the tag.
Outgoing calls have the caller number appended to the tag.
/trunk/funcs/func_redirecting.c
<https://reviewboard.asterisk.org/r/652/#comment4258>
Memory leak! REDIRECTING(to/from-all) can leak if the ast_strdup(num) fails in redirecting_id_write(). ast_party_redirecting_free() should be called unconditionally after these switches instead of only in the ID_FIELD_VALID case.
/trunk/include/asterisk/channel.h
<https://reviewboard.asterisk.org/r/652/#comment4241>
Red spot
/trunk/include/asterisk/channel.h
<https://reviewboard.asterisk.org/r/652/#comment4259>
This field should be grouped with the name and number fields for padding reasons.
/trunk/include/asterisk/channel.h
<https://reviewboard.asterisk.org/r/652/#comment4260>
Delete this line. It is an artifact from the merge and function renaming done on trunk.
/trunk/include/asterisk/channel.h
<https://reviewboard.asterisk.org/r/652/#comment4242>
More red spots
/trunk/include/asterisk/frame.h
<https://reviewboard.asterisk.org/r/652/#comment4261>
An extra blank line showed up here.
/trunk/main/channel.c
<https://reviewboard.asterisk.org/r/652/#comment4243>
Red spot
/trunk/main/channel.c
<https://reviewboard.asterisk.org/r/652/#comment4244>
Red spot
/trunk/main/channel.c
<https://reviewboard.asterisk.org/r/652/#comment4245>
This could be made like the ast_channel_redirecting_macro(). Then you would not need the awkward union or to test is_frame twice.
/trunk/main/rtp_engine.c
<https://reviewboard.asterisk.org/r/652/#comment4262>
Control will never reach these lines. I think these lines are in the wrong place.
/trunk/main/rtp_engine.c
<https://reviewboard.asterisk.org/r/652/#comment4246>
Red spot
- rmudgett
On 2010-05-07 12:27:29, Mark Michelson wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/652/
> -----------------------------------------------------------
>
> (Updated 2010-05-07 12:27:29)
>
>
> Review request for Asterisk Developers.
>
>
> Summary
> -------
>
> Digium has a commercial customer who has made extensive use of the connected party and
> redirecting information present in later versions of Asterisk Business Edition and which
> is to be in the upcoming 1.8 release. Through their use of the feature, new problems and solutions
> have come about. This patch adds several enhancements to maximize usage of the connected party
> and redirecting information functionality.
>
> First, Asterisk trunk already had connected line interception macros. These macros allow you to
> manipulate connected line information before it was sent out to its target. This patch adds the
> same feature except for redirecting information instead.
>
> Second, the ast_callerid and ast_party_id structures have been enhanced to provide a "tag." This
> tag can be set with func_callerid, func_connectedline, func_redirecting, and in the case of DAHDI,
> mISDN, and SIP channels, can be set in a configuration file. The idea behind the callerid tag is
> that it can be set to whatever value the administrator likes. Later, when running connected line
> and redirecting macros, the admin can read the tag off the appropriate structure to determine what
> action to take. You can think of this sort of like a channel variable, except that instead of having
> the variable associated with a channel, the variable is associated with a specific identity within
> Asterisk.
>
> Third, app_dial has two new options, s and u. The s option lets a dialplan writer force a specific
> caller ID tag to be placed on the outgoing channel. The u option allows the dialplan writer to force
> a specific calling presentation value on the outgoing channel.
>
> Fourth, there is a new control frame subclass called AST_CONTROL_READ_ACTION added. This was added
> to correct a very specific situation. In the case of SIP semi-attended (blond) transfers, the party
> being transferred would not have the opportunity to run a connected line interception macro to
> possibly alter the transfer target's connected line information. The issue here was that during a
> blond transfer, the SIP transfer code has no bridged channel on which to queue the connected line
> update. The way this was corrected was to add this new control frame subclass. Now, we queue an
> AST_CONTROL_READ_ACTION frame on the channel on which the connected line interception macro should
> be run. When ast_read is called to read the frame, ast_read responds by calling a callback function
> associated with the specific read action the control frame describes. In this case, the action taken
> is to run the connected line interception macro on the transferee's channel.
>
>
> Diffs
> -----
>
> /trunk/apps/app_dial.c 261563
> /trunk/apps/app_queue.c 261563
> /trunk/channels/chan_dahdi.c 261563
> /trunk/channels/chan_local.c 261563
> /trunk/channels/chan_misdn.c 261563
> /trunk/channels/chan_sip.c 261563
> /trunk/channels/misdn/chan_misdn_config.h 261563
> /trunk/channels/misdn/isdn_lib.h 261563
> /trunk/channels/misdn_config.c 261563
> /trunk/channels/sip/include/sip.h 261563
> /trunk/funcs/func_callerid.c 261563
> /trunk/funcs/func_connectedline.c 261563
> /trunk/funcs/func_redirecting.c 261563
> /trunk/include/asterisk/channel.h 261563
> /trunk/include/asterisk/frame.h 261563
> /trunk/main/channel.c 261563
> /trunk/main/dial.c 261563
> /trunk/main/features.c 261563
> /trunk/main/rtp_engine.c 261563
>
> Diff: https://reviewboard.asterisk.org/r/652/diff
>
>
> Testing
> -------
>
> The Digium customer mentioned before has done extensive testing of connected line and redirecting
> scenarios, as has Digium's Product Qualification department.
>
>
> Thanks,
>
> Mark
>
>
More information about the asterisk-dev
mailing list