[asterisk-dev] [Code Review] Asterisk Unique ID for call logging Phase II
Mark Michelson
reviewboard at asterisk.org
Thu Mar 29 12:39:59 CDT 2012
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/1823/#review5908
-----------------------------------------------------------
The only thing left that worries me in this review is the changes to bridging.c
I've taken a more extensive look into bridging, and here are some of the worries I have:
1. Multiple channels joining a bridge may have a PBX running. Consider app_confbridge, which uses the code in bridging.c. For the most part, this works by having multiple callers place calls to extensions that run the ConfBridge() app. Each channel associated with these callers will be running a PBX. Now, the threading model for bridging is ... interesting. So what you'll end up with here is:
* The dedicated bridge thread will inherit the callid of the first call that joins.
* Individual threads writing frames to the bridge and having frames written to them from the bridge will still have the callid allocated to them when their PBX thread was created.
This could be a bit awkward.
Based on this, I think it's a good idea to remove the callid inheritance from a bridge. This way, the logging will end up more accurate than before. You may opt to allocate a separate callid for bridge threads just so that logging pertaining to the bridge itself can be logged under a specific ID, but I'm unsure if that's actually useful.
2. Not all channels joining a bridge will have a PBX running. In theory, a caller channel could place calls out to 10 channels and them use ast_bridge_impart() to put them all into a ConfBridge. The result, as you have it right now will be that the caller's thread, the bridge thread, and all of the threads created from imparting channels will have the same callid. This can potentially make sense since there was only one call placed into Asterisk. However, it may come across as inconsistent since in the more typical ConfBridge case, each individual channel thread would have a different callid. Another possible situation is to have two callers each place outbound calls to 10 channels and then impart all the outbound channels into the one confbridge. In this case, the bridge will have the callid of the first caller to enter the bridge, half of the imparted channels will have the callid of caller 1 and the other half of the imparted channels will have the callid of caller 2. It's worth mentioning that there's no existing code path in Asterisk to actually do this, but the APIs exist.
3. bridge_multiplexed is a bridge technology that has 1-to-1 bridges all occurring in the same thread. As it is right now, this isn't too much of an issue because you haven't actually modified bridge_multiplexed's thread creation to inherit a callid from channels joining the bridge. It's also not an issue because I don't think anything actually makes use of this bridge technology at the moment. However, the situation needs to be kept in mind for this.
So from all of this, I think the real meat of what I'm saying here is that bridging is complicated, and so having all callids get inherited like you have is not going to work. I think that callids in bridging should be saved for phase 3.
/trunk/include/asterisk/logger.h
<https://reviewboard.asterisk.org/r/1823/#comment10834>
s/revtal/retval/
/trunk/main/features.c
<https://reviewboard.asterisk.org/r/1823/#comment10844>
While it should have been done in the first place, it's now even more important that the value of ast_pthread_create() is checked. This way you can unref the callid if thread creation fails. Otherwise you have a reference leak.
- Mark
On March 27, 2012, 11:13 a.m., jrose wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/1823/
> -----------------------------------------------------------
>
> (Updated March 27, 2012, 11:13 a.m.)
>
>
> Review request for Asterisk Developers, Mark Michelson, junky, and Matt Jordan.
>
>
> Summary
> -------
>
> https://wiki.asterisk.org/wiki/display/AST/Unique+Call-ID+Logging
>
> Since none of this patch has appeared on the public reviewboard yet, I'll go ahead and fill everyone in on phase I's objectives which have already been met.
> Phase I
> Implement <call-id> structure and factory. Bind <call-id> pointers to newly created channel threads.
> Modify logging to read thread storage so that channel thread log messages include the <call-id> stamp.
>
>
> As for the current objective...
> Phase II
> 1. Spread references to ast_callid structs to threads created by a pbx/channel thread already bound to a <call-id>.
> 2. Add CLI command to display <call-id> with verbose messages on CLI.
>
> The primary focus of this review is a sanity check for some of the thread associations that are being worked on for this feature and also to get some community feedback on the feature in general since it hasn't seen a great deal of that yet.
>
> What I'm looking to learn from this review is if I missed any obvious places where this callid should be passed to a new thread and if the various places that I am passing it around right now are all appropriate. It could also be that there is a better approach to handling this passing of callids that I'm just not aware of right now.
>
> This code can be found in the following repository:
> https://origsvn.digium.com/svn/asterisk/team/jrose/call_identifiers
>
>
> Diffs
> -----
>
> /trunk/main/pbx.c 360623
> /trunk/main/features.c 360623
> /trunk/main/logger.c 360623
> /trunk/include/asterisk/logger.h 360623
> /trunk/main/bridging.c 360623
> /trunk/main/dial.c 360623
> /trunk/apps/app_mixmonitor.c 360623
> /trunk/configs/logger.conf.sample 360623
> /trunk/include/asterisk/bridging.h 360623
> /trunk/CHANGES 360623
>
> Diff: https://reviewboard.asterisk.org/r/1823/diff
>
>
> Testing
> -------
>
> Honestly, I'm not sure if all of these thread associations are appropriate or not. The one I understand best right now is mixmonitor, which handles thread associations like you would expect... it inherits them from the parent thread if the parent thread is associated with a call-id. In other words, if it's activated from the dialplan, it will be a part of the call. If it's activated from AMI or CLI, then the mixmonitor won't have a callid associated with it. This doesn't necessarily seem inappropriate, though it wouldn't necessarily be inappropriate to go ahead and bind mixmonitor to the callid when created in those ways either... but nothing can be done about that until the additon to handle callids in channels goes through in phase III.
>
> For a little demonstration of this feature in action, the following is a call where the following happens:
> 1. SIP/gold dials an extension which mixmonitors a dial to SIP/101
> 2. SIP/gold holds on SIP/101 and then unholds
> 3. SIP/gold blind transfers SIP/101 to SIP/123
> 4. SIP/101 hangs up the line
>
> [2012-03-2015:01:30] VERBOSE[28696] netsock2.c: == Using SIP RTP CoS mark 5
> [2012-03-2015:01:30] DEBUG[28729] logger.c: CALL_ID [C000000] created by thread.
> [2012-03-2015:01:30] DEBUG[28729][C000000] logger.c: CALL_ID [C000000] bound to thread.
> [2012-03-2015:01:30] VERBOSE[28729][C000000] pbx.c: -- Executing [006 at sipphones:1] MixMonitor("SIP/gold-00000000", "recordingx.wav,m(1111 at default)") in new stack
> [2012-03-2015:01:30] VERBOSE[28729][C000000] pbx.c: -- Executing [006 at sipphones:2] Dial("SIP/gold-00000000", "SIP/101") in new stack
> [2012-03-2015:01:30] DEBUG[28730][C000000] logger.c: CALL_ID [C000000] bound to thread.
> [2012-03-2015:01:30] VERBOSE[28730][C000000] app_mixmonitor.c: == Begin MixMonitor Recording SIP/gold-00000000
> [2012-03-2015:01:30] VERBOSE[28729][C000000] netsock2.c: == Using SIP RTP CoS mark 5
> [2012-03-2015:01:30] VERBOSE[28729][C000000] app_dial.c: -- Called SIP/101
> [2012-03-2015:01:30] VERBOSE[28729][C000000] app_dial.c: -- SIP/101-00000001 is ringing
> [2012-03-2015:01:33] VERBOSE[28729][C000000] app_dial.c: -- SIP/101-00000001 answered SIP/gold-00000000
> [2012-03-2015:02:17] VERBOSE[28729][C000000] res_musiconhold.c: -- Started music on hold, class 'default', on SIP/101-00000001
> [2012-03-2015:02:22] VERBOSE[28729][C000000] res_musiconhold.c: -- Stopped music on hold on SIP/101-00000001
> [2012-03-2015:02:22] DEBUG[28733] logger.c: CALL_ID [C000001] created by thread.
> [2012-03-2015:02:22] DEBUG[28733][C000001] logger.c: CALL_ID [C000001] bound to thread.
> [2012-03-2015:02:22] VERBOSE[28733][C000001] pbx.c: -- Executing [123*@sipphones:1] Dial("SIP/101-00000001", "SIP/123") in new stack
> [2012-03-2015:02:22] VERBOSE[28733][C000001] netsock2.c: == Using SIP RTP CoS mark 5
> [2012-03-2015:02:22] VERBOSE[28733][C000001] app_dial.c: -- Called SIP/123
> [2012-03-2015:02:22] NOTICE[28696] chan_sip.c: Got OK on REFER Notify message
> [2012-03-2015:02:22] VERBOSE[28729][C000000] pbx.c: == Spawn extension (sipphones, 006, 2) exited non-zero on 'SIP/gold-00000000'
> [2012-03-2015:02:22] VERBOSE[28730][C000000] app_mixmonitor.c: == MixMonitor close filestream (mixed)
> [2012-03-2015:02:22] VERBOSE[28730][C000000] app_mixmonitor.c: == End MixMonitor Recording SIP/gold-00000000
> [2012-03-2015:02:22] VERBOSE[28733][C000001] app_dial.c: -- SIP/123-00000002 is ringing
> [2012-03-2015:02:23] VERBOSE[28733][C000001] app_dial.c: -- SIP/123-00000002 answered SIP/101-00000001
> [2012-03-2015:02:23] VERBOSE[28733][C000001] rtp_engine.c: -- Locally bridging SIP/101-00000001 and SIP/123-00000002
> [2012-03-2015:02:35] VERBOSE[28733][C000001] pbx.c: == Spawn extension (sipphones, 123*, 1) exited non-zero on 'SIP/101-00000001'
>
> I'm still working on test cases for the other associations I'm performing.
>
>
> Thanks,
>
> jrose
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20120329/0732e3f4/attachment-0001.htm>
More information about the asterisk-dev
mailing list