Re: Patch for rc.d/devd on FreeBSD 9-current

In message: <AANLkTimpbAzY2gu8Fsly6wqDYZVg7C0ID-vzeH3A3N8s_at_mail.gmail.com>
            Garrett Cooper <yanefbsd_at_gmail.com> writes:
: > This patch is wrong.  The problems with it:
: >
: > (1) You don't need to unlink the pipe.  If you can't unlink it, then
: >    you won't remove the pid file.
: 
: The reason why my previous patch deleted `PIPE' file was because it's
: not a true pipe, but instead a named file in the filesystem, as that
: never gets cleaned up otherwise (please see my latest post).

That's Ok.  The problem is elsewhere.  The PIPE should not be deleted,
since that introduces races with other programs wishing to listen to
the pipe.

: > (2) If devd dies suddenly (kill -9, segv, etc), then the pid file will
: >    remain around, and devd will fail to start.
: 
: Correct.

Not quite.  The pid file doesn't prevent devd starting.  The lock on
the pid file does.

Why is the pid file still locked?  Is locking broken in current? (no,
it isn't: all relevant regression tests pass) What is the root cause
then?

For me, devd refused to start because it exec'd dhclient.  dhclient
survived having devd die.  devd didn't close the lock file fd, so
dhclient inherited it.

My first knee-jerk reaction was 'why isn't that close on exec?'
That's a good question, the short answer of which is 'because there
are times you don't want it to be.'

But even that's not the root cause.  Why is devd using system(3) to
execute commands?  Why isn't it taking more care to create a cleaner
environment for commands to execute in?  why isn't it following the
advice in the pidfile_close man page?  These are all good questions.
Also, why do the child processes get an fd to read the stream of
events from the kernel on?  Isn't that an information leak?

So, I've copied system and added the proper calls to clean all this
up.

Sadly, this only addresses the complaint that Garrett brought up.  It
doesn't address the legitimate concerns that Hans has about startup
sequence and races with mounting /usr/local.  That's a very good
question, indeed.  You want to defer some actions until other services
are available.  In a launchd world, you fork and run the processes.
They use whatever resources they need, but block on resources that
haven't finished starting yet.  That's one possible solution here, but
outside the scope of devd, I'm afraid...

Warner

Index: devd.hh
===================================================================
--- devd.hh	(revision 209492)
+++ devd.hh	(working copy)
_at__at_ -153,6 +153,7 _at__at_
 	void set_pidfile(const char *);
 	void reset();
 	void parse();
+	void close_pidfile();
 	void open_pidfile();
 	void write_pidfile();
 	void remove_pidfile();
Index: devd.cc
===================================================================
--- devd.cc	(revision 209492)
+++ devd.cc	(working copy)
_at__at_ -41,6 +41,7 _at__at_
 #include <sys/stat.h>
 #include <sys/sysctl.h>
 #include <sys/types.h>
+#include <sys/wait.h>
 #include <sys/un.h>
 
 #include <ctype.h>
_at__at_ -49,6 +50,7 _at__at_
 #include <err.h>
 #include <fcntl.h>
 #include <libutil.h>
+#include <paths.h>
 #include <regex.h>
 #include <signal.h>
 #include <stdlib.h>
_at__at_ -152,13 +154,67 _at__at_
 	// nothing
 }
 
+static int
+my_system(const char *command)
+{
+	pid_t pid, savedpid;
+	int pstat;
+	struct sigaction ign, intact, quitact;
+	sigset_t newsigblock, oldsigblock;
+
+	if (!command)		/* just checking... */
+		return(1);
+
+	/*
+	 * Ignore SIGINT and SIGQUIT, block SIGCHLD. Remember to save
+	 * existing signal dispositions.
+	 */
+	ign.sa_handler = SIG_IGN;
+	::sigemptyset(&ign.sa_mask);
+	ign.sa_flags = 0;
+	::sigaction(SIGINT, &ign, &intact);
+	::sigaction(SIGQUIT, &ign, &quitact);
+	::sigemptyset(&newsigblock);
+	::sigaddset(&newsigblock, SIGCHLD);
+	::sigprocmask(SIG_BLOCK, &newsigblock, &oldsigblock);
+	switch(pid = ::fork()) {
+	case -1:			/* error */
+		break;
+	case 0:				/* child */
+		/*
+		 * Restore original signal dispositions and exec the command.
+		 */
+		::sigaction(SIGINT, &intact, NULL);
+		::sigaction(SIGQUIT,  &quitact, NULL);
+		::sigprocmask(SIG_SETMASK, &oldsigblock, NULL);
+		/*
+		 * Close the PID file, and all other open descriptors.
+		 * Inherit std{in,out,err} only.
+		 */
+		cfg.close_pidfile();
+		::closefrom(3);
+		::execl(_PATH_BSHELL, "sh", "-c", command, (char *)NULL);
+		::_exit(127);
+	default:			/* parent */
+		savedpid = pid;
+		do {
+			pid = ::wait4(savedpid, &pstat, 0, (struct rusage *)0);
+		} while (pid == -1 && errno == EINTR);
+		break;
+	}
+	::sigaction(SIGINT, &intact, NULL);
+	::sigaction(SIGQUIT,  &quitact, NULL);
+	::sigprocmask(SIG_SETMASK, &oldsigblock, NULL);
+	return(pid == -1 ? -1 : pstat);
+}
+
 bool
 action::do_action(config &c)
 {
 	string s = c.expand_string(_cmd);
 	if (Dflag)
 		fprintf(stderr, "Executing '%s'\n", s.c_str());
-	::system(s.c_str());
+	my_system(s.c_str());
 	return (true);
 }
 
_at__at_ -391,6 +447,13 _at__at_
 }
 
 void
+config::close_pidfile()
+{
+	
+	pidfile_close(pfh);
+}
+
+void
 config::remove_pidfile()
 {