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

Wed Mar 18 18:56:58 CDT 2009

> On 2009-03-18 16:24:42, Russell Bryant wrote:
> > Round 1 of comments

Thanks for the quick review, Russell! I haven't addressed everything here, but I think I've made it sufficiently clear what I have and have not yet fixed. Most of what I haven't addressed is just because I didn't have time to get to it yet.

> On 2009-03-18 16:24:42, Russell Bryant wrote:
> > /trunk/CHANGES, lines 25-27
> > <http://reviewboard.digium.com/r/201/diff/3/?file=3223#file3223line25>
> >
> >     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?

Done.

> On 2009-03-18 16:24:42, Russell Bryant wrote:
> > /trunk/apps/app_dial.c, lines 639-646
> > <http://reviewboard.digium.com/r/201/diff/3/?file=3224#file3224line639>
> >
> >     This is an unrelated change, and should be committed separately.

I reverted the braces on the if statement and the addition of the doxygen \brief tag.

> On 2009-03-18 16:24:42, Russell Bryant wrote:
> > /trunk/apps/app_dial.c, line 778
> > <http://reviewboard.digium.com/r/201/diff/3/?file=3224#file3224line778>
> >
> >     trim the whitespace

Done. Also, next time you add a comment like this, would you mind selecting a few more lines of code? It's hard to easily find the spot in the code just from seeing one blank line. I know the line number is given, but it's not helpful if more changes have been merged into app_dial since I posted this.

> On 2009-03-18 16:24:42, Russell Bryant wrote:
> > /trunk/channels/chan_iax2.c, lines 11230-11243
> > <http://reviewboard.digium.com/r/201/diff/3/?file=3230#file3230line11230>
> >
> >     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.

Done. I found three places to make such changes. If I missed any, let me know.

> On 2009-03-18 16:24:42, Russell Bryant wrote:
> > /trunk/channels/chan_local.c, lines 428-435
> > <http://reviewboard.digium.com/r/201/diff/3/?file=3231#file3231line428>
> >
> >     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.

In all other cases, chan_local queues frames on the other channel using local_queue_frame, which has deadlock avoidance built into it. Unfortunately, with the way connected line and redirecting updates work, using local_queue_frame here is not feasible (see the large comment above this section for further details).

This is going to require a bit more thought to get correct. One way that would work would be to make the build_connected_line_data and build_redirecting_data functions in channel.c public so that we can then use local_queue_frame and not have to worry about the potential for deadlock.

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

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. 

> On 2009-03-18 16:24:42, Russell Bryant wrote:
> > /trunk/channels/chan_sip.c, line 9026
> > <http://reviewboard.digium.com/r/201/diff/3/?file=3235#file3235line9026>
> >
> >     move { to next line

done

> On 2009-03-18 16:24:42, Russell Bryant wrote:
> > /trunk/channels/chan_sip.c, lines 10146-10148
> > <http://reviewboard.digium.com/r/201/diff/3/?file=3235#file3235line10146>
> >
> >     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.

This function is called from only two places. I can verify that in one of the places, the channel is locked but the sip_pvt is not. That's easy to fix. The other place where this is called is a bit more difficult to figure out all the locks. I've got this marked as something to investigate further.

> On 2009-03-18 16:24:42, Russell Bryant wrote:
> > /trunk/configs/sip.conf.sample, lines 212-219
> > <http://reviewboard.digium.com/r/201/diff/3/?file=3245#file3245line212>
> >
> >     Please use spaces for indentation here

No problem. I could have sworn I fixed that. Oh well, must've done that for something else. Done.

> On 2009-03-18 16:24:42, Russell Bryant wrote:
> > /trunk/funcs/func_connectedline.c, lines 187-204
> > <http://reviewboard.digium.com/r/201/diff/3/?file=3246#file3246line187>
> >
> >     Can you convert this to XML docs?

Done.

> On 2009-03-18 16:24:42, Russell Bryant wrote:
> > /trunk/funcs/func_redirecting.c, lines 196-197
> > <http://reviewboard.digium.com/r/201/diff/3/?file=3247#file3247line196>
> >
> >     Please check for allocation failure

I've made it so that allocation failures here will return ID_FIELD_INVALID. Richard may want to comment on whether this is correct or not.

> On 2009-03-18 16:24:42, Russell Bryant wrote:
> > /trunk/funcs/func_redirecting.c, lines 375-403
> > <http://reviewboard.digium.com/r/201/diff/3/?file=3247#file3247line375>
> >
> >     Can you convert this to XML docs?

Done.

> On 2009-03-18 16:24:42, Russell Bryant wrote:
> > /trunk/include/asterisk/callerid.h, lines 414-420
> > <http://reviewboard.digium.com/r/201/diff/3/?file=3248#file3248line414>
> >
> >     Please use doxygen formatted comments.  Also, I would prefer that they were not on the same line.

Done.

> On 2009-03-18 16:24:42, Russell Bryant wrote:
> > /trunk/include/asterisk/callerid.h, lines 422-424
> > <http://reviewboard.digium.com/r/201/diff/3/?file=3248#file3248line422>
> >
> >     Please fully document all new API calls.
> 
>  wrote:
>     These functions are fully documented at the function definition.

Richard, would you mind taking on the task of moving the doxygen documentation from the function definitions and putting them here with the prototypes? Thanks.

> On 2009-03-18 16:24:42, Russell Bryant wrote:
> > /trunk/include/asterisk/channel.h, lines 2030-2044
> > <http://reviewboard.digium.com/r/201/diff/3/?file=3249#file3249line2030>
> >
> >     Please document all new API calls.
> 
>  wrote:
>     These functions are fully documented at the function definition.

Same here, too.

> On 2009-03-18 16:24:42, Russell Bryant wrote:
> > /trunk/include/asterisk/channel.h, lines 2046-2088
> > <http://reviewboard.digium.com/r/201/diff/3/?file=3249#file3249line2046>
> >
> >     Please complete the documentation here to include parameters.
> 
>  wrote:
>     Fully documented at the function definition.

And again.

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

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.

> On 2009-03-18 16:24:42, Russell Bryant wrote:
> > /trunk/main/dial.c, lines 176-183
> > <http://reviewboard.digium.com/r/201/diff/3/?file=3253#file3253line176>
> >
> >     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 ...

Yeah, apparently this macro is in both apps/app_dial.c and main/dial.c. Moving this to strings.h would make sense to me, since I think this same construct is used in many places. For now, I've reverted the change from free to ast_free.

> On 2009-03-18 16:24:42, Russell Bryant wrote:
> > /trunk/main/features.c, line 1493
> > <http://reviewboard.digium.com/r/201/diff/3/?file=3254#file3254line1493>
> >
> >     I think the channel is erroneously left unlocked here

Fixed.

> On 2009-03-18 16:24:42, Russell Bryant wrote:
> > /trunk/main/features.c, line 1587
> > <http://reviewboard.digium.com/r/201/diff/3/?file=3254#file3254line1587>
> >
> >     The channel needs to be locked here

Fixed.

> On 2009-03-18 16:24:42, Russell Bryant wrote:
> > /trunk/main/features.c, lines 1692-1695
> > <http://reviewboard.digium.com/r/201/diff/3/?file=3254#file3254line1692>
> >
> >     missing channel locking here

Fixed.

> On 2009-03-18 16:24:42, Russell Bryant wrote:
> > /trunk/main/features.c, line 2201
> > <http://reviewboard.digium.com/r/201/diff/3/?file=3254#file3254line2201>
> >
> >     missing channel locking here

Fixed.

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

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.

- Mark

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