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

[Mark Michelson]

> On 2009-03-23 12:37:08, Russell Bryant wrote:
> > /trunk/apps/app_queue.c, line 633
> > <http://reviewboard.digium.com/r/201/diff/6/?file=3509#file3509line633>
> >
> >     This could be declared as a boolean.

Done


> On 2009-03-23 12:37:08, Russell Bryant wrote:
> > /trunk/apps/app_queue.c, lines 2462-2482
> > <http://reviewboard.digium.com/r/201/diff/6/?file=3509#file3509line2462>
> >
> >     This section needs some channel locking love.

Got it taken care of, boss.


> On 2009-03-23 12:37:08, Russell Bryant wrote:
> > /trunk/apps/app_queue.c, lines 2840-2842
> > <http://reviewboard.digium.com/r/201/diff/6/?file=3509#file3509line2840>
> >
> >     Channel locking needed here

FIXT


> On 2009-03-23 12:37:08, Russell Bryant wrote:
> > /trunk/apps/app_queue.c, lines 2906-2916
> > <http://reviewboard.digium.com/r/201/diff/6/?file=3509#file3509line2906>
> >
> >     Channel locking needed here

Snap!


> On 2009-03-23 12:37:08, Russell Bryant wrote:
> > /trunk/apps/app_queue.c, lines 2941-2943
> > <http://reviewboard.digium.com/r/201/diff/6/?file=3509#file3509line2941>
> >
> >     Channel locking

Crackle!


> On 2009-03-23 12:37:08, Russell Bryant wrote:
> > /trunk/channels/chan_misdn.c, lines 627-630
> > <http://reviewboard.digium.com/r/201/diff/6/?file=3516#file3516line627>
> >
> >     There are still a lot of these "end" comments that need to be stripped before we can merge this.

I applied "%s/\t\/\* end[^*]*\*\///g" to chan_misdn.c, channel.c, and callerid.c

That took care of about 80 instances. If you see any more, let me know what file they're in.


> On 2009-03-23 12:37:08, Russell Bryant wrote:
> > /trunk/channels/chan_sip.c, lines 9054-9057
> > <http://reviewboard.digium.com/r/201/diff/6/?file=3518#file3518line9054>
> >
> >     I'm not sure if I commented on this already, but this function (add_rpid()) assumes that p->owner is locked.  Could you please verify that this is handled properly?  Also, after doing so, document p->owner being locked as a precondition.

All right. I already have made sure that p->owner is locked as per your last set of comments. I will add to the documentation the precondition.


> On 2009-03-23 12:37:08, Russell Bryant wrote:
> > /trunk/funcs/func_redirecting.c, lines 52-98
> > <http://reviewboard.digium.com/r/201/diff/6/?file=3530#file3530line52>
> >
> >     There are some errors in this XML documentation.  Run "make validate-docs" to see them.

Fixed.


> On 2009-03-23 12:37:08, Russell Bryant wrote:
> > /trunk/channels/chan_misdn.c, lines 979-994
> > <http://reviewboard.digium.com/r/201/diff/6/?file=3516#file3516line979>
> >
> >     It would be nice to stick these values in an enum.

I'll leave this and the other chan_misdn changes to Richard.


- Mark


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


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
> 
>