[asterisk-dev] [Code Review] 2578: Refactor features configuration

Mark Michelson reviewboard at asterisk.org
Mon Jun 3 09:48:19 CDT 2013 


> On June 1, 2013, 12:10 a.m., rmudgett wrote:
> > /trunk/bridges/bridge_builtin_features.c, line 64
> > <https://reviewboard.asterisk.org/r/2578/diff/2/?file=38821#file38821line64>
> >
> >     RAII_VAR is overkill here since all you want to get is digit_timeout.  Also you can defer getting the digit_timeout until you need it in the one location.

As the API is written, you don't have a choice other than to grab the xfer_config. There are not accessor functions for each individual option.

I felt that grabbing the xfer_config locally in this function was better than passing just the digit timeout into the function as a parameter because this would scale better if other transfer options became relevant in this function.


> On June 1, 2013, 12:10 a.m., rmudgett wrote:
> > /trunk/bridges/bridge_builtin_features.c, lines 313-327
> > <https://reviewboard.asterisk.org/r/2578/diff/2/?file=38821#file38821line313>
> >
> >     You should keep the local override of the features.conf DTMF settings.  Just use the configured strings in place of the hard coded ones.

When you say "local override of the features.conf DTMF settings" Are you referring to the "attended_transfer" structure from the left side of the diff, or are you referring to the hard-coded values that were present in the left side of the diff?


> On June 1, 2013, 12:10 a.m., rmudgett wrote:
> > /trunk/channels/chan_dahdi.c, line 10140
> > <https://reviewboard.asterisk.org/r/2578/diff/2/?file=38822#file38822line10140>
> >
> >     Use an under bar in the name for clarity: pickup_exten.
> >     
> >     Should do this throughout the patch: pickup_cfg->pickup_exten

I chose to purposely give the structure field the same name as the option used in features.conf even though it violates my personal taste in variable names.


> On June 1, 2013, 12:10 a.m., rmudgett wrote:
> > /trunk/channels/chan_dahdi.c, lines 10216-10219
> > <https://reviewboard.asterisk.org/r/2578/diff/2/?file=38822#file38822line10216>
> >
> >     Is it necessary to create a local copy of the pickup_exten string when you are keeping a reference to pickup_cfg?

Ah yes, the local copy thing.

When ast_get_chan_features_XXX_config or ast_get_chan_featuremap_config() is called, the reference returned is pointing to the data on a channel's datastore. While we don't have to worry about this data being freed while we have a reference, we do have to worry about the integrity of that data once the channel is unlocked. At any time, someone could issue an AMI SetVar command to set, say, FEATURE(pickupexten) on a channel. This data must be protected by the channel's lock. This is why the data is copied out of the returned configuration before the channel is unlocked.

I explored alternatives.

1) Swap the channel's datastore data every time the data changes. In other words, if someone were to use the FEATURE() or FEATUREMAP() dialplan function to set data, a new configuration would be allocated and would be swapped in for the old channel datastore data. The issue here is that if someone were to opt to set all feature information in their dialplan, then the consecutive calls to FEATURE() or FEATUREMAP() would result in a large number of allocations and swaps. This made the idea unattractive.

2) Get a copy of the channel's datastore data every time we want access to it. Again, the number of allocations seemed unwieldy here given how often the channel's datastore would be accessed (pickupexten, for instance, is looked up a LOT by channel drivers).

Given the two alternatives, I thought that making local copies of what was necessary while keeping the channel locked was the best way to go.


> On June 1, 2013, 12:10 a.m., rmudgett wrote:
> > /trunk/channels/chan_sip.c, lines 21738-21739
> > <https://reviewboard.asterisk.org/r/2578/diff/2/?file=38825#file38825line21738>
> >
> >     Missed removing ast_unlock_call_features() here.

It looks like I had chan_sip unselected in menuselect entirely. I'll get it fixed up in the next diff.


> On June 1, 2013, 12:10 a.m., rmudgett wrote:
> > /trunk/main/features.c, line 7149
> > <https://reviewboard.asterisk.org/r/2578/diff/2/?file=38834#file38834line7149>
> >
> >     pickup_cfg should only be gotten from chan not target.  It is chan that is doing the pickup not target.

See my comment further below. I got a bit mixed up on which channel the prompt was played to.


> On June 1, 2013, 12:10 a.m., rmudgett wrote:
> > /trunk/main/features_config.c, line 51
> > <https://reviewboard.asterisk.org/r/2578/diff/2/?file=38835#file38835line51>
> >
> >     This sound is played to the picker not the caller.

Ah, I see where I went wrong here. The BRIDGE_PLAY_SOUND variable is set on 'target' in ast_pickup_call(). This is assuming that 'target' will be masqueraded into by the picker so that when the bridge is established, the picker will hear the pickup sound.


> On June 1, 2013, 12:10 a.m., rmudgett wrote:
> > /trunk/main/features_config.c, lines 341-349
> > <https://reviewboard.asterisk.org/r/2578/diff/2/?file=38835#file38835line341>
> >
> >     Aren't you supposed to also parse the features.conf [parkinglot_*] sections?

That wasn't part of my task. Parking lot configuration currently lives in res_parking.conf. Parking lot configuration could be migrated back over to features.conf and mixed in with the rest of the features.conf configuration with some effort though.


> On June 1, 2013, 12:10 a.m., rmudgett wrote:
> > /trunk/main/features_config.c, lines 528-530
> > <https://reviewboard.asterisk.org/r/2578/diff/2/?file=38835#file38835line528>
> >
> >     Should featuregroups also be copied?
> >     
> >     Further analysis shows that you do not need to setup applicationmap in this copy nor do you need to setup featuregroups.

