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

Thu Mar 19 16:48:44 CDT 2009

> On 2009-03-18 16:24:42, Russell Bryant wrote:
> > Round 1 of comments
> 
> Mark Michelson wrote:
>     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.
> 
> Mark Michelson wrote:
>     Oh, by the way, the items I have mentioned as being done have been committed to the issue8824 branch. I'm going to withholding posting any new diffs until all the points you have brought up have been taken care of.

All right, in case you don't care to read all my additional comments here, the short version is that I have corrected everything you have suggested except for several places where we are not checking for an allocation failure when using ast_strdup. The rest is now taken care of. Bring on more criticism!

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

I've changed this so that the two cases of ast_strdup in local_call will return -1 on failure.

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

Writing code like I just wrote for fixing this makes me feel dirty, but I believe it is fixed now.

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

Got this fixed. Actually, there could still be issues if VOCAL_DATA_HACK is defined, but no one defines that.

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

Fixed this by doing exactly what I suggested.

> On 2009-03-18 16:24:42, Russell Bryant wrote:
> > /trunk/main/callerid.c, lines 982-1002
> > <http://reviewboard.digium.com/r/201/diff/3/?file=3251#file3251line982>
> >
> >     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.

DONE AS A PLUM

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

Richard took care of that.

> 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.
> 
>  wrote:
>     Same here, too.

Richard took care of this.

> 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.
> 
>  wrote:
>     And again.

Richard took care of this.

> On 2009-03-18 16:24:42, Russell Bryant wrote:
> > /trunk/main/callerid.c, lines 1234-1242
> > <http://reviewboard.digium.com/r/201/diff/3/?file=3251#file3251line1234>
> >
> >     The documentation of this API call should be in the header file.  This same comment applies to a number of other places.

Richard took care of this.

> On 2009-03-18 16:24:42, Russell Bryant wrote:
> > /trunk/main/channel.c, line 1358
> > <http://reviewboard.digium.com/r/201/diff/3/?file=3252#file3252line1358>
> >
> >     Please check for memory allocation failure.  This applies to many uses of ast_strdup() in this patch.

I fixed the two ast_strdup calls in ast_channel_alloc.

There are a bunch of places in Asterisk where ast_strdup is used without checking for allocation failure. Unfortunately, much of the code written in channel.c for this feature was written with void return types and was not written with the idea that a failure could potentially occur. This will need to be looked at in further detail.

> 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.
> 
>  wrote:
>     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.  :-)

I see that Richard added a comment about this being future code.

> On 2009-03-18 16:24:42, Russell Bryant wrote:
> > /trunk/main/channel.c, lines 6020-6032
> > <http://reviewboard.digium.com/r/201/diff/3/?file=3252#file3252line6020>
> >
> >     remove commented out code
> 
>  wrote:
>     This code is relevant for when the struct ast_callerid is broken up.

Here, too.

> On 2009-03-18 16:24:42, Russell Bryant wrote:
> > /trunk/main/channel.c, lines 6074-6086
> > <http://reviewboard.digium.com/r/201/diff/3/?file=3252#file3252line6074>
> >
> >     commented out code
> 
>  wrote:
>     This code is relevant for when the struct ast_callerid is broken up.

And here, too.

> 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 ...
> 
>  wrote:
>     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.
> 
>  wrote:
>     The code of this macro is manually inlined in other files.  Those other files would definitely benefit if it were in a common header file.

I think a change like this is a good idea, but I also think that changing this for this particular merge is introducing scope creep. I think this should be handled separately from this merge.

> 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
> 
>  wrote:
>     Fixed.
> 
>  wrote:
>     Mark, please note that ast_party_connected_line_collect_caller() is a SHALLOW copy so the strings point to the original memory.  This function simply collects the scattered data from struct ast_callerid into the new struct ast_party_connectedline so it can be operated upon.  Any locks must be maintained because you are still dealing with the strings in the original location.

Thanks for the note. I have corrected this by changing from using ast_party_connected_line_collect_caller to using ast_copy_caller_to_connected. I also have made sure that the dynamically allocated memory in the connected struct is freed with ast_party_connected_line_free.

> 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
> 
>  wrote:
>     Fixed.
> 
>  wrote:
>     Same comment about the SHALLOW copy and locks here.

Fixed here, too.

> 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
> 
>  wrote:
>     Fixed.

Fixed here, too.

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