Date: Sun, 27 Jun 2010 20:17:16 -0600 (MDT) From: "M. Warner Losh" <imp@bsdimp.com> To: yanefbsd@gmail.com Cc: freebsd-current@FreeBSD.org, pjd@FreeBSD.org, hselasky@c2i.net Subject: Re: Patch for rc.d/devd on FreeBSD 9-current Message-ID: <20100627.201716.1108826596298620201.imp@bsdimp.com> In-Reply-To: <AANLkTikI223vbyBdEqLuA6FjcBBeQcqFujOimP5horsv@mail.gmail.com> References: <AANLkTilnYGNz7V6z6AkeKsqUvOMN8yLvO57GM1gOIsTD@mail.gmail.com> <20100627.160845.256787458594170652.imp@bsdimp.com> <AANLkTikI223vbyBdEqLuA6FjcBBeQcqFujOimP5horsv@mail.gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
In message: <AANLkTikI223vbyBdEqLuA6FjcBBeQcqFujOimP5horsv@mail.gmail.c=
om>
Garrett Cooper <yanefbsd@gmail.com> writes:
: On Sun, Jun 27, 2010 at 3:08 PM, M. Warner Losh <imp@bsdimp.com> wrot=
e:
: > In message: <AANLkTilnYGNz7V6z6AkeKsqUvOMN8yLvO57GM1gOIsTD@mail.gma=
il.com>
: > =A0 =A0 =A0 =A0 =A0 =A0Garrett Cooper <yanefbsd@gmail.com> writes:
: > : On Sat, Jun 26, 2010 at 1:45 PM, Garrett Cooper <yanefbsd@gmail.c=
om> wrote:
: > : > On Sat, Jun 26, 2010 at 1:29 PM, Hans Petter Selasky <hselasky@=
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 =A0 =A0 =A0 =A0(revision 209463)
: > : >> +++ devd =A0 =A0 =A0 =A0(local)
: > : >> @@ -4,7 +4,7 @@
: > : >> =A0#
: > : >>
: > : >> =A0# PROVIDE: devd
: > : >> -# REQUIRE: netif
: > : >> +# REQUIRE: netif ldconfig
: > : >> =A0# BEFORE: NETWORKING mountcritremote
: > : >> =A0# 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
: > : =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
: > : --- devd.cc =A0 (revision 209530)
: > : +++ devd.cc =A0 (working copy)
: > : @@ -739,6 +739,7 @@
: > : =A0static void
: > : =A0event_loop(void)
: > : =A0{
: > : + =A0 =A0 bool warn_user_about_cleanup;
: > : =A0 =A0 =A0 int rv;
: > : =A0 =A0 =A0 int fd;
: > : =A0 =A0 =A0 char buffer[DEVCTL_MAXBUF];
: > : @@ -804,6 +805,17 @@
: > : =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 new_client(server_fd)=
;
: > : =A0 =A0 =A0 }
: > : =A0 =A0 =A0 close(fd);
: > : + =A0 =A0 close(server_fd);
: > : +
: > : + =A0 =A0 if (unlink(PIPE) =3D=3D 0) {
: > : + =A0 =A0 =A0 =A0 =A0 =A0 cfg.remove_pidfile();
: > : + =A0 =A0 =A0 =A0 =A0 =A0 warn_user_about_cleanup =3D false;
: > : + =A0 =A0 } else
: > : + =A0 =A0 =A0 =A0 =A0 =A0 warn_user_about_cleanup =3D true;
: > : +
: > : + =A0 =A0 if (warn_user_about_cleanup =3D=3D true)
: > : + =A0 =A0 =A0 =A0 =A0 =A0 warn("cleanup failed");
: > : +
: > : =A0}
: > :
: > : =A0/*
: >
: > This patch is wrong. =A0The problems with it:
: >
: > (1) You don't need to unlink the pipe. =A0If you can't unlink it, t=
hen
: > =A0 =A0you won't remove the pid file.
: > (2) If devd dies suddenly (kill -9, segv, etc), then the pid file w=
ill
: > =A0 =A0remain around, and devd will fail to start.
: > (3) i_really_do_not_like_names_this_long_esp_when_they_are_not_need=
ed.
: >
: > The following works around the bug in pidfile_open, and allows me t=
o
: > restart devd both in a nice shutdown mode, as well as preventing de=
vd
: > from running multiple times. =A0I'm not 100% sure the error handlin=
g is
: > right, I'm still tracing through that path. =A0This seems to work.
: >
: > Warner
: >
: > Index: devd.cc
: > =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
: > --- devd.cc =A0 =A0 (revision 209492)
: > +++ devd.cc =A0 =A0 (working copy)
: > @@ -376,11 +376,18 @@
: > =A0 =A0 =A0 =A0if (_pidfile =3D=3D "")
: > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0return;
: > =A0 =A0 =A0 =A0pfh =3D pidfile_open(_pidfile.c_str(), 0600, &otherp=
id);
: > - =A0 =A0 =A0 if (pfh =3D=3D NULL) {
: > - =A0 =A0 =A0 =A0 =A0 =A0 =A0 if (errno =3D=3D EEXIST)
: > + =A0 =A0 =A0 if (pfh !=3D NULL)
: > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 return;
: > + =A0 =A0 =A0 if (errno =3D=3D EEXIST) {
: > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 /*
: > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0* Work around a bug in libutil whe=
re it will return this
: > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0* condition when the other process=
does not, in fact,
: > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0* actually exist. =A0Use kill(2) t=
o see if it is there or not.
: > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0*/
: > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 if (kill(otherpid, 0) =3D=3D 0)
: > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0errx(1, "devd alread=
y running, pid: %d", (int)otherpid);
: > + =A0 =A0 =A0 } else
: > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0warn("cannot open pid file");
: > - =A0 =A0 =A0 }
: > =A0}
: >
: > =A0void
: > @@ -804,6 +811,8 @@
: > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0new_client(server_fd=
);
: > =A0 =A0 =A0 =A0}
: > =A0 =A0 =A0 =A0close(fd);
: > + =A0 =A0 =A0 close(server_fd);
: > + =A0 =A0 =A0 cfg.remove_pidfile();
: > =A0}
: =
: I see what you mean. pidfile_open obviously fails with this requireme=
nt:
: =
: 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 n=
ot 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 i=
s NULL,
: /var/run/<progname>.pid file will be used.
: =
: The problem I think is that the flopen arguments are wrong -- it pass=
es in
: O_TRUNC, instead of checking whether or not the file exists beforehan=
d, 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@ since he wrong this function..
Warner
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20100627.201716.1108826596298620201.imp>