From owner-freebsd-current@FreeBSD.ORG Mon Jun 28 02:18:54 2010 Return-Path: Delivered-To: freebsd-current@FreeBSD.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 43A2F106566B; Mon, 28 Jun 2010 02:18:54 +0000 (UTC) (envelope-from imp@bsdimp.com) Received: from harmony.bsdimp.com (bsdimp.com [199.45.160.85]) by mx1.freebsd.org (Postfix) with ESMTP id ECF3B8FC16; Mon, 28 Jun 2010 02:18:53 +0000 (UTC) Received: from localhost (localhost [127.0.0.1]) by harmony.bsdimp.com (8.14.3/8.14.1) with ESMTP id o5S2H5qX077095; Sun, 27 Jun 2010 20:17:06 -0600 (MDT) (envelope-from imp@bsdimp.com) Date: Sun, 27 Jun 2010 20:17:16 -0600 (MDT) Message-Id: <20100627.201716.1108826596298620201.imp@bsdimp.com> To: yanefbsd@gmail.com From: "M. Warner Losh" In-Reply-To: References: <20100627.160845.256787458594170652.imp@bsdimp.com> X-Mailer: Mew version 6.3 on Emacs 22.3 / Mule 5.0 (SAKAKI) Mime-Version: 1.0 Content-Type: Text/Plain; charset=iso-8859-1 Content-Transfer-Encoding: quoted-printable Cc: freebsd-current@FreeBSD.org, pjd@FreeBSD.org, hselasky@c2i.net Subject: Re: Patch for rc.d/devd on FreeBSD 9-current X-BeenThere: freebsd-current@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: Discussions about the use of FreeBSD-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 28 Jun 2010 02:18:54 -0000 In message: Garrett Cooper writes: : On Sun, Jun 27, 2010 at 3:08 PM, M. Warner Losh wrot= e: : > In message: : > =A0 =A0 =A0 =A0 =A0 =A0Garrett Cooper writes: : > : On Sat, Jun 26, 2010 at 1:45 PM, Garrett Cooper wrote: : > : > On Sat, Jun 26, 2010 at 1:29 PM, Hans Petter Selasky 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/.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