postfix-2.9-20110907

postfix/HISTORY

@@ -16930,11 +16930,16 @@ Apologies for any names omitted.
 	Cleanup: don't log vstream_tweak "connection reset by peer"
 	errors. File: util/vstream_tweak.c.
 
-20110903
+20110904-7
 
-	Bugfix: master daemon panic with an "at process limit X"
-	error, when "postfix reload" reduced the process limit for
-	some Postfix service from (a value larger than the current
-	process count for that service) to (a value <= the current
-	process count), and then a new connection was made to that
-	service.  File: master/master_avail.c.
+	Bugfix: master daemon panic with "master_spawn: at process
+	limit", when "postfix reload" reduces the process limit
+	from (a value larger than the current process count for
+	some service) to (a value <= the current process count),
+	and then a new connection is made to that service. This
+	structural solution centralizes the decision to monitor a
+	service port (or not). To improve robustness against future
+	code changes, it clarifies some of the internal dependencies
+	that exist inside the master daemon.  Files: master/master.h,
+	master/master_avail.c, master/master_conf.c,
+	master/master_service.c, master/master_spawn.c.

postfix/src/global/mail_version.h

@@ -20,7 +20,7 @@
   * Patches change both the patchlevel and the release date. Snapshots have no
   * patchlevel; they change the release date only.
   */
-#define MAIL_RELEASE_DATE	"20110905"
+#define MAIL_RELEASE_DATE	"20110907"
 #define MAIL_VERSION_NUMBER	"2.9"
 
 #ifdef SNAPSHOT

postfix/src/master/master.h

