--GYaKytDE8aa4+VVK
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

On Fri, Jun 24, 2005 at 01:19:36PM +0200, Peter Postma wrote:
  | I've added daemon mode (inspired from FreeBSD) to our ftpd, patch is
  | attached. Comments are welcome.

Various consistency tweaks.
Please examine ftpd and ensure that the rest of your changes
seem consistent with the coding practice used within.


  | --- ftpd.8	7 Aug 2003 09:46:39 -0000	1.74
  | +++ ftpd.8	24 Jun 2005 11:08:50 -0000
  | @@ -128,6 +136,16 @@
  |  .Nm
  |  exits with an exit code of 0 if access would be granted, or 1 otherwis=
e.
  |  This can be useful for testing configurations.
  | +.It Fl D
  | +Run as daemon,

End line with `.' not `,'.


  | --- ftpd.c	23 Jun 2005 04:20:41 -0000	1.166
  | +++ ftpd.c	24 Jun 2005 11:08:53 -0000
  | @@ -462,6 +479,103 @@
  |  	}
  |  	curname[0] =3D '\0';
  | =20
  | +	if (Dflag) {
  | +		int error, fd, i, n, *socks;
  | +		struct pollfd *fds;
  | +		struct addrinfo hints, *res, *res0;
  | +
  | +		if (daemon(1, 0) =3D=3D -1) {
  | +			syslog(LOG_ERR, "failed to daemonize: %m");
  | +			exit(1);
  | +		}
  | +		(void)signal(SIGCHLD, sigchild);

Please use sigaction() for consistency with the rest of ftpd(8).
("signal(3) Must Die"!)

Something like:
		memset(&sa, 0, sizeof(sa));
		sa.sa_handler =3D SIG_IGN;
		sa.sa_flags =3D SA_NOCLDWAIT;
		sigemptyset(&sa.sa_mask);
		(void) sigaction(SIGCHLD, &sa, NULL);

I'm not sure how this will interact with ftpd_popen() and ftpd_pclose().
I think the subsequent change of SIGCHLD to SIG_DFL later in main()
(i.e, in the child ftpd) will allow ftpd_pclose()'s waitpid() to DTRT
without having the parent-daemon-ftpd reap/ignore the status.

I think your patch's behaviour of setting SIGCHLD to the sigchild()
handler that waitpid()s everything may have had adverse effects
with ftpd_pclose().

Someone with more knowledge/experience of SIGCHLD semantics in
this situation may be able to provide enlightenment.



  | +		(void)memset(&hints, 0, sizeof(hints));
  | +		hints.ai_flags =3D AI_PASSIVE;
  | +		hints.ai_family =3D af;
  | +		hints.ai_socktype =3D SOCK_STREAM;
  | +		error =3D getaddrinfo(NULL, "ftp", &hints, &res0);
  | +		if (error) {
  | +			syslog(LOG_ERR, "getaddrinfo: %s", gai_strerror(error));
  | +			exit(1);
  | +		}
  | +
  | +		for (n =3D 0, res =3D res0; res !=3D NULL; res =3D res->ai_next)
  | +			n++;
  | +		if (n =3D=3D 0) {
  | +			syslog(LOG_ERR, "no addresses available");
  | +			exit(1);
  | +		}
  | +		socks =3D malloc(n * sizeof(int));
  | +		fds =3D malloc(n * sizeof(struct pollfd));
  | +		if (socks =3D=3D NULL || fds =3D=3D NULL) {
  | +			syslog(LOG_ERR, "malloc: %m");
  | +			exit(1);
  | +		}
  | +
  | +		for (n =3D 0, res =3D res0; res !=3D NULL; res =3D res->ai_next) {
  | +			socks[n] =3D socket(res->ai_family, res->ai_socktype,
  | +			    res->ai_protocol);
  | +			if (socks[n] < 0)
  | +				continue;
  | +			(void)setsockopt(socks[n], SOL_SOCKET, SO_REUSEADDR,
  | +			    &on, sizeof(on));
  | +			if (bind(socks[n], res->ai_addr, res->ai_addrlen) < 0) {

Check against =3D=3D -1 not < 0.
(I know ftpd isn't consistent regarding this; I'll clean that up later).


  | +				(void)close(socks[n]);
  | +				continue;
  | +			}
  | +			if (listen(socks[n], 12) < 0) {

Check against =3D=3D -1 not < 0.


  | +				(void)close(socks[n]);
  | +				continue;
  | +			}
  | +
  | +			fds[n].fd =3D socks[n];
  | +			fds[n].events =3D POLLIN;
  | +			n++;
  | +		}
  | +		if (n =3D=3D 0) {
  | +			syslog(LOG_ERR, "%m");
  | +			exit(1);
  | +		}
  | +		freeaddrinfo(res0);
  | +
  | +		if (pidfile(NULL) =3D=3D -1)
  | +			syslog(LOG_ERR, "failed to write a pid file: %m");
  | +
  | +		for (;;) {
  | +			if (poll(fds, n, INFTIM) < 0) {

Check against =3D=3D -1 not < 0.


  | +				if (errno =3D=3D EINTR)
  | +					continue;
  | +				syslog(LOG_ERR, "poll: %m");
  | +				exit(1);
  | +			}
  | +			for (i =3D 0; i < n; i++) {
  | +				if (fds[i].revents & POLLIN) {
  | +					fd =3D accept(fds[i].fd, NULL, NULL);
  | +					if (fd < 0) {

Check against =3D=3D -1 not < 0.


  | +						syslog(LOG_ERR, "accept: %m");
  | +						continue;
  | +					}
  | +					switch (fork()) {
  | +					case -1:
  | +						syslog(LOG_ERR, "fork: %m");
  | +						break;
  | +					case 0:
  | +						goto child;
  | +						/* NOTREACHED */
  | +					}
  | +					(void)close(fd);
  | +				}
  | +			}
  | +		}
  | +child:

Indent the goto label by one space.
(Helps with "cvs diff -p" :)


  | +		(void)dup2(fd, STDIN_FILENO);
  | +		(void)dup2(fd, STDOUT_FILENO);
  | +		(void)dup2(fd, STDERR_FILENO);
  | +		for (i =3D 0; i < n; i++)
  | +			(void)close(socks[i]);
  | +	}
  | +
  |  	memset((char *)&his_addr, 0, sizeof(his_addr));
  |  	addrlen =3D sizeof(his_addr.si_su);
  |  	if (getpeername(0, (struct sockaddr *)&his_addr.si_su, &addrlen) < 0)=
 {
  | @@ -670,6 +784,15 @@
  |  	urgflag =3D 1;
  |  }
  | =20
  | +static void
  | +sigchild(int signo)
  | +{
  | +	int saved_errno =3D errno;
  | +
  | +	while (waitpid(-1, NULL, WNOHANG) > 0)
  | +		continue;
  | +	errno =3D saved_errno;
  | +}

This function is unnecessary and probably incorrect -- see comments
earlier about sigaction.


Cheers,
Luke.

--GYaKytDE8aa4+VVK
Content-Type: application/pgp-signature
Content-Disposition: inline

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.1 (NetBSD)

iD8DBQFC2KCNpBhtmn8zJHIRAlUSAKCIebY2+Iqpex9mbN0qPlMqh4INHgCgvnIe
WevJUeiZUm2ZQY1CfJjzvEg=
=ZUGN
-----END PGP SIGNATURE-----

--GYaKytDE8aa4+VVK--