From owner-freebsd-current@FreeBSD.ORG Mon Jun 28 17:25:36 2010 Return-Path: Delivered-To: freebsd-current@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 0BC20106566B for ; Mon, 28 Jun 2010 17:25:36 +0000 (UTC) (envelope-from imp@bsdimp.com) Received: from harmony.bsdimp.com (bsdimp.com [199.45.160.85]) by mx1.freebsd.org (Postfix) with ESMTP id AEB368FC13 for ; Mon, 28 Jun 2010 17:25:35 +0000 (UTC) Received: from localhost (localhost [127.0.0.1]) by harmony.bsdimp.com (8.14.3/8.14.1) with ESMTP id o5SHJuQ9086054; Mon, 28 Jun 2010 11:19:57 -0600 (MDT) (envelope-from imp@bsdimp.com) Date: Mon, 28 Jun 2010 11:20:08 -0600 (MDT) Message-Id: <20100628.112008.459136215980124736.imp@bsdimp.com> To: yanefbsd@gmail.com From: "M. Warner Losh" In-Reply-To: References: <20100627.160845.256787458594170652.imp@bsdimp.com> X-Mailer: Mew version 6.3 on Emacs 22.3 / Mule 5.0 (SAKAKI) Mime-Version: 1.0 Content-Type: Text/Plain; charset=iso-8859-1 Content-Transfer-Encoding: quoted-printable Cc: freebsd-current@freebsd.org, hselasky@c2i.net Subject: Re: Patch for rc.d/devd on FreeBSD 9-current X-BeenThere: freebsd-current@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: Discussions about the use of FreeBSD-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 28 Jun 2010 17:25:36 -0000 In message: Garrett Cooper writes: : > This patch is wrong. =A0The problems with it: : > : > (1) You don't need to unlink the pipe. =A0If you can't unlink it, t= hen : > =A0 =A0you 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 w= ill : > =A0 =A0remain 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 =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D --- devd.hh (revision 209492) +++ devd.hh (working copy) @@ -153,6 +153,7 @@ void set_pidfile(const char *); void reset(); void parse(); + void close_pidfile(); void open_pidfile(); void write_pidfile(); void remove_pidfile(); Index: devd.cc =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D --- devd.cc (revision 209492) +++ devd.cc (working copy) @@ -41,6 +41,7 @@ #include #include #include +#include #include = #include @@ -49,6 +50,7 @@ #include #include #include +#include #include #include #include @@ -152,13 +154,67 @@ // 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 =3D SIG_IGN; + ::sigemptyset(&ign.sa_mask); + ign.sa_flags =3D 0; + ::sigaction(SIGINT, &ign, &intact); + ::sigaction(SIGQUIT, &ign, &quitact); + ::sigemptyset(&newsigblock); + ::sigaddset(&newsigblock, SIGCHLD); + ::sigprocmask(SIG_BLOCK, &newsigblock, &oldsigblock); + switch(pid =3D ::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 =3D pid; + do { + pid =3D ::wait4(savedpid, &pstat, 0, (struct rusage *)0); + } while (pid =3D=3D -1 && errno =3D=3D EINTR); + break; + } + ::sigaction(SIGINT, &intact, NULL); + ::sigaction(SIGQUIT, &quitact, NULL); + ::sigprocmask(SIG_SETMASK, &oldsigblock, NULL); + return(pid =3D=3D -1 ? -1 : pstat); +} + bool action::do_action(config &c) { string s =3D 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); } = @@ -391,6 +447,13 @@ } = void +config::close_pidfile() +{ + = + pidfile_close(pfh); +} + +void config::remove_pidfile() { =