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

[Russell Bryant]

> On 2009-03-18 16:24:42, Russell Bryant wrote:
> > /trunk/channels/chan_local.c, lines 556-557
> > <http://reviewboard.digium.com/r/201/diff/3/?file=3231#file3231line556>
> >
> >     Are both channels properly locked for handling these read and write operations?
> 
>  wrote:
>     p->owner is locked, but p->chan is not. Though in all fairness, I can't really understand how or why anything would be accessing p->chan at this point. If you'll recall, p->chan is the Local channel that runs dialplan operations. At the time this is called, we haven't even started the PBX on p->chan. Admittedly, p->chan is in the channels list, but I still have no idea how or why anything would be accessing it.

As soon as the channel is in the global channel's list, you must be prepared for other threads to attempt to read and write fields.  As unlikely as it may be, it is possible, and code must be prepared for it.

A real example is the manager setvar action.  You can use that to set the CONNECTEDLINE() function, for example.


> On 2009-03-18 16:24:42, Russell Bryant wrote:
> > /trunk/main/callerid.c, line 1392
> > <http://reviewboard.digium.com/r/201/diff/3/?file=3251#file3251line1392>
> >
> >     Personally, I find the comments placed at the end of switch statements, loops, and functions distracting.  This is not something that we have put in our formatting guielines, and none of the existing code uses it.
> >     
> >     If a lot of people agree that it's something we should start doing, then we could add it to the guidelines and merge this as is.  Otherwise, I'd rather all of it were removed.
> 
>  wrote:
>     These end of switch, loop, and function definition comments are useful when the beginning is several pages earlier.  They also make it easier when you want to add new code after a loop or switch and need to identify the curly brace when several blocks end at the same time.  Admittedly, they are not as useful when the item is short.
> 
>  wrote:
>     I'm of the opinion that these sorts of comments aren't really helpful and just make things look "messy." If I need to find where a curly brace matches, I can press % in vim to figure that out. For #ifdef/#endif sections, however, I sometimes find such comments to be a requirement in order to properly understand what is going on.

I would agree that the comments are useful for #ifdef/#endif stuff.


> On 2009-03-18 16:24:42, Russell Bryant wrote:
> > /trunk/main/channel.c, lines 1501-1513
> > <http://reviewboard.digium.com/r/201/diff/3/?file=3252#file3252line1501>
> >
> >     Remove this commented out code if it is no longer needed.
> 
>  wrote:
>     This code is relevant for when the struct ast_callerid is broken up.

Alright.  If that's not clear in a comment, we should add one there, or someone like me will come across it later and remove it thinking it's left over.  :-)


> On 2009-03-18 16:24:42, Russell Bryant wrote:
> > /trunk/channels/chan_local.c, line 552
> > <http://reviewboard.digium.com/r/201/diff/3/?file=3231#file3231line552>
> >
> >     Check for allocation failure.
> 
>  wrote:
>     Hmm...I'm not sure this is necessary. The reason is that if the dnid is NULL, then we can still function properly. We just will have no dnid set. Of course, if you're driving more at the fact that we should bail out on the call if we have a memory allocation failure, then I can make that change.

You may be right.  It is quite a habit for me to automatically comment on a memory allocation without a check for failure.  In some of these cases, it is normal for the result to be NULL (if the parameter was NULL).

However, ideally, we would abort a call if an allocation failed that we expected to succeed.


- Russell


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