[asterisk-dev] [Code Review] Merge Called party identification changes into trunk.

[rmudgett at digium.com]

> On 2009-03-18 16:24:42, Russell Bryant wrote:
> > /trunk/main/features.c, line 1587
> > <http://reviewboard.digium.com/r/201/diff/3/?file=3254#file3254line1587>
> >
> >     The channel needs to be locked here
> 
>  wrote:
>     Fixed.

Mark, please note that ast_party_connected_line_collect_caller() is a SHALLOW copy so the strings point to the original memory.  This function simply collects the scattered data from struct ast_callerid into the new struct ast_party_connectedline so it can be operated upon.  Any locks must be maintained because you are still dealing with the strings in the original location.


> On 2009-03-18 16:24:42, Russell Bryant wrote:
> > /trunk/main/features.c, lines 1692-1695
> > <http://reviewboard.digium.com/r/201/diff/3/?file=3254#file3254line1692>
> >
> >     missing channel locking here
> 
>  wrote:
>     Fixed.

Same comment about the SHALLOW copy and locks here.


> On 2009-03-18 16:24:42, Russell Bryant wrote:
> > /trunk/main/dial.c, lines 176-183
> > <http://reviewboard.digium.com/r/201/diff/3/?file=3253#file3253line176>
> >
> >     Didn't this macro exist in another file, too?  Should we just put it in a header file?
> >     
> >     Also, this is another change unrelated to this patch ...
> 
>  wrote:
>     Yeah, apparently this macro is in both apps/app_dial.c and main/dial.c. Moving this to strings.h would make sense to me, since I think this same construct is used in many places. For now, I've reverted the change from free to ast_free.

The code of this macro is manually inlined in other files.  Those other files would definitely benefit if it were in a common header file.


- rmudgett


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviewboard.digium.com/r/201/#review570
-----------------------------------------------------------


On 2009-03-18 15:10:15, Mark Michelson wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.digium.com/r/201/
> -----------------------------------------------------------
> 
> (Updated 2009-03-18 15:10:15)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Summary
> -------
> 
> This diff encompasses the changes between the issue8824 branch and Asterisk trunk. These changes include all the changes necessary to allow for the transmission and reception of remote called party identification (COLP/CONP).
> 
> 
> This addresses bug 8824.
>     http://bugs.digium.com/view.php?id=8824
> 
> 
> Diffs
> -----
> 
>   /trunk/CHANGES 182956 
>   /trunk/apps/app_dial.c 182956 
>   /trunk/apps/app_directed_pickup.c 182956 
>   /trunk/apps/app_queue.c 182956 
>   /trunk/channels/chan_agent.c 182956 
>   /trunk/channels/chan_dahdi.c 182956 
>   /trunk/channels/chan_h323.c 182956 
>   /trunk/channels/chan_iax2.c 182956 
>   /trunk/channels/chan_local.c 182956 
>   /trunk/channels/chan_mgcp.c 182956 
>   /trunk/channels/chan_misdn.c 182956 
>   /trunk/channels/chan_phone.c 182956 
>   /trunk/channels/chan_sip.c 182956 
>   /trunk/channels/chan_skinny.c 182956 
>   /trunk/channels/chan_unistim.c 182956 
>   /trunk/channels/misdn/chan_misdn_config.h 182956 
>   /trunk/channels/misdn/isdn_lib.h 182956 
>   /trunk/channels/misdn/isdn_lib.c 182956 
>   /trunk/channels/misdn/isdn_lib_intern.h 182956 
>   /trunk/channels/misdn/isdn_msg_parser.c 182956 
>   /trunk/channels/misdn_config.c 182956 
>   /trunk/configs/misdn.conf.sample 182956 
>   /trunk/configs/sip.conf.sample 182956 
>   /trunk/funcs/func_connectedline.c PRE-CREATION 
>   /trunk/funcs/func_redirecting.c PRE-CREATION 
>   /trunk/include/asterisk/callerid.h 182956 
>   /trunk/include/asterisk/channel.h 182956 
>   /trunk/include/asterisk/frame.h 182956 
>   /trunk/main/callerid.c 182956 
>   /trunk/main/channel.c 182956 
>   /trunk/main/dial.c 182956 
>   /trunk/main/features.c 182956 
>   /trunk/main/rtp.c 182956 
> 
> Diff: http://reviewboard.digium.com/r/201/diff
> 
> 
> Testing
> -------
> 
> Digium's Product Quality department has extensively tested a remarkably similar version of these enhancements. In addition, we know of a customer who has been using this branch for some months now in a production environment. I would also be willing to bet that some of those who have been monitoring issue 8824 have also done some testing as well.
> 
> 
> Thanks,
> 
> Mark
> 
>