svn commit: r294543 - head/usr.sbin/ypldap

[Bruce Evans]

On Fri, 22 Jan 2016, Marcelo Araujo wrote:

> Log:
>  Switch from FD_SETSIZE to getdtablesize(2) as it can make the FD to be
>  tunable. Also it gets more close with the original implementation from
>  OpenBSD.

> Modified: head/usr.sbin/ypldap/yp.c
> ==============================================================================
> --- head/usr.sbin/ypldap/yp.c	Fri Jan 22 02:28:17 2016	(r294542)
> +++ head/usr.sbin/ypldap/yp.c	Fri Jan 22 03:02:38 2016	(r294543)
> @@ -83,17 +83,14 @@ void
> yp_enable_events(void)
> {
> 	int i;
> -	extern fd_set svc_fdset;
> 	struct yp_event	*ye;
>
> -	for (i = 0; i < FD_SETSIZE; i++) {
> -		if (FD_ISSET(i, &svc_fdset)) {
> -			if ((ye = calloc(1, sizeof(*ye))) == NULL)
> -				fatal(NULL);
> -			event_set(&ye->ye_event, i, EV_READ, yp_fd_event, NULL);
> -			event_add(&ye->ye_event, NULL);
> -			TAILQ_INSERT_TAIL(&env->sc_yp->yd_events, ye, ye_entry);
> -		}
> +	for (i = 0; i < getdtablesize(); i++) {
> +		if ((ye = calloc(1, sizeof(*ye))) == NULL)
> +			fatal(NULL);
> +		event_set(&ye->ye_event, i, EV_READ, yp_fd_event, NULL);
> +		event_add(&ye->ye_event, NULL);
> +		TAILQ_INSERT_TAIL(&env->sc_yp->yd_events, ye, ye_entry);
> 	}
> }

getdtablesize() is a syscall, so evaluating it every time in the loop is
slow.  Its value must be a loop invariant to work right.  Compilers
cannot reasonably tell that syscalls return invariant values.  Maybe they
should for the standard sysconf() values, but kernels have bugs like
extra limits that break the invariants.

Only old code or very unportable code should use getdtablize().  Code
newer than 1990 should use {OPEN_MAX}.  sysconf(_SC_OPEN_MAX) is
almost as easy to use as getdtablesize() provided you don't handle
errors for either.

Old code in /usr/src provides many examples of better use of getdtablesize().
In fact, the second largest subcollection of examples was in old yp code.
In FreeBSD-~5.2:

- login/login.c: this is the simplest good use.  It uses a count-down loop
   so that the top limit is only evaluated once

- rpcgen/rpc_svcount.c: this is careful to generate good code.  It generates
   a loop like the above, but with the loop invariant moved out of the loop.

- window/wwinit.c: this assigns getdtablesize() to a global but then never
   uses the value.  This was even more broken in 4.4BSD-Lite2.
   getdtablesize() was unused but the comment on the variable said that it
   was used.  The variable was the value of the highest fd seen, and was
   unused.

   This looks like some floundering to avoid using very large values
   of the limit.  It is never right to just use the result of the
   syscall, or {OPEN_MAX}.  E.g., blindly closing {OPEN_MAX} fd's in
   daemons is bad since it might make them take hours to start up.
   {OPEN_MAX} is 706977 on freefall.

- ppp/bundle.c, ppp/chap.c, ppp/chat.c, ppp/command.c, ppp/exec.c: top-down
   loop
- ppp/defs.c: used with malloc().  With no error checking of course.

- rpc/yppasswdd/yppasswdd_main.c, rpc/ypupdate/ypupdate_main.c,
   rpc/ypxfrd/ypxfrd_main.c, ypserv/yp_main.c: bottom-up loop with
   invariant evaluated earlier

- slip/sliplogin.c bottom-up loop with invariant evaluated earlier

- sysinstall/install.c, sysinstall/network.c, sysinstall/user.c: top-down
   loop
- sysinstall/system.c: 1 bottom-up-loop with invariant evaluated earlier,
   and 1 top-down loop

- syslogd/syslogd.c: top-down loop

- libexec/ftpd.c: home made popen() that mallocs() an array of size
   getdtablesize().  This actually has some error handling

- rbootd/rbootd.c: bottom-up-loop with invariant evaluated earlier

- rexecd/rexecd.c: top-down loop

- rshd/rshd.c: top-down loop

So all the examples were not too bad, modulo the bug that the API is
broken as designed (it is OK if {OPEN_MAX} is 20 instead of 706977
even on old systems where 20 is large).

{OPEN_MAX} was surprisingly little used in *bin and libexec.  I once
tried to remove all instances of OPEN_MAX and almost succeeded
(OPEN_MAX is the wrong static limit for {OPEN_MAX} and shouldn't
exist).  The only literal matches for OPEN_MAX were 4 in gperf and
4 in compat code in cron.

Now, OPEN_MAX is sed just twice more in  *bin and libexec.  It is
used in hastd as sysconf(_SC_OPEN_MAX) since hastd is too far from
BSD style to use getdtablesie().  The style differences include
too much error handling instead of too little.

getdtablesize() is now used in much the same places as in ~5.2, except:
- rexecd, slip_login, sysinstall and window went away
- I missed uses in atm in ~5.2, and there are none there now
- I missed a use in pkg_install in ~5.2, and it went away
- a more careful grep somehow found an inconsistent set for yp*
- large changes for syslogd and login.  Without looking at the code,
   I think that these are to reduces the slowness of looking at
   thousands or millions of fd's.  Looking at the code shows that
   both use closefrom(3).  closefrom() was new in FreeBSD-8.

The following old getdtablesize() loops can be converted to closefrom():
- rbootd, rexecd, rshd, atmarpd, scspd, lpr (another one I missed which
   has already been converted), pkg_install, ppp (1 instance -- most
   loops are for fcntl), sliplogin, sysinstall (all 8 instances)

closefrom() is now used in:
- casperd, devd, hastd (no error handling instead of too much), login,
   jail, lpr, syslogd.

Unfortunately, getdtablesize() is still needed for arrays as used in yp*,
and for anything using select().  If getdtablesize() is large then the
arrays should be sparse or large fd's should not be actually used.
closefrom() should probably be used early to kill unknown fd's to make
space for private fd's.  After that, getdtablesize() becomes almost
useless.  You just allocate a table large enough for the fd's that
you allocate, and don't allocate fd's sparsely.

POSIX and C guarantee that a small number (about 8 or 20) can be opened.
Hostile environments break even this.  Just open all except 7 fd's
and fork-exec or popen() your favourite application or C/POSIX test.
stdio should probably close some fd's to implement the guarantee.  But
none of libc and very few applications use closefrom().

Bruce