Path: utzoo!attcan!uunet!timbuk!cs.umn.edu!uc!noc.MR.NET!msi.umn.edu!umeecs!umich!yale!cs.utexas.edu!usc!ucsd!nosc!dog.ee.lbl.gov!ace.ee.lbl.gov!leres
From: leres@ace.ee.lbl.gov (Craig Leres)
Newsgroups: news.software.nntp
Subject: Re: Too-many-open-files bug in NNTP 1.5.10 server fixed
Message-ID: <7603@dog.ee.lbl.gov>
Date: 16 Oct 90 04:27:46 GMT
References: <1802@tub.UUCP>
Sender: usenet@dog.ee.lbl.gov
Reply-To: leres@helios.ee.lbl.gov (ucbvax!leres for uucp weenies)
Organization: Lawrence Berkeley Laboratory, Berkeley
Lines: 58
X-Local-Date: Mon, 15 Oct 90 21:27:47 PDT

Oliver Laumann writes:
> The culprit is the call to vfork() followed by a call to closelog() in
> the child process in server/spawn.c.  closelog() not only closes the
> file descriptor used by syslog(), but also sets (or clears; I don't have
> the sources) a variable shared between closelog and syslog to indicate
> that the socket must be re-opened in the next call to syslog().

Too true. Personally, I dislike code that closes all fd's after
forking. It tends to break things like syslog and yp and in fact the
closelog() code is my fix. I didn't have any problems with it since
we're a cnews site and enqueue() uses fork() instead of vfork().

Anyway, if we must keep the child close() loop, seems like the best
solution is to move the closelog() to before the vfork(). This way both
the parent and the child will know to reopen the fd. And since most
openlog's()'s delay the syslog socket open until the first actual
syslog(), this shouldn't be too expensive.

Experimental context diff for the adventurous follows.

		Craig
------
RCS file: RCS/spawn.c,v
retrieving revision 1.10
diff -c -r1.10 spawn.c
*** /tmp/,RCSt1a14422	Mon Oct 15 21:25:25 1990
--- spawn.c	Mon Oct 15 21:23:48 1990
***************
*** 164,169 ****
--- 164,178 ----
  	 * whatever), open "tempfile" for input, thus making
  	 * it stdin, and then execle the inews.  We think.
  	 */
+ #ifdef SYSLOG
+ 	/*
+ 	 * Close in such a way that syslog() will know to reopen.
+ 	 * We must do this before the vfork() otherwise the parent
+ 	 * will think the syslog fd is closed and open a new one,
+ 	 * eventually using up all the available file descriptors.
+ 	 */
+ 	closelog();
+ #endif
  
  	pid = vfork();
  	if (pid == 0) {		/* We're in child */
***************
*** 183,192 ****
  		}
  		(void) dup2(1, 2);
  
- #ifdef SYSLOG
- 		/* Close in such a way that syslog() will know to reopen */
- 		closelog();
- #endif
  		for (i = 3; i < 10; ++i) /* XXX but getdtablesize is too big */
  			(void) close(i);
  
--- 192,197 ----