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

[Mark Michelson]

> On 2009-03-23 11:34:50, Russell Bryant wrote:
> > /trunk/apps/app_dial.c, lines 120-123
> > <http://reviewboard.digium.com/r/201/diff/6/?file=3507#file3507line120>
> >
> >     This appears to be an unrelated documentation change.

yeah...I have no idea where this came from. Removed.


> On 2009-03-23 11:34:50, Russell Bryant wrote:
> > /trunk/apps/app_dial.c, lines 640-646
> > <http://reviewboard.digium.com/r/201/diff/6/?file=3507#file3507line640>
> >
> >     still have unrelated formatting changes here.

I have now copied-and-pasted the block from trunk, so these differences should be cleared up now.


> On 2009-03-23 11:34:50, Russell Bryant wrote:
> > /trunk/apps/app_dial.c, lines 776-792
> > <http://reviewboard.digium.com/r/201/diff/6/?file=3507#file3507line776>
> >
> >     This section needs some channel locking love

Fixed.


> On 2009-03-23 11:34:50, Russell Bryant wrote:
> > /trunk/apps/app_dial.c, lines 853-855
> > <http://reviewboard.digium.com/r/201/diff/6/?file=3507#file3507line853>
> >
> >     This section needs some channel locking love

Fixed.


> On 2009-03-23 11:34:50, Russell Bryant wrote:
> > /trunk/apps/app_dial.c, lines 907-909
> > <http://reviewboard.digium.com/r/201/diff/6/?file=3507#file3507line907>
> >
> >     Need some channel locking here

Fixed.


> On 2009-03-23 11:34:50, Russell Bryant wrote:
> > /trunk/apps/app_dial.c, lines 954-956
> > <http://reviewboard.digium.com/r/201/diff/6/?file=3507#file3507line954>
> >
> >     channel locking

Fixed. (ast_party_connected_line_collect_caller isn't as useful as I once had thought)


> On 2009-03-23 11:34:50, Russell Bryant wrote:
> > /trunk/apps/app_dial.c, lines 1761-1763
> > <http://reviewboard.digium.com/r/201/diff/6/?file=3507#file3507line1761>
> >
> >     Both channels must be locked during this operation.

tmp is not a channel. It is a struct chanlist *, which is local to this instance of app_dial and therefore does not need a lock.


> On 2009-03-23 11:34:50, Russell Bryant wrote:
> > /trunk/apps/app_dial.c, lines 1848-1857
> > <http://reviewboard.digium.com/r/201/diff/6/?file=3507#file3507line1848>
> >
> >     This is another tricky section that has some missing channel locking.
> >     
> >     ast_set_callerid() will lock the channel provided as the first argument.  However, if you're going to read fields directly from another channel, that channel must be locked, as well.

Fixed. Really, both channels should be locked even in current versions of Asterisk due to the constant accesses of the channels' data.


> On 2009-03-23 11:34:50, Russell Bryant wrote:
> > /trunk/apps/app_dial.c, lines 1859-1864
> > <http://reviewboard.digium.com/r/201/diff/6/?file=3507#file3507line1859>
> >
> >     channel locking

I took care of this with the same fix as above.


- Mark


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


On 2009-03-23 09:52:15, Mark Michelson wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.digium.com/r/201/
> -----------------------------------------------------------
> 
> (Updated 2009-03-23 09:52: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/apps/app_dial.c 183691 
>   /trunk/apps/app_directed_pickup.c 183691 
>   /trunk/apps/app_queue.c 183691 
>   /trunk/channels/chan_agent.c 183691 
>   /trunk/channels/chan_dahdi.c 183691 
>   /trunk/channels/chan_h323.c 183691 
>   /trunk/channels/chan_iax2.c 183691 
>   /trunk/channels/chan_local.c 183691 
>   /trunk/channels/chan_mgcp.c 183691 
>   /trunk/channels/chan_misdn.c 183691 
>   /trunk/channels/chan_phone.c 183691 
>   /trunk/channels/chan_sip.c 183691 
>   /trunk/channels/chan_skinny.c 183691 
>   /trunk/channels/chan_unistim.c 183691 
>   /trunk/channels/misdn/chan_misdn_config.h 183691 
>   /trunk/channels/misdn/isdn_lib.h 183691 
>   /trunk/channels/misdn/isdn_lib.c 183691 
>   /trunk/channels/misdn/isdn_lib_intern.h 183691 
>   /trunk/channels/misdn/isdn_msg_parser.c 183691 
>   /trunk/channels/misdn_config.c 183691 
>   /trunk/configs/misdn.conf.sample 183691 
>   /trunk/configs/sip.conf.sample 183691 
>   /trunk/funcs/func_connectedline.c PRE-CREATION 
>   /trunk/funcs/func_redirecting.c PRE-CREATION 
>   /trunk/include/asterisk/callerid.h 183691 
>   /trunk/include/asterisk/channel.h 183691 
>   /trunk/include/asterisk/frame.h 183691 
>   /trunk/main/callerid.c 183691 
>   /trunk/main/channel.c 183691 
>   /trunk/main/dial.c 183691 
>   /trunk/main/features.c 183691 
>   /trunk/main/rtp.c 183691 
> 
> 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
> 
>