Making use of set_rcvar.
Locked
12
12
 
|
Any objections?
http://people.freebsd.org/~pjd/patches/set_rcvar.patch This patch only changes scripts where set_rcvar can be used with no arguments. -- Pawel Jakub Dawidek http://www.wheelsystems.com FreeBSD committer http://www.FreeBSD.org Am I Evil? Yes, I Am! http://yomoli.com |
attachment0 (203 bytes)
|
|
|
On Fri, Jan 06, 2012 at 04:17:40PM +0100, Jilles Tjoelker wrote:
> On Fri, Jan 06, 2012 at 03:13:03PM +0100, Pawel Jakub Dawidek wrote: > > Any objections? > > > http://people.freebsd.org/~pjd/patches/set_rcvar.patch > > > This patch only changes scripts where set_rcvar can be used with no > > arguments. > > While this makes the scripts cleaner, it will lead to extra forks with > the current implementation of sh which will slow down boot to some > extent. The effect is small on x86 but may be large on embedded > platforms. set_rcvar is preferred. I'm fine either way, but would like it to be consistent across system startup scripts. I like using set_rcvar as it eliminates typing name in one more place. Very handy when you create your own script by changing existing one. > If I could change history, I would have forbidden calling a function > from a command substitution like that in the template. Instead, the > function should be called normally and should set the variable itself. > However, I think that cannot be changed anymore. > > I have a fairly small patch to sh to eliminate the fork when expanding > $(set_rcvar). However, one could object that this does not add true new > capabilities but only a new notation to the part of the language that > executes without forks (because the function could write into a variable > instead of being command-substituted). make the scripts less pretty, though. -- Pawel Jakub Dawidek http://www.wheelsystems.com FreeBSD committer http://www.FreeBSD.org Am I Evil? Yes, I Am! http://yomoli.com |
|
|
|
On Fri, Jan 06, 2012 at 05:05:58PM -0800, Doug Barton wrote:
> On 01/06/2012 06:13, Pawel Jakub Dawidek wrote: > > Any objections? > > > > http://people.freebsd.org/~pjd/patches/set_rcvar.patch > > > > This patch only changes scripts where set_rcvar can be used with no > > arguments. > > Please don't do this. > > Jilles already pointed out the important reason, it adds pointless > forks. I suggested a long time ago that we remove set_rcvar altogether > but I got a lot of resistance to it, and never pursued it. Perhaps it's > time to revisit that. there are much more bad examples than good ones: % grep -r ^rcvar /usr/src/etc/rc.d | grep set_rcvar | wc -l 66 % grep -r ^rcvar /usr/src/etc/rc.d | grep -v set_rcvar | wc -l 34 > In regards to your concern about not having to type the name of the > script when copying to create a new one, you can't really be serious > about that? If so, let me introduce you to this revolutionary thing > called "search and replace." Do you really expect me to comment this? -- Pawel Jakub Dawidek http://www.wheelsystems.com FreeBSD committer http://www.FreeBSD.org Am I Evil? Yes, I Am! http://yomoli.com |
|
|
|
|
On Sat, Jan 07, 2012 at 02:38:23PM -0800, Doug Barton wrote:
> On 01/07/2012 03:25, Pawel Jakub Dawidek wrote: > > On Fri, Jan 06, 2012 at 05:05:58PM -0800, Doug Barton wrote: > >> On 01/06/2012 06:13, Pawel Jakub Dawidek wrote: > >>> Any objections? > >>> > >>> http://people.freebsd.org/~pjd/patches/set_rcvar.patch > >>> > >>> This patch only changes scripts where set_rcvar can be used with no > >>> arguments. > >> > >> Please don't do this. > >> > >> Jilles already pointed out the important reason, it adds pointless > >> forks. I suggested a long time ago that we remove set_rcvar altogether > >> but I got a lot of resistance to it, and never pursued it. Perhaps it's > >> time to revisit that. > > > > It is a total mess now then and it is definiately not intuitive when > > there are much more bad examples than good ones: > > I agree, which is why I previously proposed assigning them all directly > when possible (which is in almost all cases). If no one speaks up > opposing this idea in the next few days I'm still prepared to proceed. -- Pawel Jakub Dawidek http://www.wheelsystems.com FreeBSD committer http://www.FreeBSD.org Am I Evil? Yes, I Am! http://yomoli.com |
|
In reply to this post by dougb
Doug Barton <[hidden email]> wrote
in <[hidden email]>: do> On 01/07/2012 03:25, Pawel Jakub Dawidek wrote: do> > On Fri, Jan 06, 2012 at 05:05:58PM -0800, Doug Barton wrote: do> >> On 01/06/2012 06:13, Pawel Jakub Dawidek wrote: do> >>> Any objections? do> >>> do> >>> http://people.freebsd.org/~pjd/patches/set_rcvar.patch do> >>> do> >>> This patch only changes scripts where set_rcvar can be used with no do> >>> arguments. do> >> do> >> Please don't do this. do> >> do> >> Jilles already pointed out the important reason, it adds pointless do> >> forks. I suggested a long time ago that we remove set_rcvar altogether do> >> but I got a lot of resistance to it, and never pursued it. Perhaps it's do> >> time to revisit that. do> > do> > It is a total mess now then and it is definiately not intuitive when do> > there are much more bad examples than good ones: do> do> I agree, which is why I previously proposed assigning them all directly do> when possible (which is in almost all cases). If no one speaks up do> opposing this idea in the next few days I'm still prepared to proceed. I am always wondering if defining $rcvar as "${name}_enable" at the end of load_rc_config() when $rcvar is undefined is bad idea. Is there any problem with removing rcvar=... in individual rc.d scripts except for non-standard ones (empty or different from ${name}_enable)? It looks simpler than writing the same line "rcvar=${name}_enable" many times in various places. -- Hiroki |
|
|
On 01/07/2012 15:12, Hiroki Sato wrote:
> I am always wondering if defining $rcvar as "${name}_enable" at the > end of load_rc_config() when $rcvar is undefined is bad idea. > > Is there any problem with removing rcvar=... in individual rc.d > scripts except for non-standard ones (empty or different from > ${name}_enable)? It looks simpler than writing the same line > "rcvar=${name}_enable" many times in various places. This sounds like a great idea in theory, but in practice it doesn't work out, for 2 reasons. First, we have a lot of scripts in the base (about 1/3) that rely on the lack of any rcvar meaning that it gets run unconditionally. In order to provide backwards compatibility we'd have to add code to enable things by default that were previously unset. That's not hard to do, but .... The other reason is that for ports, the scripts generally look like this: load_rc_config foo : ${foo_enable:=NO} See the problem? There are ways around both of these issues, but they involve much more dramatic changes that I don't think buy us much, if anything. There are already a large'ish number of things that are repeated in each rc.d script, so leaving this one in doesn't hurt anything. Whereas removing the forks caused by calling set_rcvar actually gains us a shorter boot time.* Attached is a patch that does what I suggested a long time ago, removes set_rcvar() entirely and assigns each rcvar statically. I'll commit this in a few days if no one objects. (Note, it can't be committed yet because the scripts in ports that call set_rcvar() have to be modified first.) Doug * Yes, I know that we're only gaining a second, maybe two. But each time we do something to improve the boot time the cumulative effect is what we're after. -- You can observe a lot just by watching. -- Yogi Berra Breadth of IT experience, and depth of knowledge in the DNS. Yours for the right price. :) http://SupersetSolutions.com/ _______________________________________________ [hidden email] mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-rc To unsubscribe, send any mail to "[hidden email]" |
|
|
|
|
|
In reply to this post by Stefan Esser-3
On Mon, Jan 09, 2012 at 10:40:30AM +0100, Stefan Esser wrote:
> Am 09.01.2012 00:12, schrieb Doug Barton: > > Attached is a patch that does what I suggested a long time ago, removes > > set_rcvar() entirely and assigns each rcvar statically. I'll commit this > > in a few days if no one objects. (Note, it can't be committed yet > > because the scripts in ports that call set_rcvar() have to be modified > > first.) > [...] > > Index: rc.d/nscd > > =================================================================== > > --- rc.d/nscd (revision 229825) > > +++ rc.d/nscd (working copy) > > @@ -19,7 +19,7 @@ > > . /etc/rc.subr > > > > name="nscd" > > -rcvar=`set_rcvar` > > +rcvar="nscd_enable" > > Why not generally use > > rcvar="${name}_enable" > > instead of (e.g.) > > rcvar="nscd_enable" > > in all scripts *in your patch set*, for which the outcome is the same? I can't see how this might be a bad idea, really. -- Pawel Jakub Dawidek http://www.wheelsystems.com FreeBSD committer http://www.FreeBSD.org Am I Evil? Yes, I Am! http://yomoli.com |
|
|
|
|
In reply to this post by dougb
Doug Barton <[hidden email]> wrote
in <[hidden email]>: do> On 01/07/2012 15:12, Hiroki Sato wrote: do> > I am always wondering if defining $rcvar as "${name}_enable" at the do> > end of load_rc_config() when $rcvar is undefined is bad idea. do> > do> > Is there any problem with removing rcvar=... in individual rc.d do> > scripts except for non-standard ones (empty or different from do> > ${name}_enable)? It looks simpler than writing the same line do> > "rcvar=${name}_enable" many times in various places. do> do> This sounds like a great idea in theory, but in practice it doesn't work do> out, for 2 reasons. First, we have a lot of scripts in the base (about do> 1/3) that rely on the lack of any rcvar meaning that it gets run do> unconditionally. In order to provide backwards compatibility we'd have do> to add code to enable things by default that were previously unset. do> That's not hard to do, but .... do> do> The other reason is that for ports, the scripts generally look like this: do> do> load_rc_config foo do> do> : ${foo_enable:=NO} do> do> See the problem? Removing rcvar=`set_rcvar`, and then adding rcvar="" into scripts that need to be run unconditionally would work. However, I have no strong opinion about that. I agree that it needs some more code anyway and keeping things simple is better. Doug Barton <[hidden email]> wrote in <[hidden email]>: do> > The use of "${name}_enable" does not add measurable overhead, but that do> > way more of an existing script might be used as a prototype unchanged. do> do> I understand what you're saying, and I know that the whole "use do> variables wherever we can" thing is all '1337 and computer science'y, do> but it's silly. The concept of a universal template that can be copied do> and pasted for different services is a pipe dream. There are already do> many things that need to be changed in the new script, and not updating do> rcvar for a new script causes clear and obvious failure messages. I prefer to use ${name}_enable because putting the same keyword in two places always leads to a stupid typo issue. -- Hiroki |
|
|
In reply to this post by dougb
On Mon, Jan 09, 2012 at 02:26:07AM -0800, Doug Barton wrote:
> On 01/09/2012 02:21, Pawel Jakub Dawidek wrote: > > On Mon, Jan 09, 2012 at 10:40:30AM +0100, Stefan Esser wrote: > >> Am 09.01.2012 00:12, schrieb Doug Barton: > >>> Attached is a patch that does what I suggested a long time ago, removes > >>> set_rcvar() entirely and assigns each rcvar statically. I'll commit this > >>> in a few days if no one objects. (Note, it can't be committed yet > >>> because the scripts in ports that call set_rcvar() have to be modified > >>> first.) > >> [...] > >>> Index: rc.d/nscd > >>> =================================================================== > >>> --- rc.d/nscd (revision 229825) > >>> +++ rc.d/nscd (working copy) > >>> @@ -19,7 +19,7 @@ > >>> . /etc/rc.subr > >>> > >>> name="nscd" > >>> -rcvar=`set_rcvar` > >>> +rcvar="nscd_enable" > >> > >> Why not generally use > >> > >> rcvar="${name}_enable" > >> > >> instead of (e.g.) > >> > >> rcvar="nscd_enable" > >> > >> in all scripts *in your patch set*, for which the outcome is the same? > > > > I fully agree. There is one less thing to change after reusing a script. > > I can't see how this might be a bad idea, really. > > See my previous response. repeat myself: I can't see how this might be a bad idea. Do you? -- Pawel Jakub Dawidek http://www.wheelsystems.com FreeBSD committer http://www.FreeBSD.org Am I Evil? Yes, I Am! http://yomoli.com |
|
|
|
|
|
|
On Fri, Jan 13, 2012 at 01:57:11AM -0800, Doug Barton wrote:
> On 01/09/2012 05:35, Hiroki Sato wrote: > > I prefer to use ${name}_enable because putting the same keyword in > > two places always leads to a stupid typo issue. > > So, here's the thing. I understand what you're saying, I really do. The > problem is, you're still wrong. :) > > There are MANY places where you have to use the value of $name literally > already, and typos count in every single one of them: > > 1. The name of the script file > 2. PROVIDE > 3. REQUIRE > 4. names of service-specific methods, e.g.: start_precmd="foo_prestart" > 5. Default values in /etc/defaults/rc.conf > 6. rc.conf.5 > > And that's just off the top of my head. The ideal of being able to > copy/paste example rc.d scripts for use with new services without having > to change [m]any code sounds great in theory. In practice, search and > replace is an absolute necessity. So having one more place where you > have to replace the name of the old service with the new one isn't going > to hurt anything. using variables is wrose than hardcoding names everywhere. > I haven't seen any objection to the _substance_ of my proposal, so I'm > going to go ahead with it tomorrow. Please don't. Touching so many rc.d files is PITA during mergemaster. If we want to do that, let's do that once. The current consensus as I see it, is that you are the only one wanting to hardcode names and there are many in favour of using variables. -- Pawel Jakub Dawidek http://www.wheelsystems.com FreeBSD committer http://www.FreeBSD.org Am I Evil? Yes, I Am! http://yomoli.com |
|
Loading...
| Powered by Nabble | Edit this page |