Date: Mon, 28 Jun 2010 08:26:58 -0600 (MDT) From: "M. Warner Losh" <imp@bsdimp.com> To: yanefbsd@gmail.com Cc: freebsd-current@freebsd.org, hselasky@c2i.net Subject: Re: Patch for rc.d/devd on FreeBSD 9-current Message-ID: <20100628.082658.385399974558107030.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.
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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20100628.082658.385399974558107030.imp>