[svn-commits] tilghman: branch 1.6.0 r232699 - in /branches/1.6.0: ./ res/res_musiconhold.c
SVN commits to the Digium repositories
svn-commits at lists.digium.com
Wed Dec 2 18:32:45 CST 2009
Author: tilghman
Date: Wed Dec 2 18:32:37 2009
New Revision: 232699
URL: http://svnview.digium.com/svn/asterisk?view=rev&rev=232699
Log:
Merged revisions 232660-232661 via svnmerge from
https://origsvn.digium.com/svn/asterisk/trunk
........
r232660 | tilghman | 2009-12-02 18:08:55 -0600 (Wed, 02 Dec 2009) | 19 lines
Fix multiple issues with musiconhold, which led to classes not getting destroyed properly.
* Classes are now tracked past removal from the core container, and module
removal is actively prevented until all references are freed.
* A hanging reference stored in the channel has been removed. This could have
caused a mismatch and the music state not properly cleared, if two or more
reloads occurred between MOH being stopped and MOH being restarted.
* In certain circumstances, duplicate classes were possible.
* A race existed at reload time between a process being killed and the thread
responsible for reading from the related pipe respawning that process.
* Several reference counts have also been corrected. At least one could have
caused deleted classes to stick around forever, consuming resources. This
originally manifested as MOH external processes that were not killed at
reload time.
(closes issue #16279, closes issue #16207)
Reported by: parisioa, dcabot
Patches:
20091202__issue16279__2.diff.txt uploaded by tilghman (license 14)
Tested by: parisioa, tilghman
........
r232661 | tilghman | 2009-12-02 18:09:36 -0600 (Wed, 02 Dec 2009) | 2 lines
Remove debugging line
........
Modified:
branches/1.6.0/ (props changed)
branches/1.6.0/res/res_musiconhold.c
Propchange: branches/1.6.0/
------------------------------------------------------------------------------
Binary property 'trunk-merged' - no diff available.
Modified: branches/1.6.0/res/res_musiconhold.c
URL: http://svnview.digium.com/svn/asterisk/branches/1.6.0/res/res_musiconhold.c?view=diff&rev=232699&r1=232698&r2=232699
==============================================================================
--- branches/1.6.0/res/res_musiconhold.c (original)
+++ branches/1.6.0/res/res_musiconhold.c Wed Dec 2 18:32:37 2009
@@ -71,6 +71,8 @@
#include "asterisk/astobj2.h"
#define INITIAL_NUM_FILES 8
+#define HANDLE_REF 1
+#define DONT_UNREF 0
static char *play_moh = "MusicOnHold";
static char *wait_moh = "WaitMusicOnHold";
@@ -124,11 +126,13 @@
struct moh_files_state {
struct mohclass *class;
+ char name[MAX_MUSICCLASS];
int origwfmt;
int samples;
int sample_queue;
int pos;
int save_pos;
+ int save_total;
char *save_pos_filename;
};
@@ -139,6 +143,9 @@
#define MOH_SORTALPHA (1 << 4)
#define MOH_CACHERTCLASSES (1 << 5) /*!< Should we use a separate instance of MOH for each user or not */
+
+/* Custom astobj2 flag */
+#define MOH_NOTDELETED (1 << 30) /*!< Find only records that aren't deleted? */
static struct ast_flags global_flags[1] = {{0}}; /*!< global MOH_ flags */
@@ -181,6 +188,8 @@
};
static struct ao2_container *mohclasses;
+static struct ao2_container *deleted_classes;
+static pthread_t deleted_thread;
#define LOCAL_MPG_123 "/usr/local/bin/mpg123"
#define MPG_123 "/usr/bin/mpg123"
@@ -216,7 +225,7 @@
state->save_pos = state->pos;
- mohclass_unref(state->class);
+ state->class = mohclass_unref(state->class);
}
static int ast_moh_files_next(struct ast_channel *chan)
@@ -325,7 +334,12 @@
return NULL;
}
- if (state->class != class) {
+ /* LOGIC: Comparing an unrefcounted pointer is a really bad idea, because
+ * malloc may allocate a different class to the same memory block. This
+ * might only happen when two reloads are generated in a short period of
+ * time, but it's still important to protect against.
+ * PROG: Compare the quick operation first, to save CPU. */
+ if (state->save_total != class->total_files || strcmp(state->name, class->name) != 0) {
memset(state, 0, sizeof(*state));
if (ast_test_flag(class, MOH_RANDOMIZE) && class->total_files) {
state->pos = ast_random() % class->total_files;
@@ -334,6 +348,9 @@
state->class = mohclass_ref(class);
state->origwfmt = chan->writeformat;
+ /* For comparison on restart of MOH (see above) */
+ ast_copy_string(state->name, class->name, sizeof(state->name));
+ state->save_total = class->total_files;
ast_verb(3, "Started music on hold, class '%s', on %s\n", class->name, chan->name);
@@ -540,6 +557,43 @@
close(fds[1]);
}
return fds[0];
+}
+
+static void *deleted_monitor(void *data)
+{
+ struct ao2_iterator iter;
+ struct mohclass *class;
+ struct ast_module *mod = NULL;
+
+ for (;;) {
+ pthread_testcancel();
+ if (ao2_container_count(deleted_classes) == 0) {
+ if (mod) {
+ ast_module_unref(mod);
+ mod = NULL;
+ }
+ poll(NULL, 0, -1);
+ } else {
+ /* While deleted classes are still in use, prohibit unloading */
+ mod = ast_module_ref(ast_module_info->self);
+ }
+ pthread_testcancel();
+ iter = ao2_iterator_init(deleted_classes, 0);
+ while ((class = ao2_iterator_next(&iter))) {
+ if (ao2_ref(class, 0) == 2) {
+ ao2_unlink(deleted_classes, class);
+ }
+ ao2_ref(class, -1);
+ }
+ ao2_iterator_destroy(&iter);
+ if (ao2_container_count(deleted_classes) == 0 && mod) {
+ ast_module_unref(mod);
+ mod = NULL;
+ }
+ pthread_testcancel();
+ sleep(60);
+ }
+ return NULL;
}
static void *monmp3thread(void *data)
@@ -602,11 +656,25 @@
class->srcfd = -1;
pthread_testcancel();
if (class->pid > 1) {
- killpg(class->pid, SIGHUP);
- usleep(100000);
- killpg(class->pid, SIGTERM);
- usleep(100000);
- killpg(class->pid, SIGKILL);
+ do {
+ if (killpg(class->pid, SIGHUP) < 0) {
+ ast_log(LOG_WARNING, "Unable to send a SIGHUP to MOH process?!!: %s\n", strerror(errno));
+ }
+ usleep(100000);
+ if (killpg(class->pid, SIGTERM) < 0) {
+ if (errno == ESRCH) {
+ break;
+ }
+ ast_log(LOG_WARNING, "Unable to terminate MOH process?!!: %s\n", strerror(errno));
+ }
+ usleep(100000);
+ if (killpg(class->pid, SIGKILL) < 0) {
+ if (errno == ESRCH) {
+ break;
+ }
+ ast_log(LOG_WARNING, "Unable to kill MOH process?!!: %s\n", strerror(errno));
+ }
+ } while (0);
class->pid = 0;
}
} else {
@@ -735,7 +803,9 @@
return 0;
}
-static struct mohclass *get_mohbyname(const char *name, int warn)
+#define get_mohbyname(a,b,c) _get_mohbyname(a,b,c,__FILE__,__LINE__,__PRETTY_FUNCTION__)
+
+static struct mohclass *_get_mohbyname(const char *name, int warn, int flags, const char *file, int lineno, const char *funcname)
{
struct mohclass *moh = NULL;
struct mohclass tmp_class = {
@@ -744,7 +814,7 @@
ast_copy_string(tmp_class.name, name, sizeof(tmp_class.name));
- moh = ao2_find(mohclasses, &tmp_class, 0);
+ moh = ao2_find(mohclasses, &tmp_class, flags);
if (!moh && warn) {
ast_log(LOG_DEBUG, "Music on Hold class '%s' not found in memory\n", name);
@@ -1076,27 +1146,28 @@
}
/*!
- * \note This function owns the reference it gets to moh
+ * \note This function owns the reference it gets to moh if unref is true
*/
-static int moh_register(struct mohclass *moh, int reload, int unref)
+#define moh_register(a,b,c) _moh_register(a,b,c,__FILE__,__LINE__,__PRETTY_FUNCTION__)
+static int _moh_register(struct mohclass *moh, int reload, int unref, const char *file, int line, const char *funcname)
{
struct mohclass *mohclass = NULL;
- if ((mohclass = get_mohbyname(moh->name, 0)) && !moh_diff(mohclass, moh)) {
- if (!mohclass->delete) {
- ast_log(LOG_WARNING, "Music on Hold class '%s' already exists\n", moh->name);
- mohclass = mohclass_unref(mohclass);
- if (unref) {
- moh = mohclass_unref(moh);
- }
- return -1;
- }
+ if ((mohclass = _get_mohbyname(moh->name, 0, MOH_NOTDELETED, file, line, funcname)) && !moh_diff(mohclass, moh)) {
+ ast_log(LOG_WARNING, "Music on Hold class '%s' already exists\n", moh->name);
+ mohclass = mohclass_unref(mohclass);
+ if (unref) {
+ moh = mohclass_unref(moh);
+ }
+ return -1;
+ } else if (mohclass) {
+ /* Found a class, but it's different from the one being registered */
mohclass = mohclass_unref(mohclass);
}
time(&moh->start);
moh->start -= respawn_time;
-
+
if (!strcasecmp(moh->mode, "files")) {
if (init_files_class(moh)) {
moh = mohclass_unref(moh);
@@ -1129,6 +1200,9 @@
struct moh_files_state *state = chan->music_state;
if (state) {
+ if (state->class) {
+ state->class = mohclass_unref(state->class);
+ }
ast_free(chan->music_state);
chan->music_state = NULL;
}
@@ -1136,7 +1210,9 @@
static void moh_class_destructor(void *obj);
-static struct mohclass *moh_class_malloc(void)
+#define moh_class_malloc() _moh_class_malloc(__FILE__,__LINE__,__PRETTY_FUNCTION__)
+
+static struct mohclass *_moh_class_malloc(const char *file, int line, const char *funcname)
{
struct mohclass *class;
@@ -1167,26 +1243,26 @@
* 4) The default class.
*/
if (!ast_strlen_zero(chan->musicclass)) {
- mohclass = get_mohbyname(chan->musicclass, 1);
+ mohclass = get_mohbyname(chan->musicclass, 1, 0);
if (!mohclass && realtime_possible) {
var = ast_load_realtime("musiconhold", "name", chan->musicclass, NULL);
}
}
if (!mohclass && !var && !ast_strlen_zero(mclass)) {
- mohclass = get_mohbyname(mclass, 1);
+ mohclass = get_mohbyname(mclass, 1, 0);
if (!mohclass && realtime_possible) {
var = ast_load_realtime("musiconhold", "name", mclass, NULL);
}
}
if (!mohclass && !var && !ast_strlen_zero(interpclass)) {
- mohclass = get_mohbyname(interpclass, 1);
+ mohclass = get_mohbyname(interpclass, 1, 0);
if (!mohclass && realtime_possible) {
var = ast_load_realtime("musiconhold", "name", interpclass, NULL);
}
}
if (!mohclass && !var) {
- mohclass = get_mohbyname("default", 1);
+ mohclass = get_mohbyname("default", 1, 0);
if (!mohclass && realtime_possible) {
var = ast_load_realtime("musiconhold", "name", "default", NULL);
}
@@ -1263,7 +1339,7 @@
* has a pointer to a freed mohclass, so any operations involving the mohclass container would result in reading
* invalid memory.
*/
- moh_register(mohclass, 0, 0);
+ moh_register(mohclass, 0, DONT_UNREF);
} else {
/* We don't register RT moh class, so let's init it manualy */
@@ -1368,8 +1444,19 @@
{
struct mohclass *class = obj;
struct mohdata *member;
+ pthread_t tid = 0;
ast_debug(1, "Destroying MOH class '%s'\n", class->name);
+
+ /* Kill the thread first, so it cannot restart the child process while the
+ * class is being destroyed */
+ if (class->thread != AST_PTHREADT_NULL && class->thread != 0) {
+ tid = class->thread;
+ class->thread = AST_PTHREADT_NULL;
+ pthread_cancel(tid);
+ /* We'll collect the exit status later, after we ensure all the readers
+ * are dead. */
+ }
if (class->pid > 1) {
char buff[8192];
@@ -1384,11 +1471,25 @@
/* Back when this was just mpg123, SIGKILL was fine. Now we need
* to give the process a reason and time enough to kill off its
* children. */
- killpg(pid, SIGHUP);
- usleep(100000);
- killpg(pid, SIGTERM);
- usleep(100000);
- killpg(pid, SIGKILL);
+ do {
+ if (killpg(pid, SIGHUP) < 0) {
+ ast_log(LOG_WARNING, "Unable to send a SIGHUP to MOH process?!!: %s\n", strerror(errno));
+ }
+ usleep(100000);
+ if (killpg(pid, SIGTERM) < 0) {
+ if (errno == ESRCH) {
+ break;
+ }
+ ast_log(LOG_WARNING, "Unable to terminate MOH process?!!: %s\n", strerror(errno));
+ }
+ usleep(100000);
+ if (killpg(pid, SIGKILL) < 0) {
+ if (errno == ESRCH) {
+ break;
+ }
+ ast_log(LOG_WARNING, "Unable to kill MOH process?!!: %s\n", strerror(errno));
+ }
+ } while (0);
while ((ast_wait_for_input(class->srcfd, 100) > 0) &&
(bytes = read(class->srcfd, buff, 8192)) && time(NULL) < stime) {
@@ -1402,12 +1503,6 @@
while ((member = AST_LIST_REMOVE_HEAD(&class->members, list))) {
free(member);
- }
-
- if (class->thread) {
- pthread_cancel(class->thread);
- pthread_join(class->thread, NULL);
- class->thread = AST_PTHREADT_NULL;
}
if (class->filearray) {
@@ -1418,6 +1513,11 @@
free(class->filearray);
class->filearray = NULL;
}
+
+ /* Finally, collect the exit status of the monitor thread */
+ if (tid > 0) {
+ pthread_join(tid, NULL);
+ }
}
static int moh_class_mark(void *obj, void *arg, int flags)
@@ -1433,7 +1533,12 @@
{
struct mohclass *class = obj;
- return class->delete ? CMP_MATCH : 0;
+ if (class->delete) {
+ ao2_link(deleted_classes, obj);
+ pthread_kill(deleted_thread, SIGURG);
+ return CMP_MATCH;
+ }
+ return 0;
}
static int load_moh_classes(int reload)
@@ -1524,7 +1629,7 @@
}
/* Don't leak a class when it's already registered */
- if (!moh_register(class, reload, 1)) {
+ if (!moh_register(class, reload, HANDLE_REF)) {
numclasses++;
}
}
@@ -1635,6 +1740,20 @@
}
}
ao2_iterator_destroy(&i);
+ i = ao2_iterator_init(deleted_classes, 0);
+ for (; (class = ao2_iterator_next(&i)); mohclass_unref(class)) {
+ ast_cli(a->fd, "(Deleted) Class: %s (%d)\n", class->name, ao2_ref(class, 0) - 2);
+ ast_cli(a->fd, "\tMode: %s\n", S_OR(class->mode, "<none>"));
+ ast_cli(a->fd, "\tDirectory: %s\n", S_OR(class->dir, "<none>"));
+ ast_cli(a->fd, "\tRealtime: %s\n", class->realtime ? "yes" : "no");
+ if (ast_test_flag(class, MOH_CUSTOM)) {
+ ast_cli(a->fd, "\tApplication: %s\n", S_OR(class->args, "<none>"));
+ }
+ if (strcasecmp(class->mode, "files")) {
+ ast_cli(a->fd, "\tFormat: %s\n", ast_getformatname(class->format));
+ }
+ }
+ ao2_iterator_destroy(&i);
return CLI_SUCCESS;
}
@@ -1656,7 +1775,9 @@
{
struct mohclass *class = obj, *class2 = arg;
- return strcasecmp(class->name, class2->name) ? 0 : CMP_MATCH | CMP_STOP;
+ return strcasecmp(class->name, class2->name) ? 0 :
+ (flags & MOH_NOTDELETED) && (class->delete || class2->delete) ? 0 :
+ CMP_MATCH | CMP_STOP;
}
static int load_module(void)
@@ -1664,6 +1785,14 @@
int res;
if (!(mohclasses = ao2_container_alloc(53, moh_class_hash, moh_class_cmp))) {
+ return AST_MODULE_LOAD_DECLINE;
+ }
+
+ if (!(deleted_classes = ao2_container_alloc(53, moh_class_hash, moh_class_cmp))) {
+ return AST_MODULE_LOAD_DECLINE;
+ }
+
+ if (ast_pthread_create_background(&deleted_thread, NULL, deleted_monitor, NULL)) {
return AST_MODULE_LOAD_DECLINE;
}
@@ -1734,6 +1863,10 @@
res |= ast_unregister_application(stop_moh);
ast_cli_unregister_multiple(cli_moh, sizeof(cli_moh) / sizeof(struct ast_cli_entry));
+ pthread_cancel(deleted_thread);
+ pthread_kill(deleted_thread, SIGURG);
+ pthread_join(deleted_thread, NULL);
+
return res;
}
More information about the svn-commits
mailing list