Date: Sun, 27 Jun 2010 17:23:33 -0700 From: Garrett Cooper <yanefbsd@gmail.com> To: "M. Warner Losh" <imp@bsdimp.com> Cc: freebsd-current@freebsd.org, hselasky@c2i.net Subject: Re: Patch for rc.d/devd on FreeBSD 9-current Message-ID: <AANLkTikI223vbyBdEqLuA6FjcBBeQcqFujOimP5horsv@mail.gmail.com> In-Reply-To: <20100627.160845.256787458594170652.imp@bsdimp.com> References: <201006262229.09747.hselasky@c2i.net> <AANLkTilVB_E-BiOtC-gENBQ7FdPTLcmu8qpmdwU1GyXd@mail.gmail.com> <AANLkTilnYGNz7V6z6AkeKsqUvOMN8yLvO57GM1gOIsTD@mail.gmail.com> <20100627.160845.256787458594170652.imp@bsdimp.com>
next in thread | previous in thread | raw e-mail | index | archive | help
On Sun, Jun 27, 2010 at 3:08 PM, M. Warner Losh <imp@bsdimp.com> wrote:
> In message: <AANLkTilnYGNz7V6z6AkeKsqUvOMN8yLvO57GM1gOIsTD@mail.gmail.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.com> wr=
ote:
> : > On Sat, Jun 26, 2010 at 1:29 PM, Hans Petter Selasky <hselasky@c2i.ne=
t> wrote:
> : >> Hi,
> : >>
> : >> Sometimes utilities that are started by devd require libraries outsi=
de
> : >> /usr/lib. One example is the webcamd utility which is started by dev=
d 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 appea=
r
> : > 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, then
> =A0 =A0you won't remove the pid file.
> (2) If devd dies suddenly (kill -9, segv, etc), then the pid file will
> =A0 =A0remain 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. =A0I'm not 100% sure the error handling 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, &otherpid);
> - =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 where 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) to 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 already runn=
ing, 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 requirement:
If a file can not be locked, a PID of an already running daemon is ret=
urned
in the pidptr argument (if it is not NULL). The function does not wri=
te
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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?AANLkTikI223vbyBdEqLuA6FjcBBeQcqFujOimP5horsv>