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

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

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