[Asterisk-cvs] asterisk/res res_musiconhold.c,1.62,1.63

[kpfleming at lists.digium.com]

Update of /usr/cvsroot/asterisk/res
In directory mongoose.digium.com:/tmp/cvs-serv11456/res

Modified Files:
	res_musiconhold.c 
Log Message:
fix threading portability problem with FreeBSD (bug #4532)
ensure that all mpg123 child processes get killed when the parent is killed (bug #4532)


Index: res_musiconhold.c
===================================================================
RCS file: /usr/cvsroot/asterisk/res/res_musiconhold.c,v
retrieving revision 1.62
retrieving revision 1.63
diff -u -d -r1.62 -r1.63
--- res_musiconhold.c	30 Jun 2005 18:08:27 -0000	1.62
+++ res_musiconhold.c	11 Jul 2005 21:42:25 -0000	1.63
@@ -402,19 +402,44 @@
 		ast_log(LOG_WARNING, "Fork failed: %s\n", strerror(errno));
 		return -1;
 	}
-	if (!class->pid) {
-		int x;
-		close(fds[0]);
-		/* Stdout goes to pipe */
-		dup2(fds[1], STDOUT_FILENO);
-		/* Close unused file descriptors */
-		for (x=3;x<8192;x++) {
-			if (-1 != fcntl(x, F_GETFL)) {
-				close(x);
+	if (class->pid != 0) {	/* parent */
+		close(fds[1]);
+		return fds[0];
+	} else {
+		/* Child */
+		int i;
+		/*
+		 * On BSD systems with userland pthreads libraries, we need
+		 * to call fcntl() _before_ close() to avoid resetting
+		 * O_NONBLOCK on the internal descriptors.
+		 * It should not harm in other systems.
+		 *
+		 * After that, close the descriptors not needed in the child.
+		 * It is also important that we do not share descriptors
+		 * with the child process or it could change their blocking
+		 * state and give trouble in the thread scheduling.
+		 * Here, parent and child are connected only through the
+		 * endpoints of a pipe, so they share no descriptors.
+		 */  
+		for (i=0; i < getdtablesize(); i++) {
+			long fl = fcntl(i, F_GETFL);
+			if (fl != -1 && i != fds[1]) {
+				/* open and must be closed in the child */
+				fcntl(i, F_SETFL, O_NONBLOCK | fl);
+				close(i);
 			}
 		}
-		/* Child */
+		/* Stdout in the child goes to pipe */
+		dup2(fds[1], STDOUT_FILENO);
 		chdir(class->dir);
+		/*
+		 * mpg123 may fork children to be more responsive, and we
+		 * want to kill them all when we exit. To do so we set
+		 * the process group id of mpg123 and children to a different
+		 * value than the asterisk process so we can kill all at once.
+		 * So remember, class->pid is really class->pgid!
+		 */
+		setpgid(0, getpid());
 		if (ast_test_flag(class, MOH_CUSTOM)) {
 			execv(argv[0], argv);
 		} else {
@@ -425,14 +450,9 @@
 			/* Check PATH as a last-ditch effort */
 			execvp("mpg123", argv);
 		}
-		ast_log(LOG_WARNING, "Exec failed: %s\n", strerror(errno));
-		close(fds[1]);
 		exit(1);
-	} else {
-		/* Parent */
-		close(fds[1]);
+		return 0; /* unreached */
 	}
-	return fds[0];
 }
 
 static void *monmp3thread(void *data)
@@ -511,7 +531,7 @@
 				close(class->srcfd);
 				class->srcfd = -1;
 				if (class->pid) {
-					kill(class->pid, SIGKILL);
+					killpg(class->pid, SIGKILL); /* pgid! */
 					class->pid = 0;
 				}
 			} else
@@ -957,7 +977,7 @@
 			stime = time(NULL) + 2;
 			pid = moh->pid;
 			moh->pid = 0;
-			kill(pid, SIGKILL);
+			killpg(pid, SIGKILL); /* pgid! */
 			while ((ast_wait_for_input(moh->srcfd, 100) > -1) && (bytes = read(moh->srcfd, buff, 8192)) && time(NULL) < stime) {
 				tbytes = tbytes + bytes;
 			}