[asterisk-commits] rmudgett: trunk r395559 - in /trunk: apps/ bridges/ include/asterisk/ main/ r...

SVN commits to the Asterisk project asterisk-commits at lists.digium.com
Fri Jul 26 16:10:27 CDT 2013


Author: rmudgett
Date: Fri Jul 26 16:10:24 2013
New Revision: 395559

URL: http://svnview.digium.com/svn/asterisk?view=rev&rev=395559
Log:
Improved feature limits interval hook implementaion.

* Fixed feature limits to not use special members of struct
ast_bridge_features.

* Fixed memory leak in off nominal paths of bridge_builtin_set_limits().

* Fixed off nominal path in ast_bridge_features_limits_construct() freeing
unallocated memory if it was not called by bridge_builtin_set_limits().

* Made bridge_builtin_interval_features.so unloadable.

* Simplified parking's use of its duration interval hook.

* Made BridgeWait S option not depend upon another module being loaded.

(closes issue ASTERISK-22107)
Reported by: Matt Jordan

Review: https://reviewboard.asterisk.org/r/2701/

Modified:
    trunk/apps/app_bridgewait.c
    trunk/bridges/bridge_builtin_interval_features.c
    trunk/include/asterisk/bridge_features.h
    trunk/main/bridge.c
    trunk/res/parking/parking_bridge_features.c

Modified: trunk/apps/app_bridgewait.c
URL: http://svnview.digium.com/svn/asterisk/trunk/apps/app_bridgewait.c?view=diff&rev=395559&r1=395558&r2=395559
==============================================================================
--- trunk/apps/app_bridgewait.c (original)
+++ trunk/apps/app_bridgewait.c Fri Jul 26 16:10:24 2013
@@ -201,31 +201,33 @@
 	AST_APP_OPTION_ARG('S', MUXFLAG_TIMEOUT, OPT_ARG_TIMEOUT),
 });
 
+static int bridgewait_timeout_callback(struct ast_bridge *bridge, struct ast_bridge_channel *bridge_channel, void *hook_pvt)
+{
+	ast_verb(3, "Channel %s timed out.\n", ast_channel_name(bridge_channel->chan));
+	ast_bridge_channel_leave_bridge(bridge_channel, BRIDGE_CHANNEL_STATE_END);
+	return -1;
+}
+
 static int apply_option_timeout(struct ast_bridge_features *features, char *duration_arg)
 {
-	struct ast_bridge_features_limits hold_limits;
+	unsigned int duration;
 
 	if (ast_strlen_zero(duration_arg)) {
-		ast_log(LOG_ERROR, "No duration value provided for the timeout ('S') option.\n");
-		return -1;
-	}
-
-	if (ast_bridge_features_limits_construct(&hold_limits)) {
-		ast_log(LOG_ERROR, "Could not construct duration limits. Bridge canceled.\n");
-		return -1;
-	}
-
-	if (sscanf(duration_arg, "%u", &hold_limits.duration) != 1
-		|| hold_limits.duration == 0) {
-		ast_log(LOG_ERROR, "Duration value provided for the timeout ('S') option must be greater than 0\n");
-		ast_bridge_features_limits_destroy(&hold_limits);
-		return -1;
-	}
-
-	/* Limits struct holds time as milliseconds, so muliply 1000x */
-	hold_limits.duration *= 1000;
-	ast_bridge_features_set_limits(features, &hold_limits, AST_BRIDGE_HOOK_REMOVE_ON_PULL);
-	ast_bridge_features_limits_destroy(&hold_limits);
+		ast_log(LOG_ERROR, "Timeout option 'S': No value provided.\n");
+		return -1;
+	}
+	if (sscanf(duration_arg, "%u", &duration) != 1 || duration == 0) {
+		ast_log(LOG_ERROR, "Timeout option 'S': Invalid value provided '%s'.\n",
+			duration_arg);
+		return -1;
+	}
+
+	duration *= 1000;
+	if (ast_bridge_interval_hook(features, duration, bridgewait_timeout_callback,
+		NULL, NULL, AST_BRIDGE_HOOK_REMOVE_ON_PULL)) {
+		ast_log(LOG_ERROR, "Timeout option 'S': Could not create timer.\n");
+		return -1;
+	}
 
 	return 0;
 }

