[asterisk-dev] [Code Review] 2486: Rework CDRs for Asterisk 12
jrose
reviewboard at asterisk.org
Wed May 8 16:36:36 CDT 2013
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/2486/#review8508
-----------------------------------------------------------
team/group/bridge_construction/apps/app_cdr.c
<https://reviewboard.asterisk.org/r/2486/#comment16433>
You can reduce nesting here with:
if (ast_test_flag(&flags, AST_CDR_FLAG_DISABLE_ALL) &&
ast_cdr_clear_property(ast_channel_name(chan), AST_CDR_FLAG_DISABLE_ALL)) {
res = 1;
}
team/group/bridge_construction/channels/chan_agent.c
<https://reviewboard.asterisk.org/r/2486/#comment16440>
Not sure if this is worthwhile, but you could always convert this to use the standard application argument parsing stuff.
team/group/bridge_construction/funcs/func_cdr.c
<https://reviewboard.asterisk.org/r/2486/#comment16445>
So is this character buffer here malloced to avoid a stack overflow or something? It doesn't seem like there is really that much on the stack in this function though... maybe a few thousand bytes worth of strings if the parse string is really crazy. Seems odd to do that over a 128 character string buffer.
I might be completely off base with that assumption.
team/group/bridge_construction/funcs/func_cdr.c
<https://reviewboard.asterisk.org/r/2486/#comment16446>
This could mean that billsec wasn't retrieved, but it could also mean that it just wasn't in the right format for some reason rather than that it actually failed to retrieve it. It might be appropriate to post the string representation of the buffer. Possibly just in a debug log message.
team/group/bridge_construction/funcs/func_cdr.c
<https://reviewboard.asterisk.org/r/2486/#comment16447>
idem
team/group/bridge_construction/funcs/func_cdr.c
<https://reviewboard.asterisk.org/r/2486/#comment16448>
more
team/group/bridge_construction/funcs/func_cdr.c
<https://reviewboard.asterisk.org/r/2486/#comment16449>
and here too.
team/group/bridge_construction/include/asterisk/cdr.h
<https://reviewboard.asterisk.org/r/2486/#comment16450>
lifecycle->life cycle
team/group/bridge_construction/include/asterisk/cdr.h
<https://reviewboard.asterisk.org/r/2486/#comment16451>
Strangly -> Strangely
team/group/bridge_construction/include/asterisk/cdr.h
<https://reviewboard.asterisk.org/r/2486/#comment16452>
Looks like you might have some unnecessary white space before the #endif here.
team/group/bridge_construction/include/asterisk/channel.h
<https://reviewboard.asterisk.org/r/2486/#comment16454>
Switch these tabs for spaces
team/group/bridge_construction/include/asterisk/config_options.h
<https://reviewboard.asterisk.org/r/2486/#comment16455>
Tabs to spaces for the documentation alignment.
team/group/bridge_construction/include/asterisk/stasis_channels.h
<https://reviewboard.asterisk.org/r/2486/#comment16456>
tabs to spaces for the documentation alignment
team/group/bridge_construction/include/asterisk/test.h
<https://reviewboard.asterisk.org/r/2486/#comment16457>
sucess -> success
team/group/bridge_construction/include/asterisk/test.h
<https://reviewboard.asterisk.org/r/2486/#comment16458>
sucess -> success again
team/group/bridge_construction/include/asterisk/test.h
<https://reviewboard.asterisk.org/r/2486/#comment16459>
sucess again.
team/group/bridge_construction/main/cdr.c
<https://reviewboard.asterisk.org/r/2486/#comment16460>
'of of this extension'
doubled the 'of's
team/group/bridge_construction/main/cdr.c
<https://reviewboard.asterisk.org/r/2486/#comment16462>
no space between The and \ref. Not sure if this is a problem, but it's not typical anyway.
team/group/bridge_construction/main/cdr.c
<https://reviewboard.asterisk.org/r/2486/#comment16463>
idem
team/group/bridge_construction/main/cdr.c
<https://reviewboard.asterisk.org/r/2486/#comment16466>
All of these should be aligned with spaces instead of tabs.
team/group/bridge_construction/main/cdr.c
<https://reviewboard.asterisk.org/r/2486/#comment16479>
Probably unnecessary, but rather than getting an iterator, checking for the iterator, etc. you could probably just execute the contents of your while loop in a separate function ran by the ao2_callback.
team/group/bridge_construction/main/cdr.c
<https://reviewboard.asterisk.org/r/2486/#comment16477>
team/group/bridge_construction/main/cdr.c
<https://reviewboard.asterisk.org/r/2486/#comment16478>
I sort of feel like this should be converted in a separate function. Strings to disposition values and disposition values to the dial status string I mean.
so this would become:
cdr->disposition = dial_status_to_cause(dial_status);
if (cdr->disposition == AST_CDR_ANSWERED) {
cdr_object_transition_state(cdr, &dialed_pending_state_fn_table);
}
Something like that anyway.
team/group/bridge_construction/main/cdr.c
<https://reviewboard.asterisk.org/r/2486/#comment16480>
Similar to above comment about directly performing while loop actions through ao2_callback.
team/group/bridge_construction/main/cdr.c
<https://reviewboard.asterisk.org/r/2486/#comment16481>
Nesting for this loop seems pretty bad and it could be reduced at least a little by changing the else clause against if (new_shapshot) to instead be:
if (!new_snapshot) {
...
ao2_unlock(cdr);
continue;
}
/* nominal stuff */
team/group/bridge_construction/main/cdr.c
<https://reviewboard.asterisk.org/r/2486/#comment16482>
same construction here
team/group/bridge_construction/main/cdr.c
<https://reviewboard.asterisk.org/r/2486/#comment16483>
and here within the same function. A lot of this code appears to be shared between the two passes and could probably be cleaned up a bit too.
team/group/bridge_construction/main/cdr.c
<https://reviewboard.asterisk.org/r/2486/#comment16484>
Heavy nesting in the else clause here. It looks like you could just return in the if condition too.
This is where I'm leaving off for the first part of this review. The third page on this one is huge.
- jrose
On May 1, 2013, 6:37 p.m., Matt Jordan wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/2486/
> -----------------------------------------------------------
>
> (Updated May 1, 2013, 6:37 p.m.)
>
>
> Review request for Asterisk Developers.
>
>
> Bugs: ASTERISK-21196
> https://issues.asterisk.org/jira/browse/ASTERISK-21196
>
>
> Repository: Asterisk
>
>
> Description
> -------
>
> Sorry for e-mail spam folks, but apparently Review Board chocked on the last posting. Hopefully this one takes.
>
> This patch reworks CDRs for Asterisk 12. For more information on why and what the records should look like, please view the specification on the wiki: https://wiki.asterisk.org/wiki/display/AST/Asterisk+12+CDR+Specification
>
> Since the Bridge API migration resulted in all CDR code in features.c no longer being applicable, and the model of masquerades during transfers now no longer occurs (mostly), the vast majority of complex CDR code sprinkled throughout Asterisk was either wrong, or would never be executed. Rather than piling the CDR code back into the Bridging API, this patch instead builds CDRs off of messages received over the Stasis-Core message bus. However, as a result, all direct manipulation of CDRs by channel drivers, applications, and the Asterisk core is now highly suspect and most likely null and void.
>
> This patch rips most of the existing code out.
>
> Messages received over Stasis-Core are first dispatched by top level message handlers. The handlers job is to (a) find the CDRs that are affected by the message, (b) dispatch the message to the CDRs, and (c) based on the statuses, determine if more CDRs are needed.
>
> Each CDR has a virtual table that determines what effect the message has on it. Depending on the state of the CDR, the message may be dropped; invalid; or it may alter the state of the CDR. CDRs transition states when they need to, and the top level message handlers generally don't care what state the CDR is in.
>
> CDRs exist in a chain. As a channel does stuff, additional CDRs may be appended to the chain, and there may be multiple active CDRs at any given moment in time.
>
> As a result of all of this, ForkCDR, NoCDR, and ResetCDR have essentially been rewritten and simplified greatly. The need for ForkCDR is significantly less, as additional CDRs are created between all pairs of channels in multi-party and transfer situations.
>
> A new CDR function, CDR_PROP, has been added that lets the dialplan override who is the Party A when two channels enter into a bridge together. This function also lets you disable/re-enable CDRs on a channel, which precludes the need for NoCDR and one of ResetCDR's options.
>
> Finally, a set of unit tests have been written that cover the existing functionality.
>
> This patch is not complete - there is still some additional work to do and testing to perform. The following needs to be covered under subsequent reviews:
> * Linkedid propagation. As CEL will need this as well, this will most likely become a core concept (if possible)
> * Park. A unit test exists for this, however, the holding bridge is not yet handled properly. This is rather trivial, as most of the Bridge state logic simply needs to be avoided if a holding bridge is entered.
> * Call Pickup. We need the Stasis Message for this before this can be completed.
> * Attended transfers into an application. We need the Stasis Message for this too.
> * Quite a few BUGBUG comments in app_queue (which needs the Dial messages - see ASTERISK-21551) and the media channel (see ASTERISK-21713)
>
> Hopefully, this patch is far enough along that the approach can be verified and major bugs can be commented on. Ideally, this would go into bridge_construction so that the Test Suite running against it can be updated.
>
> A few open questions:
> * What do we want to do with peeraccount? In general, this property on the channel needs to be set by the bridging API, but much like the BRIDGEPEER channel variable, has issues in reasonably complex scenarios. Right now, the peeraccount is passed through the CDRs by virtue of the CDR engine knowing who the channel is bridged with, but inspecting this value through a function becomes problematic since we can't return a single value in multi-party bridges. CEL will also need an answer to this question. (There is also some bridging logic that applies a channel's accountcode to who its bridged with if the other channel doesn't have an accountcode; this does need to be performed by the bridging layer)
> * There is certainly room for debate with where the userfield, accountcode, and amaflags should live. Currently that is CDR, channel, channel. The channel userfield is generally unused. These could all be combined; however, that forces some parity between CEL userfield and CDR userfield that didn't used to exist.
>
>
> Diffs
> -----
>
> team/group/bridge_construction/include/asterisk/config_options.h 387018
> team/group/bridge_construction/include/asterisk/channel.h 387018
> team/group/bridge_construction/include/asterisk/cel.h 387018
> team/group/bridge_construction/include/asterisk/cdr.h 387018
> team/group/bridge_construction/funcs/func_channel.c 387018
> team/group/bridge_construction/funcs/func_cdr.c 387018
> team/group/bridge_construction/funcs/func_callerid.c 387018
> team/group/bridge_construction/channels/chan_unistim.c 387018
> team/group/bridge_construction/channels/chan_skinny.c 387018
> team/group/bridge_construction/channels/chan_sip.c 387018
> team/group/bridge_construction/channels/chan_mgcp.c 387018
> team/group/bridge_construction/channels/chan_local.c 387018
> team/group/bridge_construction/channels/chan_iax2.c 387018
> team/group/bridge_construction/channels/chan_h323.c 387018
> team/group/bridge_construction/channels/chan_dahdi.c 387018
> team/group/bridge_construction/channels/chan_agent.c 387018
> team/group/bridge_construction/cel/cel_tds.c 387018
> team/group/bridge_construction/cel/cel_radius.c 387018
> team/group/bridge_construction/cel/cel_manager.c 387018
> team/group/bridge_construction/cdr/cdr_tds.c 387018
> team/group/bridge_construction/cdr/cdr_syslog.c 387018
> team/group/bridge_construction/cdr/cdr_radius.c 387018
> team/group/bridge_construction/cdr/cdr_pgsql.c 387018
> team/group/bridge_construction/cdr/cdr_odbc.c 387018
> team/group/bridge_construction/cdr/cdr_manager.c 387018
> team/group/bridge_construction/cdr/cdr_custom.c 387018
> team/group/bridge_construction/cdr/cdr_csv.c 387018
> team/group/bridge_construction/cdr/cdr_adaptive_odbc.c 387018
> team/group/bridge_construction/bridges/bridge_softmix.c 387018
> team/group/bridge_construction/apps/app_queue.c 387018
> team/group/bridge_construction/apps/app_osplookup.c 387018
> team/group/bridge_construction/apps/app_forkcdr.c 387018
> team/group/bridge_construction/apps/app_followme.c 387018
> team/group/bridge_construction/apps/app_dumpchan.c 387018
> team/group/bridge_construction/apps/app_disa.c 387018
> team/group/bridge_construction/apps/app_dial.c 387018
> team/group/bridge_construction/apps/app_cdr.c 387018
> team/group/bridge_construction/apps/app_authenticate.c 387018
> team/group/bridge_construction/addons/chan_ooh323.c 387018
> team/group/bridge_construction/addons/cdr_mysql.c 387018
> team/group/bridge_construction/UPGRADE.txt 387018
> team/group/bridge_construction/CHANGES 387018
> team/group/bridge_construction/include/asterisk/stasis_channels.h 387018
> team/group/bridge_construction/include/asterisk/strings.h 387018
> team/group/bridge_construction/include/asterisk/test.h 387018
> team/group/bridge_construction/include/asterisk/time.h 387018
> team/group/bridge_construction/main/asterisk.c 387018
> team/group/bridge_construction/main/bridging.c 387018
> team/group/bridge_construction/main/cdr.c 387018
> team/group/bridge_construction/main/cel.c 387018
> team/group/bridge_construction/main/channel.c 387018
> team/group/bridge_construction/main/channel_internal_api.c 387018
> team/group/bridge_construction/main/cli.c 387018
> team/group/bridge_construction/main/config_options.c 387018
> team/group/bridge_construction/main/features.c 387018
> team/group/bridge_construction/main/manager.c 387018
> team/group/bridge_construction/main/manager_channels.c 387018
> team/group/bridge_construction/main/pbx.c 387018
> team/group/bridge_construction/main/stasis_channels.c 387018
> team/group/bridge_construction/main/test.c 387018
> team/group/bridge_construction/main/utils.c 387018
> team/group/bridge_construction/res/res_agi.c 387018
> team/group/bridge_construction/res/res_config_sqlite.c 387018
> team/group/bridge_construction/res/res_monitor.c 387018
> team/group/bridge_construction/res/res_stasis.c 387018
> team/group/bridge_construction/tests/test_cdr.c PRE-CREATION
>
> Diff: https://reviewboard.asterisk.org/r/2486/diff/
>
>
> Testing
> -------
>
> Unit testing
>
>
> Thanks,
>
> Matt Jordan
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20130508/ce40b894/attachment-0001.htm>
More information about the asterisk-dev
mailing list