[Home]

Summary:ASTERISK-03675: [PATCH] adds support for applications starting as feature
Reporter:crich (crich)Labels:
Date Opened:2005-03-13 07:43:18.000-0600Date Closed:2008-01-15 15:45:05.000-0600
Priority:MajorRegression?No
Status:Closed/CompleteComponents:Core/NewFeature
Versions:Frequency of
Occurrence
Related
Issues:
Environment:Attachments:( 0) res_features.patch
( 1) res_features.patch.2.txt
( 2) res_features.patch.3.txt
( 3) res_features.patch.txt
Description:This patch adds an section: [applicationmap] to features.conf.

In this Section an user can define a key-sequence, and an
application and the party on which this application is executed when the sequence is pressed.

The Syntax is:

[applicationmap]
<featurename> => <keysequence>,<caller|callee>,<App> [,App Args]


This is implemented by adding support for dynamic feature adding. So we have now 2 new Functions:

ast_register_feature(struct ast_call_feature *p);
ast_unregister_feature(struct ast_call_feature *p);

which explain theirself fairly well i think.

The ast_call_feature struct is now moved to features.h and contains 3 new Memenbers:

char *app;
char *app_arg;
char *party;

which explain theirself as good as the reg/unreg functions i think.

As You see this patch adds also the ability for apps and chan_drivers to register dynamic features.

Hope this is useful to anyone.

Note: You can use the Goto App also to jump into the extensions conf ;)


****** ADDITIONAL INFORMATION ******

Disclaimer is on file

Diff Cmd:  
cvs diff -u configs/features.conf.sample include/asterisk/features.h res/res_features.c
Comments:By: Olle Johansson (oej) 2005-03-13 09:29:13.000-0600

Please add doxygen docs to features.h so we can generate proper docs for these new exciting functions.

By: crich (crich) 2005-03-13 15:28:41.000-0600

/*! Blah Blah */

was that doxygen like ?

By: crich (crich) 2005-03-13 15:37:30.000-0600

Now with doxygen style comments ;)

By: k3v (k3v) 2005-03-13 16:18:24.000-0600

how about <caller|callee|both> ?

By: Olle Johansson (oej) 2005-03-13 17:55:25.000-0600

Thank you for that documentation fix, crich!

By: crich (crich) 2005-03-14 00:56:03.000-0600

Nice Idea k3v, but would need an additional Thread which creates a lot of overhead ..

By: crich (crich) 2005-03-14 15:33:21.000-0600

What must be done to get this stuff into cvs-head ?

By: Brian West (bkw918) 2005-03-14 23:09:33.000-0600

bug mark like crazy!!!

/b

By: crich (crich) 2005-03-15 13:49:17.000-0600

Doens't he has a watch to this mantis? how do i bug mark like crazy ?

By: Matt O'Gorman (mogorman) 2005-03-15 13:58:38.000-0600

it does not appear as such, but i have a feeling he might look at it soon...

By: Mark Spencer (markster) 2005-03-15 14:21:54.000-0600

Looks interesting, but several things need to be addressed:

1) Formatting seems to be in error.  It should use tabs, not spaces...

2) No // style comments

3) ast_verbose comments must be qualified properly with what level of option_verbose is enabled (see other code using ast_verbose)

4) having dynamically loadable features implies having locks around that kind of feature handling.

5) I don't know about strtok_r, but strsep allows the pointer with current position to point to NULL, thus eliminating much of that error checking, so you can just use a = strsep(",", &foo); b = strsep(",", &foo"); without checking if foo is NULL.

6) This patch doesn't compile since the "party" field is absent.  In any case, it's conceptually a boolean, not a string, and as such should be represented as a flag.

7) This patch is extremely malloc (or strdup) intensive and leaks memory when features are removed.  There should be little or no malloc()'s required.

8) Presumably all listed features are available in all calls.  I'm not sure this is the most effective way this could be done, but making it more configurable or selectable within the dialplan could present a challenge as well.

Perhaps discussion with others on IRC (e.g. anthm, kpfleming or myself, kram) would be helpful.

By: crich (crich) 2005-03-16 08:11:34.000-0600

Hi Mark,

1) Fixed now ;) I use emacs + my own indenting style .. no it's vi indented

2) //comments are gone

3) added the if (options_verboes> x)

4) added locks for changing and traversing the features list

5) hm .. never used strsep before, saw it here and there in asterisk source,
strtok_r is simply a reentrant strtok (thread safe).

6) Fixed, its now a flag of the features object

7) removed every malloc/strdup, therefore i needed to change the ast_call_feature element char *sname to char sname[32]. Only the malloc to create dynamic features from features.conf is still there, i think theres no other way ..

