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

Russell Bryant russell at digium.com
Wed Mar 18 16:24:42 CDT 2009 

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


Round 1 of comments


/trunk/CHANGES
<http://reviewboard.digium.com/r/201/#comment1324>

    Thanks for adding all of this stuff to CHANGES!
    
    Would you mind changing the formatting of this heading somehow so that it doesn't look the same as the headings between versions?



/trunk/apps/app_dial.c
<http://reviewboard.digium.com/r/201/#comment1325>

    This is an unrelated change, and should be committed separately.



/trunk/apps/app_dial.c
<http://reviewboard.digium.com/r/201/#comment1326>

    trim the whitespace



/trunk/channels/chan_iax2.c
<http://reviewboard.digium.com/r/201/#comment1327>

    In the places where you're doing 2 sets or 2 clears, you can use a single line to do it by ORing the flags together.
    
    There are some other places where this could be done, as well.



/trunk/channels/chan_local.c
<http://reviewboard.digium.com/r/201/#comment1331>

    There is probably a potential for deadlock here.  I'm not sure what other parts of chan_local do about this, but you're trying to lock 2 channels (this_channel is locked before this function is called, and the_other_channel gets locked to queue the frame), and another code path could be doing it another order.



/trunk/channels/chan_local.c
<http://reviewboard.digium.com/r/201/#comment1332>

    Check for allocation failure.  



/trunk/channels/chan_local.c
<http://reviewboard.digium.com/r/201/#comment1333>

    Are both channels properly locked for handling these read and write operations?



/trunk/channels/chan_sip.c
<http://reviewboard.digium.com/r/201/#comment1334>

    move { to next line



/trunk/channels/chan_sip.c
<http://reviewboard.digium.com/r/201/#comment1335>

    This code assumes that both the sip_pvt and the pvt->owner are locked.  I would document the function as such, and also verify that this is being done.



/trunk/configs/sip.conf.sample
<http://reviewboard.digium.com/r/201/#comment1336>

    Please use spaces for indentation here



/trunk/funcs/func_connectedline.c
<http://reviewboard.digium.com/r/201/#comment1337>

    Can you convert this to XML docs?



/trunk/funcs/func_redirecting.c
<http://reviewboard.digium.com/r/201/#comment1338>

    Please check for allocation failure



/trunk/funcs/func_redirecting.c
<http://reviewboard.digium.com/r/201/#comment1339>

    Can you convert this to XML docs?



/trunk/include/asterisk/callerid.h
<http://reviewboard.digium.com/r/201/#comment1340>

    Please use doxygen formatted comments.  Also, I would prefer that they were not on the same line.



/trunk/include/asterisk/callerid.h
<http://reviewboard.digium.com/r/201/#comment1341>

    Please fully document all new API calls.



/trunk/include/asterisk/channel.h
<http://reviewboard.digium.com/r/201/#comment1342>

    Please document all new API calls.



/trunk/include/asterisk/channel.h
<http://reviewboard.digium.com/r/201/#comment1343>

    Please complete the documentation here to include parameters.



/trunk/main/callerid.c
<http://reviewboard.digium.com/r/201/#comment1344>

    The documentation here should actually be removed, since these are API calls, so the documentation belongs in the header file.
    
    It's also worth noting that these changes are just formatting changes unrelated to the work done in this patch.  There seems to be a lot of this in the doxygen sections.
    
    At this point, it's not really worth going back and changing them, but in the future, please try to avoid making unrelated changes in a patch.  It makes it much more difficult to review the code and what changes are actually being made.



/trunk/main/callerid.c
<http://reviewboard.digium.com/r/201/#comment1345>

    The documentation of this API call should be in the header file.  This same comment applies to a number of other places.



/trunk/main/callerid.c
<http://reviewboard.digium.com/r/201/#comment1346>

    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.



/trunk/main/channel.c
<http://reviewboard.digium.com/r/201/#comment1347>

    Please check for memory allocation failure.  This applies to many uses of ast_strdup() in this patch.



/trunk/main/channel.c
<http://reviewboard.digium.com/r/201/#comment1348>

    Remove this commented out code if it is no longer needed.



/trunk/main/channel.c
<http://reviewboard.digium.com/r/201/#comment1349>

    remove commented out code



/trunk/main/channel.c
<http://reviewboard.digium.com/r/201/#comment1350>

    commented out code



/trunk/main/dial.c
<http://reviewboard.digium.com/r/201/#comment1351>

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



/trunk/main/features.c
<http://reviewboard.digium.com/r/201/#comment1352>

    I think the channel is erroneously left unlocked here



/trunk/main/features.c
<http://reviewboard.digium.com/r/201/#comment1353>

    The channel needs to be locked here



/trunk/main/features.c
<http://reviewboard.digium.com/r/201/#comment1354>

    missing channel locking here



/trunk/main/features.c
<http://reviewboard.digium.com/r/201/#comment1355>

    missing channel locking here


- Russell


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

More information about the asterisk-dev mailing list