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

jrose reviewboard at asterisk.org
Tue Jun 26 10:28:09 CDT 2012



> On June 25, 2012, 1:19 p.m., rmudgett wrote:
> > /trunk/channels/sig_analog.c, lines 1791-1794
> > <https://reviewboard.asterisk.org/r/1927/diff/8/?file=29122#file29122line1791>
> >
> >     Move this to right after the if (!chan) statement.

check


> On June 25, 2012, 1:19 p.m., rmudgett wrote:
> > /trunk/channels/sig_analog.c, lines 3718-3723
> > <https://reviewboard.asterisk.org/r/1927/diff/8/?file=29122#file29122line3718>
> >
> >     You are not cleaning up at the break.  You could just move the callid setup to after the if break statement.

check


> On June 25, 2012, 1:19 p.m., rmudgett wrote:
> > /trunk/channels/sig_pri.c, lines 5207-5215
> > <https://reviewboard.asterisk.org/r/1927/diff/8/?file=29123#file29123line5207>
> >
> >     You have a return with the owner locked here.

	if (pri->pvts[chanpos]->owner) {
		struct ast_callid *callid;
		callid = ast_channel_callid(pri->pvts[chanpos]->owner);
		ast_channel_unlock(pri->pvts[chanpos]->owner);
		if (callid) {
			ast_callid_threadassoc_add(callid);
			return callid;
		}
	}

Is the replacement. Don't know why I did something nutty like that.


> On June 25, 2012, 1:19 p.m., rmudgett wrote:
> > /trunk/channels/sig_pri.c, line 5240
> > <https://reviewboard.asterisk.org/r/1927/diff/8/?file=29123#file29123line5240>
> >
> >     Initialize to NULL.  Otherwise could be used uninitialized.

check


> On June 25, 2012, 1:19 p.m., rmudgett wrote:
> > /trunk/channels/sig_pri.c, lines 7468-7473
> > <https://reviewboard.asterisk.org/r/1927/diff/8/?file=29123#file29123line7468>
> >
> >     This can be put after the HANGUPCAUSE if statement below.

check


> On June 25, 2012, 1:19 p.m., rmudgett wrote:
> > /trunk/channels/sig_pri.c, lines 6145-6154
> > <https://reviewboard.asterisk.org/r/1927/diff/8/?file=29123#file29123line6145>
> >
> >     Move callid assignment to right after the comment here because of the possibility that we could pick another channel for the call.

check


> On June 25, 2012, 1:19 p.m., rmudgett wrote:
> > /trunk/channels/sig_pri.c, lines 6205-6210
> > <https://reviewboard.asterisk.org/r/1927/diff/8/?file=29123#file29123line6205>
> >
> >     Unnecessary blank line change.

check


> On June 25, 2012, 1:19 p.m., rmudgett wrote:
> > /trunk/channels/sig_pri.c, lines 5599-5607
> > <https://reviewboard.asterisk.org/r/1927/diff/8/?file=29123#file29123line5599>
> >
> >     This is func_pri_dchannel_chanpos_callid() inlined.

callid = func_pri_dchannel_chanpos_callid(pri, chanpos);

check


> On June 25, 2012, 1:19 p.m., rmudgett wrote:
> > /trunk/channels/sig_pri.c, lines 5540-5548
> > <https://reviewboard.asterisk.org/r/1927/diff/8/?file=29123#file29123line5540>
> >
> >     This is func_pri_dchannel_chanpos_callid() inlined.

callid = func_pri_dchannel_chanpos_callid(pri, chanpos);

check


> On June 25, 2012, 1:19 p.m., rmudgett wrote:
> > /trunk/channels/sig_pri.c, lines 5489-5500
> > <https://reviewboard.asterisk.org/r/1927/diff/8/?file=29123#file29123line5489>
> >
> >     I think you should call func_pri_dchannel_chanpos_callid() after the private is locked instead so you will cover sig_pri_handle_subcmds().

check


> On June 25, 2012, 1:19 p.m., rmudgett wrote:
> > /trunk/channels/sig_pri.c, lines 5410-5419
> > <https://reviewboard.asterisk.org/r/1927/diff/8/?file=29123#file29123line5410>
> >
> >     This is func_pri_dchannel_chanpos_callid() inlined.

callid = func_pri_dchannel_chanpos_callid(pri, chanpos);

check


> On June 25, 2012, 1:19 p.m., rmudgett wrote:
> > /trunk/channels/sig_pri.c, lines 5350-5359
> > <https://reviewboard.asterisk.org/r/1927/diff/8/?file=29123#file29123line5350>
> >
> >     This is func_pri_dchannel_chanpos_callid() inlined.

callid = func_pri_dchannel_chanpos_callid(pri, chanpos);

check


> On June 25, 2012, 1:19 p.m., rmudgett wrote:
> > /trunk/channels/sig_ss7.c, line 741
> > <https://reviewboard.asterisk.org/r/1927/diff/8/?file=29124#file29124line741>
> >
> >     Do not pass in p.  That is what linkset and chanpos describe.

check


> On June 25, 2012, 1:19 p.m., rmudgett wrote:
> > /trunk/channels/sig_ss7.c, line 832
> > <https://reviewboard.asterisk.org/r/1927/diff/8/?file=29124#file29124line832>
> >
> >     Initialize to NULL.