Modified: trunk/bridges/bridge_builtin_interval_features.c
URL: http://svnview.digium.com/svn/asterisk/trunk/bridges/bridge_builtin_interval_features.c?view=diff&rev=395559&r1=395558&r2=395559
==============================================================================
--- trunk/bridges/bridge_builtin_interval_features.c (original)
+++ trunk/bridges/bridge_builtin_interval_features.c Fri Jul 26 16:10:24 2013
@@ -64,7 +64,7 @@
 	return -1;
 }
 
-static void limits_interval_playback(struct ast_bridge *bridge, struct ast_bridge_channel *bridge_channel, struct ast_bridge_features_limits *limits, const char *file)
+static void limits_interval_playback(struct ast_bridge_channel *bridge_channel, struct ast_bridge_features_limits *limits, const char *file)
 {
 	if (!strcasecmp(file, "timeleft")) {
 		unsigned int remaining = ast_tvdiff_ms(limits->quitting_time, ast_tvnow()) / 1000;
@@ -114,11 +114,7 @@
 {
 	struct ast_bridge_features_limits *limits = hook_pvt;
 
-	if (bridge_channel->state != BRIDGE_CHANNEL_STATE_WAIT) {
-		return -1;
-	}
-
-	limits_interval_playback(bridge, bridge_channel, limits, limits->connect_sound);
+	limits_interval_playback(bridge_channel, limits, limits->connect_sound);
 	return -1;
 }
 
@@ -126,72 +122,75 @@
 {
 	struct ast_bridge_features_limits *limits = hook_pvt;
 
-	if (bridge_channel->state == BRIDGE_CHANNEL_STATE_WAIT) {
-		/* If we aren't in the wait state, something more important than this warning is happening and we should skip it. */
-		limits_interval_playback(bridge, bridge_channel, limits, limits->warning_sound);
-	}
-
-	return !limits->frequency ? -1 : limits->frequency;
-}
-
-static void copy_bridge_features_limits(struct ast_bridge_features_limits *dst, struct ast_bridge_features_limits *src)
-{
+	limits_interval_playback(bridge_channel, limits, limits->warning_sound);
+	return limits->frequency ?: -1;
+}
+
+static void bridge_features_limits_copy(struct ast_bridge_features_limits *dst, struct ast_bridge_features_limits *src)
+{
+	ast_string_fields_copy(dst, src);
+	dst->quitting_time = src->quitting_time;
 	dst->duration = src->duration;
 	dst->warning = src->warning;
 	dst->frequency = src->frequency;
-	dst->quitting_time = src->quitting_time;
-
-	ast_string_field_set(dst, duration_sound, src->duration_sound);
-	ast_string_field_set(dst, warning_sound, src->warning_sound);
-	ast_string_field_set(dst, connect_sound, src->connect_sound);
+}
+
+static void bridge_features_limits_dtor(void *vdoomed)
+{
+	struct ast_bridge_features_limits *doomed = vdoomed;
+
+	ast_bridge_features_limits_destroy(doomed);
+	ast_module_unref(ast_module_info->self);
 }
 
 static int bridge_builtin_set_limits(struct ast_bridge_features *features,
-		struct ast_bridge_features_limits *limits, enum ast_bridge_hook_remove_flags remove_flags)
-{
-	struct ast_bridge_features_limits *feature_limits;
+	struct ast_bridge_features_limits *limits,
+	enum ast_bridge_hook_remove_flags remove_flags)
+{
+	RAII_VAR(struct ast_bridge_features_limits *, feature_limits, NULL, ao2_cleanup);
 
 	if (!limits->duration) {
 		return -1;
 	}
 
-	if (features->limits) {
-		ast_log(LOG_ERROR, "Tried to apply limits to a feature set that already has limits.\n");
-		return -1;
-	}
-
-	feature_limits = ast_malloc(sizeof(*feature_limits));
+	/* Create limits hook_pvt data. */
+	ast_module_ref(ast_module_info->self);
+	feature_limits = ao2_alloc_options(sizeof(*feature_limits),
+		bridge_features_limits_dtor, AO2_ALLOC_OPT_LOCK_NOLOCK);
 	if (!feature_limits) {
-		return -1;
-	}
-
+		ast_module_unref(ast_module_info->self);
+		return -1;
+	}
 	if (ast_bridge_features_limits_construct(feature_limits)) {
 		return -1;
 	}
-
-	copy_bridge_features_limits(feature_limits, limits);
-	features->limits = feature_limits;
-
-/* BUGBUG feature interval hooks need to be reimplemented to be more stand alone. */
+	bridge_features_limits_copy(feature_limits, limits);
+	feature_limits->quitting_time = ast_tvadd(ast_tvnow(),
+		ast_samp2tv(feature_limits->duration, 1000));
+
+	/* Install limit hooks. */
+	ao2_ref(feature_limits, +1);
 	if (ast_bridge_interval_hook(features, feature_limits->duration,
-		bridge_features_duration_callback, feature_limits, NULL, remove_flags)) {
+		bridge_features_duration_callback, feature_limits, __ao2_cleanup, remove_flags)) {
 		ast_log(LOG_ERROR, "Failed to schedule the duration limiter to the bridge channel.\n");
-		return -1;
-	}
-
-	feature_limits->quitting_time = ast_tvadd(ast_tvnow(), ast_samp2tv(feature_limits->duration, 1000));
-
+		ao2_ref(feature_limits, -1);
+		return -1;
+	}
 	if (!ast_strlen_zero(feature_limits->connect_sound)) {
+		ao2_ref(feature_limits, +1);
 		if (ast_bridge_interval_hook(features, 1,
-			bridge_features_connect_callback, feature_limits, NULL, remove_flags)) {
+			bridge_features_connect_callback, feature_limits, __ao2_cleanup, remove_flags)) {
 			ast_log(LOG_WARNING, "Failed to schedule connect sound to the bridge channel.\n");
-		}
-	}
-
+			ao2_ref(feature_limits, -1);
+		}
+	}
 	if (feature_limits->warning && feature_limits->warning < feature_limits->duration) {
-		if (ast_bridge_interval_hook(features, feature_limits->duration - feature_limits->warning,
-			bridge_features_warning_callback, feature_limits, NULL, remove_flags)) {
+		ao2_ref(feature_limits, +1);
+		if (ast_bridge_interval_hook(features,
+			feature_limits->duration - feature_limits->warning,
+			bridge_features_warning_callback, feature_limits, __ao2_cleanup, remove_flags)) {
 			ast_log(LOG_WARNING, "Failed to schedule warning sound playback to the bridge channel.\n");
+			ao2_ref(feature_limits, -1);
 		}
 	}
 
@@ -200,17 +199,14 @@
 
 static int unload_module(void)
 {
+	ast_bridge_interval_unregister(AST_BRIDGE_BUILTIN_INTERVAL_LIMITS);
 	return 0;
 }
 
 static int load_module(void)
 {
-	ast_bridge_interval_register(AST_BRIDGE_BUILTIN_INTERVAL_LIMITS, bridge_builtin_set_limits);
-
-	/* Bump up our reference count so we can't be unloaded. */
-	ast_module_ref(ast_module_info->self);
-
-	return AST_MODULE_LOAD_SUCCESS;
+	return ast_bridge_interval_register(AST_BRIDGE_BUILTIN_INTERVAL_LIMITS,
+		bridge_builtin_set_limits);
 }
 
 AST_MODULE_INFO_STANDARD(ASTERISK_GPL_KEY, "Built in bridging interval features");

Modified: trunk/include/asterisk/bridge_features.h
URL: http://svnview.digium.com/svn/asterisk/trunk/include/asterisk/bridge_features.h?view=diff&rev=395559&r1=395558&r2=395559
==============================================================================
--- trunk/include/asterisk/bridge_features.h (original)
+++ trunk/include/asterisk/bridge_features.h Fri Jul 26 16:10:24 2013
@@ -237,8 +237,6 @@
 	struct ao2_container *other_hooks;
 	/*! Attached interval hooks */
 	struct ast_heap *interval_hooks;
-	/*! Limits feature data */
-	struct ast_bridge_features_limits *limits;
 	/*! Feature flags that are enabled */
 	struct ast_flags feature_flags;
 	/*! Used to assign the sequence number to the next interval hook added. */
@@ -298,13 +296,6 @@
  * \brief Structure that contains configuration information for the limits feature
  */
 struct ast_bridge_features_limits {
-	/*! Maximum duration that the channel is allowed to be in the bridge (specified in milliseconds) */
-	unsigned int duration;
-	/*! Duration into the call when warnings should begin (specified in milliseconds or 0 to disable) */
-	unsigned int warning;
-	/*! Interval between the warnings (specified in milliseconds or 0 to disable) */
-	unsigned int frequency;
-
 	AST_DECLARE_STRING_FIELDS(
 		/*! Sound file to play when the maximum duration is reached (if empty, then nothing will be played) */
 		AST_STRING_FIELD(duration_sound);
@@ -315,6 +306,12 @@
 	);
 	/*! Time when the bridge will be terminated by the limits feature */
 	struct timeval quitting_time;
+	/*! Maximum duration that the channel is allowed to be in the bridge (specified in milliseconds) */
+	unsigned int duration;
+	/*! Duration into the call when warnings should begin (specified in milliseconds or 0 to disable) */
+	unsigned int warning;
+	/*! Interval between the warnings (specified in milliseconds or 0 to disable) */
+	unsigned int frequency;
 };
 
 /*!

Modified: trunk/main/bridge.c
URL: http://svnview.digium.com/svn/asterisk/trunk/main/bridge.c?view=diff&rev=395559&r1=395558&r2=395559
==============================================================================
--- trunk/main/bridge.c (original)
+++ trunk/main/bridge.c Fri Jul 26 16:10:24 2013
@@ -2974,7 +2974,6 @@
 	memset(limits, 0, sizeof(*limits));
 
 	if (ast_string_field_init(limits, 256)) {
-		ast_free(limits);
 		return -1;
 	}
 
@@ -2987,13 +2986,14 @@
 }
 
 int ast_bridge_features_set_limits(struct ast_bridge_features *features,
-		struct ast_bridge_features_limits *limits, enum ast_bridge_hook_remove_flags remove_flags)
+	struct ast_bridge_features_limits *limits,
+	enum ast_bridge_hook_remove_flags remove_flags)
 {
 	if (builtin_interval_handlers[AST_BRIDGE_BUILTIN_INTERVAL_LIMITS]) {
-		ast_bridge_builtin_set_limits_fn bridge_features_set_limits_callback;
-
-		bridge_features_set_limits_callback = builtin_interval_handlers[AST_BRIDGE_BUILTIN_INTERVAL_LIMITS];
-		return bridge_features_set_limits_callback(features, limits, remove_flags);
+		ast_bridge_builtin_set_limits_fn callback;
+
+		callback = builtin_interval_handlers[AST_BRIDGE_BUILTIN_INTERVAL_LIMITS];
+		return callback(features, limits, remove_flags);
 	}
 
 	ast_log(LOG_ERROR, "Attempted to set limits without an AST_BRIDGE_BUILTIN_INTERVAL_LIMITS callback registered.\n");
@@ -3179,13 +3179,6 @@
 			ao2_ref(hook, -1);
 		}
 		features->interval_hooks = ast_heap_destroy(features->interval_hooks);
-	}
-
-	/* If the features contains a limits pvt, destroy that as well. */
-	if (features->limits) {
-		ast_bridge_features_limits_destroy(features->limits);
-		ast_free(features->limits);
-		features->limits = NULL;
 	}
 
 	/* Destroy the miscellaneous other hooks container. */

Modified: trunk/res/parking/parking_bridge_features.c
URL: http://svnview.digium.com/svn/asterisk/trunk/res/parking/parking_bridge_features.c?view=diff&rev=395559&r1=395558&r2=395559
==============================================================================
--- trunk/res/parking/parking_bridge_features.c (original)
+++ trunk/res/parking/parking_bridge_features.c Fri Jul 26 16:10:24 2013
@@ -363,12 +363,6 @@
 	return 0;
 }
 
-static void parking_duration_cb_destroyer(void *hook_pvt)
-{
-	struct parked_user *user = hook_pvt;
-	ao2_ref(user, -1);
-}
-
 /*! \internal
  * \brief Interval hook. Pulls a parked call from the parking bridge after the timeout is passed and sets the resolution to timeout.
  *
@@ -523,7 +517,7 @@
 	ao2_ref(user, +1);
 
 	if (ast_bridge_interval_hook(features, time_limit,
-		parking_duration_callback, user, parking_duration_cb_destroyer, AST_BRIDGE_HOOK_REMOVE_ON_PULL)) {
+		parking_duration_callback, user, __ao2_cleanup, AST_BRIDGE_HOOK_REMOVE_ON_PULL)) {
 		ast_log(LOG_ERROR, "Failed to apply duration limits to the parking call.\n");
 		ao2_ref(user, -1);
 	}




More information about the asterisk-commits mailing list