[asterisk-dev] [Code Review]: logger: Call ID logging phase III part 1 split - chan_iax2 and chan_dahdi

[jrose]

> On June 20, 2012, 4:56 p.m., rmudgett wrote:
> > /trunk/channels/sig_pri.c, lines 5495-5499
> > <https://reviewboard.asterisk.org/r/1927/diff/4/?file=28960#file28960line5495>
> >
> >     You have not protected against a possible NULL callid pointer.
> >     
> >     Also look at how sig_pri_lock_owner is used elsewhere.  sig_pri_lock_owner will return with the owner locked if it is not NULL.

First point corrected.

On the second point, I was slightly confused, but from what I see it looks like we usually just use sig_pri_lock_owner before checking to see if pri->pvts[chanpos]->owner is not NULL normally, so that's what I changed this to do.


> On June 20, 2012, 4:56 p.m., rmudgett wrote:
> > /trunk/channels/sig_pri.c, lines 7296-7334
> > <https://reviewboard.asterisk.org/r/1927/diff/4/?file=28960#file28960line7296>
> >
> >     These routines should be self contained as far as callid is concerned so you don't need to change the function prototypes.  Besides, you have not converted the called functions anyway.

Ah crap, I forgot to actually add the changes to those.  Well, you are right though.  They can handle the call id cleanup themselves, so they don't need to copy the pointer address.


> On June 20, 2012, 4:56 p.m., rmudgett wrote:
> > /trunk/channels/chan_dahdi.c, line 14304
> > <https://reviewboard.asterisk.org/r/1927/diff/4/?file=28956#file28956line14304>
> >
> >     In this function change the logging functions to explicit callid versions.  We don't need to see the callid information in the PRI debug logging output.
> >     
> >     For this function it is ast_log_callid and pass NULL for the callid.

I don't really see why we wouldn't want to have the callid information for these log messages.  If the thread is bound to a call id when it shouldn't be, that's a problem... but if we bound a thread to a call id for a legitimate reason when these functions were called, that generally means they are associated with a call of some kind.  Could you explain?


- jrose


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/1927/#review6517
-----------------------------------------------------------


On June 20, 2012, 4:27 p.m., jrose wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/1927/
> -----------------------------------------------------------
> 
> (Updated June 20, 2012, 4:27 p.m.)
> 
> 
> Review request for Asterisk Developers, rmudgett and Matt Jordan.
> 
> 
> Summary
> -------
> 
> split from: https://reviewboard.asterisk.org/r/1886/
> These changes allow channels dahdi and iax2 to set callids and bind log messages to callids.
> 
> This is the same code as in the earlier review.  It was split to commit the approved parts and to hopefully encourage some review of the remaining parts since there is less in here to review now.
> 
> 
> Diffs
> -----
> 
>   /trunk/channels/chan_dahdi.c 369042 
>   /trunk/channels/chan_agent.c 369042 
>   /trunk/channels/chan_iax2.c 369042 
>   /trunk/channels/chan_local.c 369042 
>   /trunk/channels/sig_analog.c 369042 
>   /trunk/channels/sig_pri.c 369042 
>   /trunk/channels/sig_ss7.c 369042 
>   /trunk/main/autoservice.c 369042 
>   /trunk/main/bridging.c 369042 
>   /trunk/main/logger.c 369042 
>   /trunk/main/pbx.c 369042 
> 
> Diff: https://reviewboard.asterisk.org/r/1927/diff
> 
> 
> Testing
> -------
> 
> see https://reviewboard.asterisk.org/r/1886/
> 
> 
> Thanks,
> 
> jrose
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20120621/3c5fe04f/attachment-0001.htm>