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.
> (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();
>  }

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.

Thanks,
-Garrett