[asterisk-dev] [Code Review] 2486: Rework CDRs for Asterisk 12

Matt Jordan reviewboard at asterisk.org
Thu May 16 15:19:21 CDT 2013 


> On May 8, 2013, 9:36 p.m., jrose wrote:
> > team/group/bridge_construction/apps/app_cdr.c, lines 126-130
> > <https://reviewboard.asterisk.org/r/2486/diff/1/?file=36960#file36960line126>
> >
> >     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;
> >         }

Fixed


> On May 8, 2013, 9:36 p.m., jrose wrote:
> > team/group/bridge_construction/channels/chan_agent.c, lines 2249-2258
> > <https://reviewboard.asterisk.org/r/2486/diff/1/?file=36981#file36981line2249>
> >
> >     Not sure if this is worthwhile, but you could always convert this to use the standard application argument parsing stuff.

I could, but it's chan_agent and we're going to nuke most of it later anyway. Deferred.


> On May 8, 2013, 9:36 p.m., jrose wrote:
> > team/group/bridge_construction/funcs/func_cdr.c, line 236
> > <https://reviewboard.asterisk.org/r/2486/diff/1/?file=36991#file36991line236>
> >
> >     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.

No, this is pretty goofy. I was mirroring the original declaration of ast_cdr_getvar, but really, that's just a bad idea, as it precluded using a char [] on the stack without having a second pointer.

This is now simplified to be less odd. Note that ast_cdr_formatvar is still odd, as the CDR backends were all written to expect its way of doing things.


> On May 8, 2013, 9:36 p.m., jrose wrote:
> > team/group/bridge_construction/funcs/func_cdr.c, lines 298-302
> > <https://reviewboard.asterisk.org/r/2486/diff/1/?file=36991#file36991line298>
> >
> >     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.

Actually, the only way this is hit is if you retrieve it and can't parse it. Modified to reflect the actual error reason.

(I dislike a warning that provides little information with a debug message that provides the real cause - one log message should be sufficient for error conditions)


> On May 8, 2013, 9:36 p.m., jrose wrote:
> > team/group/bridge_construction/main/cdr.c, lines 1412-1422
> > <https://reviewboard.asterisk.org/r/2486/diff/1/?file=37003#file37003line1412>
> >
> >     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.

There's a few problems with using an ao2_callback here.

First, I need to know if no one is in this bridge. The logic is different in the case where entities are in the bridge and no one is a match versus no one being in the bridge - if no one is in the bridge, we always succeed. If entities are in the bridge but the CDR is not assigned a Party B, the return code has to tell the message handler that we don't have a Party B even though we joined a bridge. (As the handler does some fancy foot work at that point with the CDR)

It'd be possible to do this with an ao2_callback by explicitly checking the CDR's party B snapshot, but that means the ao2_callback isn't really doing much here other than hiding the iteration over the matched CDRs. Doing it an ao2_callback would also mean that we would have a comparison function that did two things:
(1) Determine if the CDR is in the bridge
(2) If it is in the bridge and not finalized, perform a single_state_bridge_enter_comparison

That would make the callback function significantly more complex.

As it is, given those constraints, I think the iterator approach is simpler (and I'm all about keeping this code as simple as I can)


> On May 8, 2013, 9:36 p.m., jrose wrote:
> > team/group/bridge_construction/main/cdr.c, lines 1498-1512
> > <https://reviewboard.asterisk.org/r/2486/diff/1/?file=37003#file37003line1498>
> >
> >     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.

I'm fine with that. Fixed.


> On May 8, 2013, 9:36 p.m., jrose wrote:
> > team/group/bridge_construction/main/cdr.c, lines 1527-1533
> > <https://reviewboard.asterisk.org/r/2486/diff/1/?file=37003#file37003line1527>
> >
> >     Similar to above comment about directly performing while loop actions through ao2_callback.

This has the same rationale as to why an iterator is used instead of a callback.


> On May 8, 2013, 9:36 p.m., jrose wrote:
> > team/group/bridge_construction/main/cdr.c, lines 1921-1954
> > <https://reviewboard.asterisk.org/r/2486/diff/1/?file=37003#file37003line1921>
> >
> >     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 */

That portion isn't in a loop, so you can't do that.

You can't even really bail on the second if (!cdr), as the channel - which doesn't have a CDR of its own (and that is probably an error) - may affect another channel's CDR record where the updated channel is the Party B in the CDR.


> On May 8, 2013, 9:36 p.m., jrose wrote:
> > team/group/bridge_construction/main/cdr.c, lines 2165-2172
> > <https://reviewboard.asterisk.org/r/2486/diff/1/?file=37003#file37003line2165>
> >
> >     same construction here

Same reason.

Also: using an ao2_callback in these areas violates the principle of a function doing one thing and one thing only. When you have logic you want to apply to all items in a container, callbacks make sense (which is actually what the cdr_object_bridge_cmp_fn is doing by default). That isn't what these functions are doing however - they're getting a subset of the items in the container and then executing logic on that subset.


> On May 8, 2013, 9:36 p.m., jrose wrote:
> > team/group/bridge_construction/main/cdr.c, lines 2203-2210
> > <https://reviewboard.asterisk.org/r/2486/diff/1/?file=37003#file37003line2203>
> >
> >     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.

Those two loops are similar; however, they compare two different things.

The first loop obtains all Party A channels in the bridge, and adds their CDRs to the container of candidates. We want to give those CDRs priority over CDRs where the channel is Party B - so we have to process all CDRs where the channel is Party A first.

The second loop does the same thing, but checks all of the Party B channels. The key here is that if we already added a CDR whose Party A is the Party B channel, we don't add that CDR. This gives us all the channels in the bridge, and a CDR record, prioritizing those CDRs where the channel is Party A.

While the loops are similar, there are two fundamental differences:
1. The things being compared are different. We could use a subfunction with a function pointer to the channel snapshot, but that doesn't feel like it makes this code easier or simpler to maintain.
2. The loops themselves are technically different, as the channel snapshot can be NULL in the second loop and is never NULL in the first.

That being said, we can collapse the logic inside the two loops into a subfunction, which will make this a bit shorter, so I'll do that.


> On May 8, 2013, 9:36 p.m., jrose wrote:
> > team/group/bridge_construction/main/cdr.c, lines 2265-2275
> > <https://reviewboard.asterisk.org/r/2486/diff/1/?file=37003#file37003line2265>
> >
> >     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.

Having multiple return points isn't always better - I'm okay with bailing if the CDR is Party A, as that is the simplest and easiest case.

The rest, where the channel we're trying to put in the bridge is Party B to some channel already in the bridge, really is a tree of logic:
 * If the candidate channel has a CDR where there are Party A:
   * If the CDR for the candidate channel has a Party B, add ourselves as a new CDR where we are Party B
   * Otherwise, if the CDR for the candidate channel has no Party B, add ourselves to the existing CDR
 * Otherwise, the candidate channel is Party B. Since a channel can only be Party B in a CDR and be a candidate if they are never a Party A in any CDR that is also in that bridge, get that channel's actual CDR (where they are the Party A)
   * If they have a CDR, add ourselves as a new CDR where we are Party B
   * If they don't have a CDR (this is basically an error), make a new CDR for them and add ourselves as Party B

This can be made simpler by deferring some of the CDR object creation to another function.


- Matt


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


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/20130516/2db2e300/attachment-0001.htm>

More information about the asterisk-dev mailing list