8) I think so too, but i dislike this Channel Variable to Feature Flag Mapping. I'd prefer a dial parameter, but hum .. not sure, I just added a Flag AST_BRIDGE_DYNAMIC_FEATURES, which could be a bridge_config flag ?

I could be at irc tonight(german night) I'm crich1999

By: crich (crich) 2005-03-17 02:48:40.000-0600

Again Point 8) from Mark.

What about the Idea to have a Channel Variable called:

FEATURE_DETECT_DYNAMIC

when this is set to somethin (true), we could set the AST_FEATURE_DYNAMIC_DETECT Flag either in the bridge_config or the caller channel.

By: Brian West (bkw918) 2005-03-17 20:52:26.000-0600

We did bring this up today in the meeting.

/b

By: crich (crich) 2005-03-18 07:58:06.000-0600

Have you decided something ?

By: crich (crich) 2005-03-23 11:24:44.000-0600

I just added, that you can only make use of dynamic features when they are set in the caller Channel Variable "DYNAMIC_FEATURES" e.g. :

exten _X.,1,SetVar(DYNAMIC_FEATURES=feauter1,feature2)
..

Only if this variable is set a feature lookup is made and if it is set we check also if the requestet feature is allowed through this variable.

This makes it possible to allow and disallow features per Call.

By: crich (crich) 2005-04-01 09:08:43.000-0600

Let me resume our DevCall Meeting:

1. Why not using Transfer to jump into extensions.conf
2. Why not having Features Flags, which are settable in Users Sections

Transfer is not really the same. Lets say There is a call between 2 people, one could make a blind transfer to an application, this would cancel the connection. An attended Transfer works only against the other party. This patch offers the possibility to start any application either on the caller or on the callee, when the application finishes the call goes on.

Having predefined Feature Flags would limit the possibilities of this patch. Also there is no real user concept in the asterisk at the moment.

This Patch aims especially the problem of a channel driver which has special options.  
In case of zaptel it could be the echo cancel enabling and disabling.
In other channel drivers like mISDN we could enable/disable crypting, changing the volume, enable/disable echo-cancel and enable disable dtmf detection during a call.

A call center Agent could Change the field of the CDR depending on the call "value".

I think there are lots of possibilities which i can't all imagine by this time. I would like to use this espacially for chan_misdn.

Without this patch i needed to implement a feature interpreter directly into chan_misdn, which is not really a nice solution.

By: Andreas Anderson (aanderson) 2005-04-05 06:10:14

*BUMP*

Is there a "vote for this bug" button somewhere? This is a really cool idea, how do we get this into CVS? "bug mark like crazy" is not really nice :-D

Turn on encryption, change codec, mark a call in cdr, buzz the pizzaguy in, the list is endless what could be done with this...

By: crich (crich) 2005-04-05 09:28:26

Nice Idea, what about a feature voting system ?! ;) that would be nice to have eh.

By: Michael Jerris (mikej) 2005-05-02 09:50:10

Is this up to date?  Can I delete all but the newest patch?

By: Michael Jerris (mikej) 2005-05-02 09:52:16

Kevin, can you please review this.  It appears all of the open issues have been atleast commented on if not fully addressed.

By: crich (crich) 2005-05-02 10:05:31

I think you can delete the older Patches. I'd still like to get this into CVS, but I don't know what to do more.

By: Mark Spencer (markster) 2005-05-09 10:11:36

I definitely want some serious code review on this.  That modifications to the parsing/loading of these functions looks fishy to me and there appear to be some formatting issues.  I'll sign off on the concept architecturally at least.

By: Kevin P. Fleming (kpfleming) 2005-05-15 00:34:01

Notes:

- Here we go with yet another open-coded linked list implementation (with locking, even). (No, that's not your fault crich, it's just common practice). Please use the macros from linkedlists.h for managing the ast_call_feature lists.

- strsep is a big improvement over strtok_r in readability, even though it's marginally less portable. There is no thread-safety issue, since strsep does not use static buffers.

- You call memset() on a newly-allocated feature, even if malloc() fails.

- Use ast_copy_string() (which is new) instead of strncpy, or ensure that the length passed to strncpy() is one less than the buffer length.

- Something funky is going on here with the formatting (or even the logic, in fact this block seems to be partially duplicated):

