From owner-freebsd-current@FreeBSD.ORG Mon Jun 28 00:23:42 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 838DD106564A for ; Mon, 28 Jun 2010 00:23:42 +0000 (UTC) (envelope-from yanefbsd@gmail.com) Received: from mail-qw0-f54.google.com (mail-qw0-f54.google.com [209.85.216.54]) by mx1.freebsd.org (Postfix) with ESMTP id 3A9898FC0C for ; Mon, 28 Jun 2010 00:23:41 +0000 (UTC) Received: by qwg5 with SMTP id 5so1815132qwg.13 for ; Sun, 27 Jun 2010 17:23:34 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=gamma; h=domainkey-signature:mime-version:received:received:in-reply-to :references:date:message-id:subject:from:to:cc:content-type :content-transfer-encoding; bh=gjRXfi/lGhVczbXa3aZ5Bgx8qX7l1QAYQy4RShm/dsI=; b=ixMDkGgFNC+uX9Azosj/r6c53J6yZK7ER7Dfhu3yuPdzKPQOQIctBx1RV5e7QEH5vt lTR06caLzw8t1sFAcWzoAAtUvKy4Ycmr67pkIZC9z2KdauuRyNIZUWiP0mrKiN/c2B94 clgMLe/g6RGBNAedKxa02A+8WHJsmR4RIXehk= DomainKey-Signature: a=rsa-sha1; c=nofws; d=gmail.com; s=gamma; h=mime-version:in-reply-to:references:date:message-id:subject:from:to :cc:content-type:content-transfer-encoding; b=xGrVqx8CJhBxc2ucUzYT0NS972IdToCbaRT97c8OJgRPPrYRoDZu62250nqYoyShCd 9/203TVMHcJTenjNVe+EyMgn9WmRKDYSTGk+/q1DBp2zl5T8DizWElFQCvwTkiSEYkdt 6t+njqB5/g76GE/S1VMxf90qpA1qCRj6q3+9A= MIME-Version: 1.0 Received: by 10.229.211.18 with SMTP id gm18mr2150441qcb.12.1277684613839; Sun, 27 Jun 2010 17:23:33 -0700 (PDT) Received: by 10.229.80.75 with HTTP; Sun, 27 Jun 2010 17:23:33 -0700 (PDT) In-Reply-To: <20100627.160845.256787458594170652.imp@bsdimp.com> References: <201006262229.09747.hselasky@c2i.net> <20100627.160845.256787458594170652.imp@bsdimp.com> Date: Sun, 27 Jun 2010 17:23:33 -0700 Message-ID: From: Garrett Cooper To: "M. Warner Losh" 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 00:23:42 -0000 On Sun, Jun 27, 2010 at 3:08 PM, M. Warner Losh wrote: > In message: > =A0 =A0 =A0 =A0 =A0 =A0Garrett Cooper writes: > : On Sat, Jun 26, 2010 at 1:45 PM, Garrett Cooper wr= ote: > : > On Sat, Jun 26, 2010 at 1:29 PM, Hans Petter Selasky 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/.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