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

[rmudgett]

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



/trunk/channels/sig_analog.c
<https://reviewboard.asterisk.org/r/1927/#comment12397>

    Move this to right after the if (!chan) statement.



/trunk/channels/sig_analog.c
<https://reviewboard.asterisk.org/r/1927/#comment12398>

    You are not cleaning up at the break.  You could just move the callid setup to after the if break statement.



/trunk/channels/sig_pri.c
<https://reviewboard.asterisk.org/r/1927/#comment12401>

    You have a return with the owner locked here.



/trunk/channels/sig_pri.c
<https://reviewboard.asterisk.org/r/1927/#comment12400>

    Initialize to NULL.  Otherwise could be used uninitialized.



/trunk/channels/sig_pri.c
<https://reviewboard.asterisk.org/r/1927/#comment12402>

    This is func_pri_dchannel_chanpos_callid() inlined.



/trunk/channels/sig_pri.c
<https://reviewboard.asterisk.org/r/1927/#comment12403>

    This is func_pri_dchannel_chanpos_callid() inlined.



/trunk/channels/sig_pri.c
<https://reviewboard.asterisk.org/r/1927/#comment12406>

    I think you should call func_pri_dchannel_chanpos_callid() after the private is locked instead so you will cover sig_pri_handle_subcmds().



/trunk/channels/sig_pri.c
<https://reviewboard.asterisk.org/r/1927/#comment12404>

    This is func_pri_dchannel_chanpos_callid() inlined.



/trunk/channels/sig_pri.c
<https://reviewboard.asterisk.org/r/1927/#comment12405>

    This is func_pri_dchannel_chanpos_callid() inlined.



/trunk/channels/sig_pri.c
<https://reviewboard.asterisk.org/r/1927/#comment12408>

    Move callid assignment to right after the comment here because of the possibility that we could pick another channel for the call.



/trunk/channels/sig_pri.c
<https://reviewboard.asterisk.org/r/1927/#comment12407>

    Unnecessary blank line change.



/trunk/channels/sig_pri.c
<https://reviewboard.asterisk.org/r/1927/#comment12409>

    This can be put after the HANGUPCAUSE if statement below.



/trunk/channels/sig_ss7.c
<https://reviewboard.asterisk.org/r/1927/#comment12416>

    Do not pass in p.  That is what linkset and chanpos describe.



/trunk/channels/sig_ss7.c
<https://reviewboard.asterisk.org/r/1927/#comment12414>

    Initialize to NULL.



/trunk/channels/sig_ss7.c
<https://reviewboard.asterisk.org/r/1927/#comment12417>

    Revert this change.  This is for channel management.



/trunk/channels/sig_ss7.c
<https://reviewboard.asterisk.org/r/1927/#comment12418>

    Revert this change.  This is a new incoming call.



/trunk/channels/sig_ss7.c
<https://reviewboard.asterisk.org/r/1927/#comment12419>

    Revert this change.



/trunk/channels/sig_ss7.c
<https://reviewboard.asterisk.org/r/1927/#comment12420>

    Revert.



/trunk/channels/sig_ss7.c
<https://reviewboard.asterisk.org/r/1927/#comment12421>

    Revert



/trunk/channels/sig_ss7.c
<https://reviewboard.asterisk.org/r/1927/#comment12422>

    Revert.  Part of channel blocking management.



/trunk/channels/sig_ss7.c
<https://reviewboard.asterisk.org/r/1927/#comment12423>

    Revert.  Part of channel blocking management.



/trunk/channels/sig_ss7.c
<https://reviewboard.asterisk.org/r/1927/#comment12424>

    Revert



/trunk/channels/sig_ss7.c
<https://reviewboard.asterisk.org/r/1927/#comment12425>

    Revert



/trunk/channels/sig_ss7.c
<https://reviewboard.asterisk.org/r/1927/#comment12426>

    Revert



/trunk/channels/sig_ss7.c
<https://reviewboard.asterisk.org/r/1927/#comment12427>

    Revert



/trunk/channels/sig_ss7.c
<https://reviewboard.asterisk.org/r/1927/#comment12428>

    Revert



/trunk/channels/sig_ss7.c
<https://reviewboard.asterisk.org/r/1927/#comment12415>

    Move the callid cleanup to after the if (chanpos > -1) statement.



/trunk/include/asterisk/logger.h
<https://reviewboard.asterisk.org/r/1927/#comment12410>

    I think this can be deleted since nobody uses it.



/trunk/main/features.c
<https://reviewboard.asterisk.org/r/1927/#comment12395>

    Stray blank line added.



/trunk/main/features.c
<https://reviewboard.asterisk.org/r/1927/#comment12396>

    It would be better to move that stray blank line to after this line. :)



/trunk/main/logger.c
<https://reviewboard.asterisk.org/r/1927/#comment12411>

    Rather than passing in callid_mode, why not change __ast_verbose() and ast_verbose() to be like ast_log().  Get callid themselves.


- rmudgett


On June 22, 2012, 11:18 a.m., jrose wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/1927/
> -----------------------------------------------------------
> 
> (Updated June 22, 2012, 11:18 a.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_agent.c 369189 
>   /trunk/channels/chan_dahdi.c 369189 
>   /trunk/channels/chan_iax2.c 369189 
>   /trunk/channels/chan_local.c 369189 
>   /trunk/channels/sig_analog.c 369189 
>   /trunk/channels/sig_pri.c 369189 
>   /trunk/channels/sig_ss7.c 369189 
>   /trunk/include/asterisk/logger.h 369189 
>   /trunk/main/autoservice.c 369189 
>   /trunk/main/bridging.c 369189 
>   /trunk/main/features.c 369189 
>   /trunk/main/logger.c 369189 
>   /trunk/main/pbx.c 369189 
> 
> 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/20120625/c7fa66f2/attachment-0001.htm>