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

[Mark Michelson]

> On 2009-03-24 15:46:42, Russell Bryant wrote:
> > /trunk/include/asterisk/callerid.h, lines 89-90
> > <http://reviewboard.digium.com/r/201/diff/8/?file=3637#file3637line89>
> >
> >     I know this is just a documentation formatting change here, but yikes, this function needs a buf length argument!
> >     
> >     (Don't worry about it in this patch.)

Agreed. Right now there is only one user of this function, in chan_dahdi. It supplies a buffer sized at 32000 bytes (heap-allocated). Disgusting.


> On 2009-03-24 15:46:42, Russell Bryant wrote:
> > /trunk/channels/chan_sip.c, line 10910
> > <http://reviewboard.digium.com/r/201/diff/8/?file=3624#file3624line10910>
> >
> >     Missing a space before the =

Fixed. (Yay! You're finding small things now!)


> On 2009-03-24 15:46:42, Russell Bryant wrote:
> > /trunk/channels/chan_sip.c, lines 9112-9113
> > <http://reviewboard.digium.com/r/201/diff/8/?file=3624#file3624line9112>
> >
> >     I would rather that this be an ast_str_alloca()d ast_str to simplify this bit of code.

Agreed and fixed.


> On 2009-03-24 15:46:42, Russell Bryant wrote:
> > /trunk/channels/chan_sip.c, line 8814
> > <http://reviewboard.digium.com/r/201/diff/8/?file=3624#file3624line8814>
> >
> >     The case part isn't needed here, since we're only dealing with digits.

righty-right. fixed.


> On 2009-03-24 15:46:42, Russell Bryant wrote:
> > /trunk/channels/chan_iax2.c, lines 11360-11365
> > <http://reviewboard.digium.com/r/201/diff/8/?file=3619#file3619line11360>
> >
> >     As an optimization, these could all be one line of code.

Done.


> On 2009-03-24 15:46:42, Russell Bryant wrote:
> > /trunk/apps/app_queue.c, lines 3022-3025
> > <http://reviewboard.digium.com/r/201/diff/8/?file=3615#file3615line3022>
> >
> >     channel should be locked when reading the name

Fixed this.


> On 2009-03-24 15:46:42, Russell Bryant wrote:
> > /trunk/apps/app_queue.c, line 3011
> > <http://reviewboard.digium.com/r/201/diff/8/?file=3615#file3615line3011>
> >
> >     The channel should be locked before reading the name.

Fixed all accesses of in and o->chan's name fields in wait_for_answer.


> On 2009-03-24 15:46:42, Russell Bryant wrote:
> > /trunk/apps/app_directed_pickup.c, lines 103-105
> > <http://reviewboard.digium.com/r/201/diff/8/?file=3614#file3614line103>
> >
> >     This assumes chan is locked, but it isn't.

Got it!


- Mark


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


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