[asterisk-dev] murf: trunk r172890 - in /trunk: ./ apps/ include/asterisk/ main/

[Russell Bryant]

SVN commits to the Digium repositories wrote:
> Author: murf
> Date: Mon Feb  2 11:37:15 2009
> New Revision: 172890
> 
> URL: http://svn.digium.com/svn-view/asterisk?view=rev&rev=172890
> Log:
> This change allows the disconnect feature (as in "one-touch" in features.c)
> to be used within the dial app, before a call is bridged.

> Modified: trunk/apps/app_dial.c
> URL: http://svn.digium.com/svn-view/asterisk/trunk/apps/app_dial.c?view=diff&rev=172890&r1=172889&r2=172890
> ==============================================================================
> --- trunk/apps/app_dial.c (original)
> +++ trunk/apps/app_dial.c Mon Feb  2 11:37:15 2009
> @@ -560,6 +560,7 @@
>  	uint64_t flags;
>  };
>  
> +static int detect_disconnect(struct ast_channel *chan, char code);
>  
>  static void hanguptree(struct chanlist *outgoing, struct ast_channel *exception, int answered_elsewhere)
>  {
> @@ -1054,8 +1055,8 @@
>  				}
>  
>  				if (ast_test_flag64(peerflags, OPT_CALLER_HANGUP) &&
> -						(f->subclass == '*')) { /* hmm it it not guaranteed to be '*' anymore. */
> -					ast_verb(3, "User hit %c to disconnect call.\n", f->subclass);
> +						detect_disconnect(in, f->subclass)) {
> +					ast_verb(3, "User requested call disconnect.\n");
>  					*to = 0;
>  					strcpy(pa->status, "CANCEL");
>  					ast_cdr_noanswer(in->cdr);
> @@ -1098,6 +1099,58 @@
>  
>  	return peer;
>  }
> +
> +static char featurecode[FEATURE_MAX_LEN + 1] = "";

This is totally broken.  This global buffer is used for call-specific 
data for every call.

> +
> +static int detect_disconnect(struct ast_channel *chan, char code)
> +{
> +	struct feature_interpret_result result;
> +	int x;
> +	struct ast_flags features;
> +	int res = FEATURE_RETURN_PASSDIGITS;
> +	struct ast_call_feature *feature;
> +	char *cptr;
> +	const char *dynamic_features = pbx_builtin_getvar_helper(chan, "DYNAMIC_FEATURES");

This is not thread-safe.  You have to hold the channel lock around the 
entire time that you hold a pointer to the result of this function to 
ensure that it stays valid.

> +	int len;
> +
> +	len = strlen(featurecode);
> +	if (len >= FEATURE_MAX_LEN) {
> +		featurecode[0] = '\0';
> +	}
> +	cptr = &featurecode[strlen(featurecode)];
> +	cptr[0] = code;
> +	cptr[1] = '\0';
> +
> +	memset(&features, 0, sizeof(struct ast_flags));
> +	ast_set_flag(&features, AST_FEATURE_DISCONNECT);

You can just initialize features this way at declaration time ...



> Modified: trunk/include/asterisk/features.h
> URL: http://svn.digium.com/svn-view/asterisk/trunk/include/asterisk/features.h?view=diff&rev=172890&r1=172889&r2=172890
> ==============================================================================
> --- trunk/include/asterisk/features.h (original)
> +++ trunk/include/asterisk/features.h Mon Feb  2 11:37:15 2009
> @@ -36,6 +36,15 @@
>  
>  #define PARK_APP_NAME "Park"
>  
> +#define FEATURE_RETURN_HANGUP		-1
> +#define FEATURE_RETURN_SUCCESSBREAK	 0
> +#define FEATURE_RETURN_PASSDIGITS	 21
> +#define FEATURE_RETURN_STOREDIGITS	 22
> +#define FEATURE_RETURN_SUCCESS	 	 23
> +#define FEATURE_RETURN_KEEPTRYING    24
> +
> +typedef int (*feature_operation)(struct ast_channel *chan, struct ast_channel *peer, struct ast_bridge_config *config, char *code, int sense, void *data);
> +

Public API entries must begin with an ast_ prefix.

Also, why did you need to add these here even though this header already 
had the AST_FEATURE_ versions quoted below?

> -#define AST_FEATURE_RETURN_HANGUP                   -1
> -#define AST_FEATURE_RETURN_SUCCESSBREAK             0
> -#define AST_FEATURE_RETURN_PASSDIGITS               21
> -#define AST_FEATURE_RETURN_STOREDIGITS              22
> -#define AST_FEATURE_RETURN_SUCCESS                  23
> -#define AST_FEATURE_RETURN_KEEPTRYING               24
> +#define AST_FEATURE_RETURN_HANGUP                   FEATURE_RETURN_HANGUP
> +#define AST_FEATURE_RETURN_SUCCESSBREAK             FEATURE_RETURN_SUCCESSBREAK
> +#define AST_FEATURE_RETURN_PASSDIGITS               FEATURE_RETURN_PASSDIGITS
> +#define AST_FEATURE_RETURN_STOREDIGITS              FEATURE_RETURN_STOREDIGITS
> +#define AST_FEATURE_RETURN_SUCCESS                  FEATURE_RETURN_SUCCESS
> +#define AST_FEATURE_RETURN_KEEPTRYING               FEATURE_RETURN_KEEPTRYING


> +
> +struct feature_interpret_result {
> +    struct ast_call_feature *builtin_feature;
> +    struct ast_call_feature *dynamic_features[20];
> +    struct ast_call_feature *group_features[20];
> +    int num_dyn_features;
> +    int num_grp_features;
> +};

Again, an ast_ prefix is required here.

> +int ast_feature_detect(struct ast_channel *chan, const struct ast_flags *features, char *code, struct feature_interpret_result *result, const char *dynamic_features);
> +
> +void ast_features_lock(void);
> +void ast_features_unlock(void);

Please include documentation with every new API call introduced.

> +			result->dynamic_features[result->num_dyn_features++] = feature;
> +			if (result->num_dyn_features >= (sizeof(result->dynamic_features) / sizeof(result->dynamic_features[0]))) {

Use ARRAY_LEN() here.

> +	if (sense == FEATURE_SENSE_CHAN) {
> +		ast_copy_flags(&features, &(config->features_caller), AST_FLAGS_ALL);
> +		dynamic_features = pbx_builtin_getvar_helper(chan, "DYNAMIC_FEATURES");
> +	} else {
> +		ast_copy_flags(&features, &(config->features_callee), AST_FLAGS_ALL);
> +		dynamic_features = pbx_builtin_getvar_helper(peer, "DYNAMIC_FEATURES");
> +	}

The handling of the "dynamic_features" variable here is not thread safe.

-- 
Russell Bryant
Digium, Inc. | Senior Software Engineer, Open Source Team Lead
445 Jan Davis Drive NW - Huntsville, AL 35806 - USA
Check us out at: www.digium.com & www.asterisk.org