From 50aba6be3654e348960a3bda2312f7a913f5c160 Mon Sep 17 00:00:00 2001 From: Richard Mudgett Date: Fri, 26 Jul 2013 21:10:24 +0000 Subject: 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/ git-svn-id: https://origsvn.digium.com/svn/asterisk/trunk@395559 65c4cc65-6c06-0410-ace0-fbb531ad65f3 --- apps/app_bridgewait.c | 30 ++++++----- bridges/bridge_builtin_interval_features.c | 84 ++++++++++++++---------------- include/asterisk/bridge_features.h | 15 +++--- main/bridge.c | 17 ++---- res/parking/parking_bridge_features.c | 8 +-- 5 files changed, 68 insertions(+), 86 deletions(-) diff --git a/apps/app_bridgewait.c b/apps/app_bridgewait.c index fc2c10697..bdfd87e62 100644 --- a/apps/app_bridgewait.c +++ b/apps/app_bridgewait.c @@ -201,32 +201,34 @@ AST_APP_OPTIONS(bridgewait_opts, { 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"); + ast_log(LOG_ERROR, "Timeout option 'S': No value provided.\n"); return -1; } - - if (ast_bridge_features_limits_construct(&hold_limits)) { - ast_log(LOG_ERROR, "Could not construct duration limits. Bridge canceled.\n"); + 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; } - 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); + 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; } - /* 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); - return 0; } diff --git a/bridges/bridge_builtin_interval_features.c b/bridges/bridge_builtin_interval_features.c index 4df9cd874..4a091126b 100644 --- a/bridges/bridge_builtin_interval_features.c +++ b/bridges/bridge_builtin_interval_features.c @@ -64,7 +64,7 @@ static int bridge_features_duration_callback(struct ast_bridge *bridge, struct a 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 @@ static int bridge_features_connect_callback(struct ast_bridge *bridge, struct as { 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 @@ static int bridge_features_warning_callback(struct ast_bridge *bridge, struct as { 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; + limits_interval_playback(bridge_channel, limits, limits->warning_sound); + return limits->frequency ?: -1; } -static void copy_bridge_features_limits(struct ast_bridge_features_limits *dst, struct ast_bridge_features_limits *src) +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 *limits, + enum ast_bridge_hook_remove_flags remove_flags) { - struct ast_bridge_features_limits *feature_limits; + 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) { + ast_module_unref(ast_module_info->self); return -1; } - if (ast_bridge_features_limits_construct(feature_limits)) { return -1; } + bridge_features_limits_copy(feature_limits, limits); + feature_limits->quitting_time = ast_tvadd(ast_tvnow(), + ast_samp2tv(feature_limits->duration, 1000)); - copy_bridge_features_limits(feature_limits, limits); - features->limits = feature_limits; - -/* BUGBUG feature interval hooks need to be reimplemented to be more stand alone. */ + /* 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"); + ao2_ref(feature_limits, -1); return -1; } - - feature_limits->quitting_time = ast_tvadd(ast_tvnow(), ast_samp2tv(feature_limits->duration, 1000)); - 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 bridge_builtin_set_limits(struct ast_bridge_features *features, 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"); diff --git a/include/asterisk/bridge_features.h b/include/asterisk/bridge_features.h index 05fdf2587..3a4fd9444 100644 --- a/include/asterisk/bridge_features.h +++ b/include/asterisk/bridge_features.h @@ -237,8 +237,6 @@ struct ast_bridge_features { 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 @@ struct ast_bridge_features_automixmonitor { * \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 @@ struct ast_bridge_features_limits { ); /*! 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; }; /*! diff --git a/main/bridge.c b/main/bridge.c index 6a6744490..e3a00ed2e 100644 --- a/main/bridge.c +++ b/main/bridge.c @@ -2974,7 +2974,6 @@ int ast_bridge_features_limits_construct(struct ast_bridge_features_limits *limi memset(limits, 0, sizeof(*limits)); if (ast_string_field_init(limits, 256)) { - ast_free(limits); return -1; } @@ -2987,13 +2986,14 @@ void ast_bridge_features_limits_destroy(struct ast_bridge_features_limits *limit } 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; + ast_bridge_builtin_set_limits_fn callback; - bridge_features_set_limits_callback = builtin_interval_handlers[AST_BRIDGE_BUILTIN_INTERVAL_LIMITS]; - return bridge_features_set_limits_callback(features, limits, remove_flags); + 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"); @@ -3181,13 +3181,6 @@ void ast_bridge_features_cleanup(struct ast_bridge_features *features) 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. */ ao2_cleanup(features->other_hooks); features->other_hooks = NULL; diff --git a/res/parking/parking_bridge_features.c b/res/parking/parking_bridge_features.c index 1b57f9c07..1d9e3dada 100644 --- a/res/parking/parking_bridge_features.c +++ b/res/parking/parking_bridge_features.c @@ -363,12 +363,6 @@ static int feature_park(struct ast_bridge *bridge, struct ast_bridge_channel *br 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 @@ void parking_set_duration(struct ast_bridge_features *features, struct parked_us 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); } -- cgit v1.2.3