Yep, I had made a mental note to remove the applicationmap reference from this function, but then I didn't go back and do it. It didn't cause harm, but it is useless as you pointed out.


> On June 1, 2013, 12:10 a.m., rmudgett wrote:
> > /trunk/main/features_config.c, lines 1058-1063
> > <https://reviewboard.asterisk.org/r/2578/diff/2/?file=38835#file38835line1058>
> >
> >     You are not parsing things from the app field correctly.  The new format will put the moh_class in the app_data position because parentheses are used instead of a comma.
> >     
> >     There are also two old formats documented as well:
> >     foo = *1,self,NoOp,"Boo!",default
> >     foo = *1,self,NoOp,Boo!,default
> >     The features.c:process_applicationmap_line() did not seem to handle the quoted app_data either.
> >     
> >     The self/peer activated on values have backward compatible alternates caller/callee respectively.

I don't understand your second sentence.

When the new syntax is used, the string is initially parsed as

foo = *1,    self,         NoOp(Boo!),  default
^name ^DTMF  ^active_on    ^app         ^app_data       moh_class = NULL

When we detect the parentheses in the app name, we then perform a shift like so:

foo = *1,    self,         NoOp  (Boo!),    default
^name ^DTMF  ^active_on    ^app   ^app_data ^moh_class

The shift is being performed correctly, so I don't understand what the complaint is.

The rest of your comment is understood and appropriately acknowledged, though :)


- Mark


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


On May 30, 2013, 6:46 p.m., Mark Michelson wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/2578/
> -----------------------------------------------------------
> 
> (Updated May 30, 2013, 6:46 p.m.)
> 
> 
> Review request for Asterisk Developers, jrose and rmudgett.
> 
> 
> Bugs: ASTERISK-21542
>     https://issues.asterisk.org/jira/browse/ASTERISK-21542
> 
> 
> Repository: Asterisk
> 
> 
> Description
> -------
> 
> This change separates features configuration out of features.c into a separate features_config.c file. In addition, a features_config.h file has been added as a means of getting configured values. The configuration changes are as follows:
> 
> * Configuration for features uses the config framework now.
> * The FEATURE and FEATUREMAP functions use the same handlers as the configuration. Therefore, any configuration item available in the config file is also available for the FEATURE and FEATUREMAP functions
> * Because the FEATURE and FEATUREMAP functions are more complete, feature configuration retrieval is done per channel rather than globally. Global configuration can be retrieved, but this is a rare occurrence.
> * The CLI command to show feature configuration has been moved into the features_config.c file as well
> * Three new options for attended transfers were added: atxfercomplete, atxferabort, and atxferthreeway. Now the key combinations for completing, aborting, and conferencing an attended transfer are no longer hard coded.
> 
> Since feature configuration is handled differently than it used to be, it meant that several items from features.h and features.c could be removed altogether:
> 
> * All functions having to do with configuration parsing have been removed from features.c. This includes functions that were used to modify the dialplan due to parking options being parsed.
> * Functions that retrieved call features have been removed. This includes the removal of ast_call_feature, its associated locks, the builtin_features array, and functions used to find dynamic features.
> 
> The result of removing some of the above meant that there was quite a bit of dead code in features.c. However, I did not remove this code and instead opted to comment it out for several reasons. The following bits have been commented out rather than being removed.
> 
> * All builtin feature operations in features.c have been commented out. These items had already been "dead" for a while, but now that nothing references them, they had to be commented out in order to compile with devmode enabled. I left these commented out since there are currently tasks to make the builtin features feature-complete. Leaving the old code around as an example is not a bad thing.
> * The unit tests in features.c have been commented out. The new parking code currently does not have unit tests, so keeping the old unit tests around but commented out allows for them to be used as a guide when writing new tests.
> 
> There was a tangential change as well: an AST_FEATURES_DTMF_MASK constant was added that can be used to tell if a builtin feature requires DTMF. This was necessary for setting up bridge features since the ast_call_feature structure was removed.
> 
> Things I'm looking for in this review:
> 
> * Are there items in features.c that I have commented out that should just plain be removed instead?
> * Are there any items in features.c that I removed that should not have been?
> 
> 
> Diffs
> -----
> 
>   /trunk/apps/app_dial.c 390144 
>   /trunk/bridges/bridge_builtin_features.c 390144 
>   /trunk/channels/chan_dahdi.c 390144 
>   /trunk/channels/chan_mgcp.c 390144 
>   /trunk/channels/chan_misdn.c 390144 
>   /trunk/channels/chan_sip.c 390144 
>   /trunk/channels/chan_unistim.c 390144 
>   /trunk/channels/sig_analog.c 390144 
>   /trunk/channels/sip/include/sip.h 390144 
>   /trunk/include/asterisk/bridging_features.h 390144 
>   /trunk/include/asterisk/channel.h 390144 
>   /trunk/include/asterisk/features.h 390144 
>   /trunk/include/asterisk/features_config.h PRE-CREATION 
>   /trunk/main/bridging.c 390144 
>   /trunk/main/features.c 390144 
>   /trunk/main/features_config.c PRE-CREATION 
>   /trunk/main/manager.c 390144 
> 
> Diff: https://reviewboard.asterisk.org/r/2578/diff/
> 
> 
> Testing
> -------
> 
> Tested configuration parsing. Ensured that general features, featuremap items, applicationmap items, and featuregroups were set and retrievable properly.
> 
> Tested that the FEATURE and FEATUREMAP dialplan functions overrode the global configuration in features.conf
> 
> Tested that items in the featuremap and applicationmap could be triggered mid-call. This includes the new attended transfer options.
> 
> 
> Thanks,
> 
> Mark Michelson
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20130603/cad62f59/attachment-0001.htm>

More information about the asterisk-dev mailing list