> On 2009-01-29 14:35:37, Mark Michelson wrote:
> > Be sure to update CHANGES.
Done
> On 2009-01-29 14:35:37, Mark Michelson wrote:
> > branches/1.4/include/asterisk/global_datastores.h, lines 29-30
> > <http://reviewboard.digium.com/r/138/diff/1/?file=2490#file2490line29>
> >
> > (Sorry, I couldn't think of a better place to add this comment)
> >
> > It looks as though you have completely removed the use of ast_dial_features from app_dial, which means you can remove the ast_dial_features datastore definitions away from global_datastores.[hc] and instead define it all in features.c instead.
Good point! Done.
> On 2009-01-29 14:35:37, Mark Michelson wrote:
> > branches/1.4/res/res_features.c, lines 1462-1477
> > <http://reviewboard.digium.com/r/138/diff/1/?file=2491#file2491line1462>
> >
> > It's a micro-optimization, but since there's such a small set of letters to actually deal with here, you could rewrite this switch statement like this:
> >
> > switch (*feature) {
> > case 't':
> > case 'T':
> > /* Code for 't' case */
> > case 'k':
> > case 'K':
> > /* etc. */
> > }
> >
> > Just one of those things to point out that you can feel free to ignore completely if you'd like. :)
I suppose--yay jump tables.
> On 2009-01-29 14:35:37, Mark Michelson wrote:
> > branches/1.4/res/res_features.c, line 1489
> > <http://reviewboard.digium.com/r/138/diff/1/?file=2491#file2491line1489>
> >
> > ds_callee_features is guaranteed to be NULL here. This will cause a crash since ast_channel_datastore_free dereferences this pointer.
Yeah, I meant to put that in the next failure case. Oops. Done.
> On 2009-01-29 14:35:37, Mark Michelson wrote:
> > branches/1.4/res/res_features.c, line 1486
> > <http://reviewboard.digium.com/r/138/diff/1/?file=2491#file2491line1486>
> >
> > Lock caller prior to calling ast_channel_datastore_find
What a good idea! That's what I get for rearranging the code two or 3 times... Done.
> On 2009-01-29 14:35:37, Mark Michelson wrote:
> > branches/1.4/res/res_features.c, line 1506
> > <http://reviewboard.digium.com/r/138/diff/1/?file=2491#file2491line1506>
> >
> > callee needs to be locked for this call to ast_channel_datastore_find.
and done
> On 2009-01-29 14:35:37, Mark Michelson wrote:
> > branches/1.4/res/res_features.c, lines 1527-1539
> > <http://reviewboard.digium.com/r/138/diff/1/?file=2491#file2491line1527>
> >
> > This error handling is a bit incomplete.
> >
> > As an example, if ds_caller_features has been allocated and added to the caller channel, then there will probably be a crash later, most likely when the channel hangs up. This is because when ast_channel_free iterates through the datastores on the channel, it will come across the freed one and then crash when accessing the invalid memory. You must always be sure to remove the datastore from a channel prior to freeing it.
reworked that. no need to do the goto at all since the two sections of code can fail independently w/o any additional problem, so now we just free the datastore if the data alloc fails in both cases (before it is added to the datastore). thanks!
- Terry
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviewboard.digium.com/r/138/#review343
-----------------------------------------------------------
On 2009-01-29 16:01:50, Terry Wilson wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.digium.com/r/138/
> -----------------------------------------------------------
>
> (Updated 2009-01-29 16:01:50)
>
>
> Review request for Asterisk Developers.
>
>
> Summary
> -------
>
> When using builtin features like parking and transfers, the AST_FEATURE_* flags would not be set correctly for all instances when either performing a builtin attended transfer, or parking a call and getting the timeout callback. Also, there was no way on a per-call basis to specify what features someone should have on picking up a parked call (since that doesn't involve the Dial() command). There was a global option for setting whether or not all users who pickup a parked call should have AST_FEATURE_REDIRECT set, but nothing for DISCONNECT, AUTOMON, or PARKCALL.
>
> This patch:
> 1) adds the BRIDGE_FEATURES dialplan variable which can be set either in the dialplan or with setvar in channels that support it. This variable can be set to any combination of 't', 'k', 'w', and 'h' (case insensitive matching of the equivalent dial options), to set what features should be activated on this channel. The patch moves the setting of the features datastores into the bridging code instead of app_dial to help facilitate this.
>
> 2) adds global options parkedcallparking, parkedcallhangup, and parkedcallrecording to be similar to the parkedcalltransfers option for globally setting features.
>
> 3) has builtin_atxfer call builtin_parkcall if being transfered to the parking extension since tracking everything through multiple masquerades, etc. is difficult and error-prone
>
> 4) attempts to fix all cases of return calls from parking and completed builtin transfers not having the correct permissions
>
>
> Diffs
> -----
>
> branches/1.4/CHANGES 172062
> branches/1.4/apps/app_dial.c 172062
> branches/1.4/configs/features.conf.sample 172062
> branches/1.4/doc/channelvariables.txt 172062
> branches/1.4/include/asterisk/global_datastores.h 172062
> branches/1.4/main/global_datastores.c 172062
> branches/1.4/res/res_features.c 172062
>
> Diff: http://reviewboard.digium.com/r/138/diff
>
>
> Testing
> -------
>
> I think I've tested the combinations of
> A calls B, (A or B) (builtin or SIP) (blind or attended) transfers (B or A) to (C or parking)
> and tested parking either timing out or being picked up.
>
> The case I used was having tT in the Dials to the extensions with an optional k or K and tested the inheritance of the parking feature.
>
> There are a lot of cases and it is certainly possible that I missed some, or typed the wrong extension and thought I was testing something that I wasn't, etc. So I would *really* appreciate it if some other people could review this and test it out before committing. Most of the stuff before hand was broken anyway, but still...
>
>
> Thanks,
>
> Terry
>
>