From owner-freebsd-current@FreeBSD.ORG Mon Jun 28 14:35:00 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 D8265106566B for ; Mon, 28 Jun 2010 14:35:00 +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 7CFC88FC12 for ; Mon, 28 Jun 2010 14:35:00 +0000 (UTC) Received: from localhost (localhost [127.0.0.1]) by harmony.bsdimp.com (8.14.3/8.14.1) with ESMTP id o5SEQlWI084199; Mon, 28 Jun 2010 08:26:47 -0600 (MDT) (envelope-from imp@bsdimp.com) Date: Mon, 28 Jun 2010 08:26:58 -0600 (MDT) Message-Id: <20100628.082658.385399974558107030.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, 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 14:35:00 -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. 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