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

In message: <AANLkTilnYGNz7V6z6AkeKsqUvOMN8yLvO57GM1gOIsTD_at_mail.gmail.com>
            Garrett Cooper <yanefbsd_at_gmail.com> writes:
: On Sat, Jun 26, 2010 at 1:45 PM, Garrett Cooper <yanefbsd_at_gmail.com> wrote:
: > On Sat, Jun 26, 2010 at 1:29 PM, Hans Petter Selasky <hselasky_at_c2i.net> wrote:
: >> Hi,
: >>
: >> Sometimes utilities that are started by devd require libraries outside
: >> /usr/lib. One example is the webcamd utility which is started by devd upon USB
: >> device insertion. What do you think about the following patch:
: >>
: >> --- devd        (revision 209463)
: >> +++ devd        (local)
: >> _at__at_ -4,7 +4,7 _at__at_
: >>  #
: >>
: >>  # PROVIDE: devd
: >> -# REQUIRE: netif
: >> +# REQUIRE: netif ldconfig
: >>  # BEFORE: NETWORKING mountcritremote
: >>  # KEYWORD: nojail shutdown
: >
: > Funny since I was just monkeying around with this. This doesn't appear
: > to be the only issue with devd:
: >
: > # devd
: > devd: devd already running, pid: 430
: > # pgrep 430; echo $?
: > 1
: > #
: >
: > Looks like /etc/rc.d/devd restart is broken :(...
: 
: This seems to fix that.
: Thanks,
: -Garrett
: 
: Index: devd.cc
: ===================================================================
: --- devd.cc	(revision 209530)
: +++ devd.cc	(working copy)
: _at__at_ -739,6 +739,7 _at__at_
:  static void
:  event_loop(void)
:  {
: +	bool warn_user_about_cleanup;
:  	int rv;
:  	int fd;
:  	char buffer[DEVCTL_MAXBUF];
: _at__at_ -804,6 +805,17 _at__at_
:  			new_client(server_fd);
:  	}
:  	close(fd);
: +	close(server_fd);
: +
: +	if (unlink(PIPE) == 0) {
: +		cfg.remove_pidfile();
: +		warn_user_about_cleanup = false;
: +	} else
: +		warn_user_about_cleanup = true;
: +
: +	if (warn_user_about_cleanup == true)
: +		warn("cleanup failed");
: +
:  }
:  
:  /*

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.
(2) If devd dies suddenly (kill -9, segv, etc), then the pid file will
    remain around, and devd will fail to start.
(3) i_really_do_not_like_names_this_long_esp_when_they_are_not_needed.

The following works around the bug in pidfile_open, and allows me to
restart devd both in a nice shutdown mode, as well as preventing devd
from running multiple times.  I'm not 100% sure the error handling is
right, I'm still tracing through that path.  This seems to work.

Warner

Index: devd.cc
===================================================================
--- devd.cc	(revision 209492)
+++ devd.cc	(working copy)
_at__at_ -376,11 +376,18 _at__at_
 	if (_pidfile == "")
 		return;
 	pfh = pidfile_open(_pidfile.c_str(), 0600, &otherpid);
-	if (pfh == NULL) {
-		if (errno == EEXIST)
+	if (pfh != NULL)
+		return;
+	if (errno == EEXIST) {
+		/*
+		 * Work around a bug in libutil where it will return this
+		 * condition when the other process does not, in fact,
+		 * actually exist.  Use kill(2) to see if it is there or not.
+		 */
+		if (kill(otherpid, 0) == 0)
 			errx(1, "devd already running, pid: %d", (int)otherpid);
+	} else
 		warn("cannot open pid file");
-	}
 }
 
 void
_at__at_ -804,6 +811,8 _at__at_
 			new_client(server_fd);
 	}
 	close(fd);
+	close(server_fd);
+	cfg.remove_pidfile();
 }
 
 /*