Patch for rc.d/devd on FreeBSD 9-current
M. Warner Losh
imp at bsdimp.com
Mon Jun 28 14:35:00 UTC 2010
In message: <AANLkTikI223vbyBdEqLuA6FjcBBeQcqFujOimP5horsv at mail.gmail.com>
Garrett Cooper <yanefbsd at gmail.com> writes:
: On Sun, Jun 27, 2010 at 3:08 PM, M. Warner Losh <imp at bsdimp.com> wrote:
: > 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)
: > : >> @@ -4,7 +4,7 @@
: > : >> #
: > : >>
: > : >> # 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)
: > : @@ -739,6 +739,7 @@
: > : static void
: > : event_loop(void)
: > : {
: > : + bool warn_user_about_cleanup;
: > : int rv;
: > : int fd;
: > : char buffer[DEVCTL_MAXBUF];
: > : @@ -804,6 +805,17 @@
: > : 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)
: > @@ -376,11 +376,18 @@
: > 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
: > @@ -804,6 +811,8 @@
: > new_client(server_fd);
: > }
: > close(fd);
: > + close(server_fd);
: > + cfg.remove_pidfile();
: > }
:
: I see what you mean. pidfile_open obviously fails with this requirement:
:
: If a file can not be locked, a PID of an already running daemon is returned
: in the pidptr argument (if it is not NULL). The function does not write
: process' PID into the file here, so it can be used before
: fork()ing and exit
: with a proper error message when needed. If the path argument is NULL,
: /var/run/<progname>.pid file will be used.
:
: The problem I think is that the flopen arguments are wrong -- it passes in
: O_TRUNC, instead of checking whether or not the file exists beforehand, and
: then attempt to read in the file (if it exists) to determine whether
: or not the PID is alive.
Apart from tidiness, my patch is bogus. The real problem is flock()
seems to be failing now. Tests of my 8.0-stable system from April
shows that it works there. Likewise, my -current system from Jan 21st
appears to work. Time to start the binary search.
Warner
More information about the freebsd-current
mailing list