+ if (!strcasecmp(party,"caller"))
+ ast_set_flag(feature,AST_FEATURE_FLAG_CALLER);
+ else
+ ast_set_flag(feature,AST_FEATURE_FLAG_CALLEE);
+ if (app_args) strncpy(feature->app_args,app_args,FEATURE_APP_ARGS_LEN);
+ strcpy(feature->exten, exten);
+ feature->operation=feature_exec_app;
+ ast_set_flag(feature,AST_FEATURE_FLAG_NEEDSDTMF);
+ ast_register_feature(feature);
+ } else {
+ strncpy(feature->sname,var->name,FEATURE_SNAME_LEN);
+ ast_clear_flag(feature, AST_FEATURE_FLAG_CALLER);

By: crich (crich) 2005-05-15 15:43:14

Thanks Mark and kpflemming, i'll review this patch next week.

By: Olle Johansson (oej) 2005-06-05 17:23:27

A very long week indeed. Any updates :-) ?

/Housekeeping

By: crich (crich) 2005-06-15 11:16:37

hehe yepp, its been a really long week, maybe we can fix this on friday (in madrid ;)

By: crich (crich) 2005-06-19 11:03:59

I reviewed the parsing stuff and tried to correct Kevins mentioned issues. Because

Set(DYNAMIC_FEATURES=x,y,) doesn't work anymore I use # as delim now so you need to do :

Set(DYNAMIC_FEATURES=hangup#play)

to enable those apps for the actual channel.

By: Kevin P. Fleming (kpfleming) 2005-06-21 15:54:37

Code review notes:

- is there any reason why 'struct ast_call_feature' is now in features.h? does any module outside of res_features.c need to be able to see inside one?

- even though the number of 'struct ast_call_feature's that will be allocated is small, I'm not too keen on hardcoded size limits for the various parts of the structure. since it's read-only (the names/apps/etc. cannot change), how about using the "allocate the strings after the structure in memory" trick so that you can both only allocate the size that is needed, and also not need any artificial size restrictions?

- there is no need to initialize variables to NULL/zero when the first use of them is going to assign a value anyway (and in fact if the compiler is not smart enough to optimize it away, it will generate useless object code to do the initializations)

- multiple calls to strsep() do not need to be conditionalized based on the result of the previous one; if the string pointer passed into strsep() is NULL it immediately returns NULL

- the config parser checks that one of app/exten/party are provided, but does not check them to ensure they are not zero length

- there is no need to copy a "" string into a variable buffer that you just set to all zeroes using memset()

- the feature is registered before setting the CALLER/CALLEE flags; is there any chance it will be used before the flag has been set?

- "ReMapping Feature" should be "Mapping Feature"

By: Michael Jerris (mikej) 2005-07-12 17:19:50

3 weeks no response, suspending. .crich, I really like this feature, can you open it back up when you have updates ready please.

By: crich (crich) 2005-08-09 12:17:51

new Version. I've fixed the mentioned issues.

regarding your first remark:

As mentioned above the patch allows other modules to register features dynamically, so of course those modules need to know about the ast_call_feature structure.

regarding your second remark:

do you think the work to make this alloc magic is it worth to be implemented, since the fixed length of the strings is at "using time" filled with only this fixed length ?

By: crich (crich) 2005-08-10 16:23:35

* bump *

By: crich (crich) 2005-08-11 11:27:21

Kevin: I'd like you to have a look at this again. Thanks!

By: crich (crich) 2005-08-18 03:56:23

* bump *

(I'm getting it back hm?)

By: Kevin P. Fleming (kpfleming) 2005-08-22 22:21:08

Committed to CVS HEAD, but it needs work... at least when reload() is called, the features will be added to the list again. Please get a new patch ready ASAP :-)

By: crich (crich) 2005-08-23 05:47:11

new patch ready

By: Kevin P. Fleming (kpfleming) 2005-08-23 10:38:13

Committed to CVS HEAD with minor mods, thanks!

By: Digium Subversion (svnbot) 2008-01-15 15:45:02.000-0600

Repository: asterisk
Revision: 6374

U   trunk/configs/features.conf.sample
U   trunk/include/asterisk/channel.h
U   trunk/include/asterisk/features.h
U   trunk/res/res_features.c

------------------------------------------------------------------------
r6374 | kpfleming | 2008-01-15 15:45:02 -0600 (Tue, 15 Jan 2008) | 2 lines

add ability to map feature sequences to applications (issue ASTERISK-3675)

------------------------------------------------------------------------

http://svn.digium.com/view/asterisk?view=rev&revision=6374

By: Digium Subversion (svnbot) 2008-01-15 15:45:05.000-0600

Repository: asterisk
Revision: 6377

U   trunk/res/res_features.c

------------------------------------------------------------------------
r6377 | kpfleming | 2008-01-15 15:45:04 -0600 (Tue, 15 Jan 2008) | 2 lines

ensure that features are not duplicated during reload (issue ASTERISK-3675, take two)

------------------------------------------------------------------------

http://svn.digium.com/view/asterisk?view=rev&revision=6377