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.

O_TRUNC likely is OK.  If you can obtain the lock, then truncation is
what you want...

Maybe the real problem is that devd locks the file, then dies.  The
file remains locked, so the flopen is failing with EWOULDBLOCK.  That
gets turned into an EEXIST return value, with the PID of of what's in
the file returned.  The flock(2) man page is silent on what happens
when a process dies (although lockf(3) says that all locks for a
process are removed when the process terminates).

But I suspect the real real problem is the implicit assumption that
flock will release the lock when a process terminates...  That isn't
the case in BSD, and as such the backup kill() that I did in the above
patch is needed, and likely should be moved into the pidfile
implementation.

I've cc'd pjd_at_ since he wrong this function..

Warner