@@ -64,9 +64,11 @@ typedef struct MASTER_SERV {
 #define MASTER_FLAG_CONDWAKE	(1<<2)	/* wake up if actually used */
 #define MASTER_FLAG_INETHOST	(1<<3)	/* endpoint name specifies host */
 #define MASTER_FLAG_LOCAL_ONLY	(1<<4)	/* no remote clients */
+#define MASTER_FLAG_LISTEN	(1<<5)	/* monitor this port */
 
 #define MASTER_THROTTLED(f)	((f)->flags & MASTER_FLAG_THROTTLE)
 #define MASTER_MARKED_FOR_DELETION(f) ((f)->flags & MASTER_FLAG_MARK)
+#define MASTER_LISTENING(f)	((f)->flags & MASTER_FLAG_LISTEN)
 
 #define MASTER_LIMIT_OK(limit, count) ((limit) == 0 || ((count) < (limit)))
 
@@ -135,7 +137,10 @@ extern void master_vars_init(void);
 extern MASTER_SERV *master_head;
 extern void master_start_service(MASTER_SERV *);
 extern void master_stop_service(MASTER_SERV *);
-extern void master_restart_service(MASTER_SERV *);
+extern void master_restart_service(MASTER_SERV *, int);
+
+#define DO_CONF_RELOAD	1	/* config files were reloaded */
+#define NO_CONF_RELOAD	0	/* no config file was reloaded */
 
  /*
   * master_events.c

postfix/src/master/master_avail.c

@@ -26,15 +26,17 @@
 /*	available process, or this module causes a new process to be
 /*	created to service the request.
 /*
-/*	When the service runs out of process slots, a warning is logged.
-/*	When the service is eligible for stress-mode operation, servers
-/*	are restarted and new servers are created with stress mode enabled.
+/*	When the service runs out of process slots, and the service
+/*	is eligible for stress-mode operation, a warning is logged,
+/*	servers are asked to restart at their convenience, and new
+/*	servers are created with stress mode enabled.
 /*
 /*	master_avail_listen() ensures that someone monitors the service's
 /*	listen socket for connection requests (as long as resources
 /*	to handle connection requests are available).  This function may
-/*	be called at random. When the maximum number of servers is running,
-/*	connection requests are left in the system queue.
+/*	be called at random times, but it must be called after each status
+/*	change of a service (throttled, process limit, etc.) or child
+/*	process (taken, available, dead, etc.).
 /*
 /*	master_avail_cleanup() should be called when the named service
 /*	is taken out of operation. It terminates child processes by
@@ -42,9 +44,13 @@
 /*
 /*	master_avail_more() should be called when the named process
 /*	has become available for servicing new connection requests.
+/*	This function updates the process availability status and
+/*	counter, and implicitly calls master_avail_listen().
 /*
 /*	master_avail_less() should be called when the named process
 /*	has become unavailable for servicing new connection requests.
+/*	This function updates the process availability status and
+/*	counter, and implicitly calls master_avail_listen().
 /* DIAGNOSTICS
 /*	Panic: internal inconsistencies.
 /* BUGS
@@ -81,16 +87,10 @@ static void master_avail_event(int event, char *context)
 {
     MASTER_SERV *serv = (MASTER_SERV *) context;
     time_t  now;
-    int     n;
 
     if (event == 0)				/* XXX Can this happen? */
-	return;
-    /* XXX Should check these when the process or service status is changed. */
-    if (!MASTER_LIMIT_OK(serv->max_proc, serv->total_proc)
-	|| MASTER_THROTTLED(serv)) {		/* XXX interface botch */
-	for (n = 0; n < serv->listen_fd_count; n++)
-	    event_disable_readwrite(serv->listen_fd[n]);
-    } else {
+	msg_panic("master_avail_event: null event");
+    else {
 
 	/*
 	 * When all servers for a public internet service are busy, we start
@@ -112,51 +112,70 @@ static void master_avail_event(int event, char *context)
 	    && !MASTER_LIMIT_OK(serv->max_proc, serv->total_proc + 1)) {
 	    now = event_time();
 	    if (serv->stress_expire_time < now)
-		master_restart_service(serv);
+		master_restart_service(serv, NO_CONF_RELOAD);
 	    serv->stress_expire_time = now + 1000;
 	}
 	master_spawn(serv);
     }
 }
 
-/* master_avail_listen - make sure that someone monitors the listen socket */
+/* master_avail_listen - enforce the socket monitoring policy */
 
 void    master_avail_listen(MASTER_SERV *serv)
 {
     const char *myname = "master_avail_listen";
+    int     listen_flag;
     time_t  now;
     int     n;
 
     /*
+     * Caution: several other master_XXX modules call master_avail_listen(),
+     * master_avail_more() or master_avail_less(). To avoid mutual dependency
+     * problems, the code below invokes no code in other master_XXX modules,
+     * and modifies no data that is maintained by other master_XXX modules.
+     * 
      * When no-one else is monitoring the service's listen socket, start
      * monitoring the socket for connection requests. All this under the
      * restriction that we have sufficient resources to service a connection
      * request.
      */
     if (msg_verbose)
-	msg_info("%s: avail %d total %d max %d", myname,
+	msg_info("%s: %s avail %d total %d max %d", myname, serv->name,
 		 serv->avail_proc, serv->total_proc, serv->max_proc);
-    if (serv->avail_proc < 1 && !MASTER_THROTTLED(serv)) {
-	if (MASTER_LIMIT_OK(serv->max_proc, serv->total_proc)) {
-	    if (msg_verbose)
-		msg_info("%s: enable events %s", myname, serv->name);
-	    for (n = 0; n < serv->listen_fd_count; n++)
-		event_enable_read(serv->listen_fd[n], master_avail_event,
-				  (char *) serv);
-	} else if ((serv->flags & MASTER_FLAG_LOCAL_ONLY) == 0
-		   && serv->max_proc != 1/* XXX postscreen(8) */
-		   && (now = event_time()) - serv->busy_warn_time > 1000) {
-	    serv->busy_warn_time = now;
-	    msg_warn("service \"%s\" (%s) has reached its process limit \"%d\": "
-		     "new clients may experience noticeable delays",
-		     serv->ext_name, serv->name, serv->max_proc);
-	    msg_warn("to avoid this condition, increase the process count "
-		     "in master.cf or reduce the service time per client");
-	    if (serv->stress_param_val)
+    if (MASTER_THROTTLED(serv) || serv->avail_proc > 0) {
+	listen_flag = 0;
+    } else if (MASTER_LIMIT_OK(serv->max_proc, serv->total_proc)) {
+	listen_flag = 1;
+    } else {
+	listen_flag = 0;
+	if (serv->stress_param_val != 0) {
+	    now = event_time();
+	    if (serv->busy_warn_time < now - 1000) {
+		serv->busy_warn_time = now;
+		msg_warn("service \"%s\" (%s) has reached its process limit \"%d\": "
+			 "new clients may experience noticeable delays",
+			 serv->ext_name, serv->name, serv->max_proc);
+		msg_warn("to avoid this condition, increase the process count "
+		      "in master.cf or reduce the service time per client");
 		msg_warn("see http://www.postfix.org/STRESS_README.html for "
 		      "examples of stress-adapting configuration settings");
+	    }
 	}
     }
+    if (listen_flag && !MASTER_LISTENING(serv)) {
+	if (msg_verbose)
+	    msg_info("%s: enable events %s", myname, serv->name);
+	for (n = 0; n < serv->listen_fd_count; n++)
+	    event_enable_read(serv->listen_fd[n], master_avail_event,
+			      (char *) serv);
+	serv->flags |= MASTER_FLAG_LISTEN;
+    } else if (!listen_flag && MASTER_LISTENING(serv)) {
+	if (msg_verbose)
+	    msg_info("%s: disable events %s", myname, serv->name);
+	for (n = 0; n < serv->listen_fd_count; n++)
+	    event_disable_readwrite(serv->listen_fd[n]);
+	serv->flags &= ~MASTER_FLAG_LISTEN;
+    }
 }
 
 /* master_avail_cleanup - cleanup */
@@ -167,8 +186,18 @@ void    master_avail_cleanup(MASTER_SERV *serv)
 
     master_delete_children(serv);		/* XXX calls
 						 * master_avail_listen */
-    for (n = 0; n < serv->listen_fd_count; n++)
-	event_disable_readwrite(serv->listen_fd[n]);	/* XXX must be last */
+
+    /*
+     * This code is redundant because master_delete_children() throttles the
+     * service temporarily before calling master_avail_listen/less(), which
+     * then turn off read events. This temporary throttling is not documented
+     * (it is only an optimization), and therefore we must not depend on it.
+     */
+    if (MASTER_LISTENING(serv)) {
+	for (n = 0; n < serv->listen_fd_count; n++)
+	    event_disable_readwrite(serv->listen_fd[n]);
+	serv->flags &= ~MASTER_FLAG_LISTEN;
+    }
 }
 
 /* master_avail_more - one more available child process */
@@ -176,9 +205,13 @@ void    master_avail_cleanup(MASTER_SERV *serv)
 void    master_avail_more(MASTER_SERV *serv, MASTER_PROC *proc)
 {
     const char *myname = "master_avail_more";
-    int     n;
 
     /*
+     * Caution: several other master_XXX modules call master_avail_listen(),
+     * master_avail_more() or master_avail_less(). To avoid mutual dependency
+     * problems, the code below invokes no code in other master_XXX modules,
+     * and modifies no data that is maintained by other master_XXX modules.
+     * 
      * This child process has become available for servicing connection
      * requests, so we can stop monitoring the service's listen socket. The
      * child will do it for us.
@@ -189,10 +222,7 @@ void    master_avail_more(MASTER_SERV *serv, MASTER_PROC *proc)
 	msg_panic("%s: process already available", myname);
     serv->avail_proc++;
     proc->avail = MASTER_STAT_AVAIL;
-    if (msg_verbose)
-	msg_info("%s: disable events %s", myname, serv->name);
-    for (n = 0; n < serv->listen_fd_count; n++)
-	event_disable_readwrite(serv->listen_fd[n]);
+    master_avail_listen(serv);
 }
 
 /* master_avail_less - one less available child process */
@@ -202,6 +232,11 @@ void    master_avail_less(MASTER_SERV *serv, MASTER_PROC *proc)
     const char *myname = "master_avail_less";
 
     /*
+     * Caution: several other master_XXX modules call master_avail_listen(),
+     * master_avail_more() or master_avail_less(). To avoid mutual dependency
+     * problems, the code below invokes no code in other master_XXX modules,
+     * and modifies no data that is maintained by other master_XXX modules.
+     * 
      * This child is no longer available for servicing connection requests.
      * When no child processes are available, start monitoring the service's
      * listen socket for new connection requests.

postfix/src/master/master_conf.c

@@ -135,7 +135,7 @@ void    master_config(void)
 	    SWAP(char *, serv->path, entry->path);
 	    SWAP(ARGV *, serv->args, entry->args);
 	    SWAP(char *, serv->stress_param_val, entry->stress_param_val);
-	    master_restart_service(serv);
+	    master_restart_service(serv, DO_CONF_RELOAD);
 	    free_master_ent(entry);
 	}
     }

postfix/src/master/master_service.c

@@ -12,15 +12,18 @@
 /*	void	master_stop_service(serv)
 /*	MASTER_SERV *serv;
 /*
-/*	void	master_restart_service(serv)
+/*	void	master_restart_service(serv, conf_reload)
 /*	MASTER_SERV *serv;
+/*	int	conf_reload;
 /* DESCRIPTION
 /*	master_start_service() enables the named service.
 /*
 /*	master_stop_service() disables named service.
 /*
 /*	master_restart_service() requests all running child processes to
-/*	commit suicide. This is typically used after a configuration reload.
+/*	commit suicide.  The conf_reload argument is either DO_CONF_RELOAD
+/*	(configuration files were reloaded, re-evaluate the child process
+/*	creation policy) or NO_CONF_RELOAD. 
 /* DIAGNOSTICS
 /* BUGS
 /* SEE ALSO
@@ -87,7 +90,7 @@ void    master_stop_service(MASTER_SERV *serv)
 
 /* master_restart_service - restart service after configuration reload */
 
-void    master_restart_service(MASTER_SERV *serv)
+void    master_restart_service(MASTER_SERV *serv, int conf_reload)
 {
 
     /*
@@ -101,4 +104,10 @@ void    master_restart_service(MASTER_SERV *serv)
      */
     master_status_init(serv);
     master_wakeup_init(serv);
+
+    /*
+     * Respond to configuration change.
+     */
+    if (conf_reload)
+	master_avail_listen(serv);
 }

postfix/src/master/master_spawn.c

@@ -110,7 +110,7 @@ static void master_unthrottle(MASTER_SERV *serv)
 	event_cancel_timer(master_unthrottle_wrapper, (char *) serv);
 	if (msg_verbose)
 	    msg_info("throttle released for command %s", serv->path);
-	master_avail_listen(serv);		/* XXX interface botch */
+	master_avail_listen(serv);
     }
 }
 
@@ -130,6 +130,7 @@ static void master_throttle(MASTER_SERV *serv)
 			    serv->throttle_delay);
 	if (msg_verbose)
 	    msg_info("throttling command %s", serv->path);
+	master_avail_listen(serv);
     }
 }
 
@@ -275,8 +276,7 @@ static void master_delete_child(MASTER_PROC *proc)
     serv->total_proc--;
     if (proc->avail == MASTER_STAT_AVAIL)
 	master_avail_less(serv, proc);
-    else if (MASTER_LIMIT_OK(serv->max_proc, serv->total_proc)
-	     && serv->avail_proc < 1)
+    else
 	master_avail_listen(serv);
     binhash_delete(master_child_table, (char *) &proc->pid,
 		   sizeof(proc->pid), (void (*) (char *)) 0);