From owner-freebsd-rc@FreeBSD.ORG Fri Oct 16 00:06:21 2009 Return-Path: Delivered-To: freebsd-rc@FreeBSD.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 5FB971065670 for ; Fri, 16 Oct 2009 00:06:21 +0000 (UTC) (envelope-from dougb@FreeBSD.org) Received: from mail2.fluidhosting.com (mx21.fluidhosting.com [204.14.89.4]) by mx1.freebsd.org (Postfix) with ESMTP id E336F8FC13 for ; Fri, 16 Oct 2009 00:06:20 +0000 (UTC) Received: (qmail 21946 invoked by uid 399); 16 Oct 2009 00:06:19 -0000 Received: from localhost (HELO foreign.dougb.net) (dougb@dougbarton.us@127.0.0.1) by localhost with ESMTPAM; 16 Oct 2009 00:06:19 -0000 X-Originating-IP: 127.0.0.1 X-Sender: dougb@dougbarton.us Message-ID: <4AD7B8FA.7020703@FreeBSD.org> Date: Thu, 15 Oct 2009 17:06:18 -0700 From: Doug Barton Organization: http://SupersetSolutions.com/ User-Agent: Thunderbird 2.0.0.23 (X11/20090822) MIME-Version: 1.0 To: Hiroki Sato References: <200910052011.n95KBXdS024044@svn.freebsd.org> <4AD3A722.9060401@FreeBSD.org> <20091015.161939.200967153.hrs@allbsd.org> In-Reply-To: <20091015.161939.200967153.hrs@allbsd.org> X-Enigmail-Version: 0.96.0 OpenPGP: id=D5B2F0FB Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Cc: markm@FreeBSD.org, freebsd-rc@FreeBSD.org Subject: Re: svn commit: r197790 - head/etc X-BeenThere: freebsd-rc@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: "Discussion related to /etc/rc.d design and implementation." List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 16 Oct 2009 00:06:21 -0000 Hiroki Sato wrote: > Hi Doug, > > Doug Barton wrote > in <4AD3A722.9060401@FreeBSD.org>: > > do> I think this is the wrong solution to the problem. In at least two > do> cases (routed and route6d) where $command is not defined in the rc.d > do> scripts this change is resulting in $command not being defined at all. > do> > do> If you look at the definition of the + parameter substitution this > do> makes sense: > do> > do> ${parameter:+word} > do> Use Alternate Value. If parameter is unset or null, null > do> is substituted; otherwise, the expansion of word is > do> substituted. > do> > do> I think that what you really wanted to do was: > > I am sorry for the delay. Your patch is reasonable to me. This > problem was there for a while, so it should be fixed asap. Ok, I've committed the fix, thanks for getting back to me. > I noticed there was something wrong about ${name}_program but it > seems I mistakenly changed it (sorry...). Then I received a report > "it does not work" so I just reverted it. Understood. I am sure you realize that it's always Ok to ask for help here on -rc. The rc.d system is not life-threateningly complex but it does have a lot of "behind the scenes" interactions that are not always obvious. I certainly don't hesitate to ask for review on changes myself and I encourage others to do so (as you have done in the past). FWIW, what I do object to about your changes in r197144 and r197790 are that in the first case you neglected to mention that you were changing that part of the code, and in the second you neglected to mention that you were changing it back to what it was before you changed it. That made debugging this problem more difficult for me than (I think) it should have been. You also did not mention that you were removing $command in your changes to route[6]d, which made debugging Mark's original complaint harder, but only for about 30 seconds or so. :) The two most important things about VCS logs are WHAT you did (should be brief, but thorough) and WHY you did it. My logs are often obnoxiously long, but you can generally find at least that information in them. Please think about what people who read the logs years from now will need to know. Of course, I realize that this is difficult to do, especially when you are just wrapping up a project and all of the information is fresh in your head and seems painfully obvious. > IMO defining $command in rc.d scripts is not a good practice. > "Always use ${name}_program and let load_rc_config() set the > $command" would be consistent and useful to avoid this sort of > problems. In the past since there was not a reliable mechanism to allow ${name}_program to override $command, and because rc.subr needs to have $command defined to work properly (see the description in the run_rc_command comments in rc.subr) it was necessary to set command in each script. When yar introduced the current version of the override code almost 4 years ago he also went through and set command explicitly in all the scripts that didn't have it, so the situation that was created here never came up until now. (FYI, the previous code that he replaced had the same effect as what I just changed it to but was slightly more complex.) I'm sort of ambivalent about whether we need to continue encouraging people to use command in the script or not. As long as what's in the script matches what's in /etc/defaults/rc.conf we're not hurting anything, although we are duplicating effort. My preference at this point is to let the change that I just made settle for a while, mostly to see if it has any negative interactions with scripts from ports, then MFC it after 8.0-RELEASE along with the changes you've made to the IPv6 stuff. After that we can start talking about ripping command= out of the individual rc.d scripts if people think that's a good idea. Doug PS, Mark you're a slacker for not getting back to me. :) -- Improve the effectiveness of your Internet presence with a domain name makeover! http://SupersetSolutions.com/