check


> On June 25, 2012, 1:19 p.m., rmudgett wrote:
> > /trunk/include/asterisk/logger.h, line 111
> > <https://reviewboard.asterisk.org/r/1927/diff/8/?file=29125#file29125line111>
> >
> >     I think this can be deleted since nobody uses it.

I don't see any reason to delete that as part of this patch regardless.  Besides, some random person might be using it in local modifications or something.


> On June 25, 2012, 1:19 p.m., rmudgett wrote:
> > /trunk/channels/sig_ss7.c, lines 1356-1362
> > <https://reviewboard.asterisk.org/r/1927/diff/8/?file=29124#file29124line1356>
> >
> >     Move the callid cleanup to after the if (chanpos > -1) statement.

check


> On June 25, 2012, 1:19 p.m., rmudgett wrote:
> > /trunk/channels/sig_ss7.c, line 1271
> > <https://reviewboard.asterisk.org/r/1927/diff/8/?file=29124#file29124line1271>
> >
> >     Revert

check


> On June 25, 2012, 1:19 p.m., rmudgett wrote:
> > /trunk/channels/sig_ss7.c, line 1258
> > <https://reviewboard.asterisk.org/r/1927/diff/8/?file=29124#file29124line1258>
> >
> >     Revert

check


> On June 25, 2012, 1:19 p.m., rmudgett wrote:
> > /trunk/channels/sig_ss7.c, line 1246
> > <https://reviewboard.asterisk.org/r/1927/diff/8/?file=29124#file29124line1246>
> >
> >     Revert

check


> On June 25, 2012, 1:19 p.m., rmudgett wrote:
> > /trunk/channels/sig_ss7.c, line 1233
> > <https://reviewboard.asterisk.org/r/1927/diff/8/?file=29124#file29124line1233>
> >
> >     Revert

check


> On June 25, 2012, 1:19 p.m., rmudgett wrote:
> > /trunk/channels/sig_ss7.c, line 1220
> > <https://reviewboard.asterisk.org/r/1927/diff/8/?file=29124#file29124line1220>
> >
> >     Revert

check


> On June 25, 2012, 1:19 p.m., rmudgett wrote:
> > /trunk/channels/sig_ss7.c, lines 1204-1208
> > <https://reviewboard.asterisk.org/r/1927/diff/8/?file=29124#file29124line1204>
> >
> >     Revert.  Part of channel blocking management.

check


> On June 25, 2012, 1:19 p.m., rmudgett wrote:
> > /trunk/channels/sig_ss7.c, lines 1190-1194
> > <https://reviewboard.asterisk.org/r/1927/diff/8/?file=29124#file29124line1190>
> >
> >     Revert.  Part of channel blocking management.

check


> On June 25, 2012, 1:19 p.m., rmudgett wrote:
> > /trunk/channels/sig_ss7.c, line 1113
> > <https://reviewboard.asterisk.org/r/1927/diff/8/?file=29124#file29124line1113>
> >
> >     Revert

check


> On June 25, 2012, 1:19 p.m., rmudgett wrote:
> > /trunk/channels/sig_ss7.c, line 1097
> > <https://reviewboard.asterisk.org/r/1927/diff/8/?file=29124#file29124line1097>
> >
> >     Revert.

check


> On June 25, 2012, 1:19 p.m., rmudgett wrote:
> > /trunk/channels/sig_ss7.c, lines 1079-1080
> > <https://reviewboard.asterisk.org/r/1927/diff/8/?file=29124#file29124line1079>
> >
> >     Revert this change.

check


> On June 25, 2012, 1:19 p.m., rmudgett wrote:
> > /trunk/channels/sig_ss7.c, lines 958-959
> > <https://reviewboard.asterisk.org/r/1927/diff/8/?file=29124#file29124line958>
> >
> >     Revert this change.  This is a new incoming call.

check


> On June 25, 2012, 1:19 p.m., rmudgett wrote:
> > /trunk/channels/sig_ss7.c, lines 932-935
> > <https://reviewboard.asterisk.org/r/1927/diff/8/?file=29124#file29124line932>
> >
> >     Revert this change.  This is for channel management.

check


> On June 25, 2012, 1:19 p.m., rmudgett wrote:
> > /trunk/main/features.c, line 5416
> > <https://reviewboard.asterisk.org/r/1927/diff/8/?file=29128#file29128line5416>
> >
> >     It would be better to move that stray blank line to after this line. :)

And found a new home.


> On June 25, 2012, 1:19 p.m., rmudgett wrote:
> > /trunk/main/features.c, line 5361
> > <https://reviewboard.asterisk.org/r/1927/diff/8/?file=29128#file29128line5361>
> >
> >     Stray blank line added.

Evicted.


> On June 25, 2012, 1:19 p.m., rmudgett wrote:
> > /trunk/main/logger.c, lines 1747-1751
> > <https://reviewboard.asterisk.org/r/1927/diff/8/?file=29129#file29129line1747>
> >
> >     Rather than passing in callid_mode, why not change __ast_verbose() and ast_verbose() to be like ast_log().  Get callid themselves.

K, got it.


- jrose


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


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/20120626/27e57273/attachment-0001.htm>


More information about the asterisk-dev mailing list