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

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)
> : >> _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.

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).

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

Correct.

> (3) i_really_do_not_like_names_this_long_esp_when_they_are_not_needed.

Fair enough :D.

> 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.

Thanks!
-Garrett