[patch] rc.d cleanup
Hi there,
This is my first patch to this project.
This is the first of many patches to come actually, but I need to find a
sponsor to guide me, and review what I submit. The patch is kinda big, and
far reaching in terms of altering almost every rc.d script.
This patch effects most of the rc.d scripts that utilize simple IF
statements, converting them to logical AND/OR's instead. For example:
if [ ! -f foo ]
then
bar
fi
Would simply become:
[ -f foo ] || bar
The exception (but not the rule) is for any situation where ELIF/ELSE is
required. In other words any exclusive conditional situations.
I also applied this notion to many simple blocks of code wrapped around
non-exclusive IF statements, such as:
[ -f foo ] && {
command-list
[...]
}
This has the result of reducing the size of the shell code, and reducing
the problem of over-engineering that plagues many scripts. I found quite
many places where there were one-line situations wrapped up in multi-line
IF statements, which I was compelled to eliminate. Further more, as I
audited the scripts, I noticed that in several places this style of
scripting was already used in various places. So I feel this make the
entire span of scripts uniform.
-Jon Disnard
___
freebsd-hackers@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-hackers
To unsubscribe, send any mail to "[EMAIL PROTECTED]"
Re: [patch] rc.d cleanup
> Giorgos Keramidas wrote:
>> On 2005-08-01 13:55, John Baldwin <[EMAIL PROTECTED]> wrote:
>> >On Monday 01 August 2005 01:29 pm, [EMAIL PROTECTED] wrote:
>> >> This patch effects most of the rc.d scripts that utilize simple IF
>> >> statements, converting them to logical AND/OR's instead. For example:
>> >>
>> >> if [ ! -f foo ]
>> >> then
>> >> bar
>> >> fi
>> >>
>> >> Would simply become:
>> >>
>> >> [ -f foo ] || bar
>> >>
>> >> The exception (but not the rule) is for any situation where ELIF/ELSE
>> is
>> >> required. In other words any exclusive conditional situations.
>> >>
>> >> I also applied this notion to many simple blocks of code wrapped
>> around
>> >> non-exclusive IF statements, such as:
>> >>
>> >> [ -f foo ] && {
>> >>command-list
>> >>[...]
>> >> }
>> >
>> > The argument I would have against this is that it is a lot easier to
>> > read the 'if foo; then ; fi' style, esp. for folks used to using C,
>> > etc. Shell scripts don't need to be overly obfuscated.
>>
>> Ditto. The if/then blocks may look superficial at first and entirely
>> redundant, but they really work much better then code like this:
>>
>> [ -f foo ] || bar
>>
>> has to be extended to form a multiline statement. Now, I know that one
>> can always write:
>>
>> [ -f foo ] || {
>> [ -d bar ] && {
>> blah
>> }
>> }
>>
>> But this quickly gets too ugly for my taste. The equivalent if/then
>> blocks:
>>
>> if [ ! -f foo ]; then
>> if [ -d bar ]; then
>> blah
>> fi
>> fi
>>
>> or even, the similar:
>>
>> if [ ! -f foo ] && [ -d bar ]; then
>> blah
>> fi
>>
>> Look much much prettier to the eyes of one who knows how to read both
>> shell scripts and C code.
>
> Thirded. I far prefer the bigger C-like if statements and think this
> patch is a huge code churn for what is basically code obfuscation.
>
> Cheers,
> Maxime
Well I certainly respect the opinions, but respectfully when has the use
of && and || become obfuscation? Secondly, the use of shell style blocks
of code is similar to the way they are done in C where curly-braces are
used to enclose blocks of code. I honestly don't believe that it is
because of people who look at C and shell code, it is probably more due to
the foundation of all that existing shell code we read that does use IF
statements instead of logical AND/OR. What is the point of using a bulky
IF statement for the evaluating simple true/false situation that the shell
supports with && or ||? I would characterize this patch as a
simplification, or rounding down to the lowest common denominator. It
doesn't touch the parts that require the bulky ELIF/ELSE's, just the
simple conditions. However, I would be willing to make the patch less
"code-churning" to only affect the non-blocks of code, aka one-liners
like: [-f foo ] && bar. I don't have the intention to obfusecate, just
good shell code. =)
-Jon
___
freebsd-hackers@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-hackers
To unsubscribe, send any mail to "[EMAIL PROTECTED]"
Re: [patch] rc.d cleanup
> [EMAIL PROTECTED] wrote this message on Mon, Aug 01, 2005 at 12:29
> -0500:
>> This has the result of reducing the size of the shell code, and reducing
>
> Unless you cross a fs frag (usually 1024 bytes), i.e. reduce the scripts
> by an average of 512bytes *per* script, you will see no disk space savings
> from these changes... considering that on my 5.4-R system over half
> (73 of 124) are under a frag in size, it isn't that much...
>
> One thing that would be really useful for rc.d system is a way to cache
> the ordering on boot... On a slow system like a 200mhz arm, the rc
> ordering can take a long while
>
> --
> John-Mark GurneyVoice: +1 415 225 5579
>
> "All that I will do, has been done, All that I have, has not."
>
Ok thanks for the dirrection. Ask and thy shall receive. well a quick hack
at least to implement cache of rcorder, although from my tests even on old
hardware rcorder is quick. I wrote this in (cring) the long-spelled out
way. Anyways... don't forget to add the essential rc.conf knobs for the
two variables shown bellow.
[root@:/etc]# diff -u rc rc.new
--- rc Sun Jul 31 17:45:19 2005
+++ rc.new Mon Aug 1 21:42:12 2005
@@ -72,7 +72,17 @@
skip="-s nostart"
[ `/sbin/sysctl -n security.jail.jailed` -eq 1 ] && skip="$skip -s nojail"
-files=`rcorder ${skip} /etc/rc.d/* 2>/dev/null`
+# check for a rc.conf knob to check for existing cache, and skip the
rcorder part
+if checkyesno ${rc_cache_enable} ; then
+ if [ -f ${rc_cache_file} ] ; then
+ files=`cat ${rc_cache_file}`
+ else
+ files=`rcorder ${skip} /etc/rc.d/* 2>/dev/null`
+ echo "${files}" > ${rc_cache_file}
+ fi
+else
+ files=`rcorder ${skip} /etc/rc.d/* 2>/dev/null`
+fi
for _rc_elem in ${files}; do
run_rc_script ${_rc_elem} ${_boot}
___
freebsd-hackers@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-hackers
To unsubscribe, send any mail to "[EMAIL PROTECTED]"
Re: [patch] rc.d cleanup
> > > [EMAIL PROTECTED] wrote: > >>>Giorgos Keramidas wrote: >>> >>> > [...] > >>>Thirded. I far prefer the bigger C-like if statements and think this >>>patch is a huge code churn for what is basically code obfuscation. >>> >>>Cheers, >>>Maxime >>> >>> >> >> >>Well I certainly respect the opinions, but respectfully when has the use >>of && and || become obfuscation? >> >> > > I work on alot of shell code and I prefer the "spelled out the long way" > approach. > > i.e. I prefer > > if [blah] > then >if [blah2] >then > etc > > it's just easier to get it right in my opinion. > remember, when the rare 100 sets of fingers in teh picture then > you have to code toteh "bloody obvios" standard because sometimes people > have to change scripts to handle some external change, and thay may or > may not] > be shell syntax experts. > > > there si a lot that needs cleanup.. Like what? I'm receptive to any suggestions for area's of shell code that need attention. > I thonk this is nto one of them.. > I do apreciate however the thought and time you have spent. Thanks, but actually I was just bored durring a long portupgrade. Next time I won't bother with such "code-chuning" muse of boredom, but like I said, I'm eager to participate somehow, so perhaps next time I will send up much smaller more digestable enhancements. =) > > > julian > >>-Jon >> >>___ >>freebsd-hackers@freebsd.org mailing list >>http://lists.freebsd.org/mailman/listinfo/freebsd-hackers >>To unsubscribe, send any mail to >> "[EMAIL PROTECTED]" >> >> > ___ freebsd-hackers@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-hackers To unsubscribe, send any mail to "[EMAIL PROTECTED]"
rcorder cache (was [patch] rc.d cleanup)
> On Mon, Aug 01, 2005 at 09:47:54PM -0500, [EMAIL PROTECTED] wrote:
>> > [EMAIL PROTECTED] wrote this message on Mon, Aug 01, 2005 at 12:29
>> > -0500:
>> >> This has the result of reducing the size of the shell code, and
>> reducing
>> >
>> > Unless you cross a fs frag (usually 1024 bytes), i.e. reduce the
>> scripts
>> > by an average of 512bytes *per* script, you will see no disk space
>> savings
>> > from these changes... considering that on my 5.4-R system over half
>> > (73 of 124) are under a frag in size, it isn't that much...
>> >
>> > One thing that would be really useful for rc.d system is a way to
>> cache
>> > the ordering on boot... On a slow system like a 200mhz arm, the rc
>> > ordering can take a long while
>> >
>> > --
>> > John-Mark Gurney Voice: +1 415 225 5579
>> >
>> > "All that I will do, has been done, All that I have, has not."
>> >
>>
>>
>> Ok thanks for the dirrection. Ask and thy shall receive. well a quick
>> hack
>> at least to implement cache of rcorder, although from my tests even on
>> old
>> hardware rcorder is quick. I wrote this in (cring) the long-spelled out
>> way. Anyways... don't forget to add the essential rc.conf knobs for the
>> two variables shown bellow.
>>
>> [root@:/etc]# diff -u rc rc.new
>> --- rc Sun Jul 31 17:45:19 2005
>> +++ rc.new Mon Aug 1 21:42:12 2005
>> @@ -72,7 +72,17 @@
>>
>> skip="-s nostart"
>> [ `/sbin/sysctl -n security.jail.jailed` -eq 1 ] && skip="$skip -s
>> nojail"
>> -files=`rcorder ${skip} /etc/rc.d/* 2>/dev/null`
>> +# check for a rc.conf knob to check for existing cache, and skip the
>> rcorder part
>> +if checkyesno ${rc_cache_enable} ; then
>> + if [ -f ${rc_cache_file} ] ; then
>> + files=`cat ${rc_cache_file}`
>> + else
>> + files=`rcorder ${skip} /etc/rc.d/* 2>/dev/null`
>> + echo "${files}" > ${rc_cache_file}
>> + fi
>> +else
>> + files=`rcorder ${skip} /etc/rc.d/* 2>/dev/null`
>> +fi
>>
>> for _rc_elem in ${files}; do
>> run_rc_script ${_rc_elem} ${_boot}
>
> I'm pretty sure you don't have access to the contents of /etc/rc.conf in
> this context. You would have to use another mechanism to control this
> feature. I'd be tempted to use the existance of a cache file (probably
> /etc/rcorder.cache since it can't be outside /) and simply require users
> to regenerate it externally when they upgrade (I'd probably add code to
> mergemaster to handle it like the various database files). Note that we
> may well break the ability to do this is easily in 7.0 if we end up
> supporting ordering of local scripts.
Would it be possible or in anyway not unreasonable to source rc.conf in
this file? Honestly I was working under the presumption that anytime
rc.subr was sourced that the knobs file was too, but possibly my bad. Not
that it matters, I can think of ways to work arround this, but I would
want to have a rc.d script later on to control the presence of the rcorder
cache file for the next boot, or something. I'd hate to use the existence
of files to control the functionality, where rc.conf knobs are ideal.
-Jon
>
> -- Brooks
>
> --
> Any statement of the form "X is the one, true Y" is FALSE.
> PGP fingerprint 655D 519C 26A7 82E7 2529 9BF0 5D8E 8BE9 F238 1AD4
>
___
freebsd-hackers@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-hackers
To unsubscribe, send any mail to "[EMAIL PROTECTED]"
[patch] rc.d/tmp (silly mkdir usage)
Howdy hackers, I'm sorry for the previous patch, so here is at least one item that really bugs me that isn't obfuscation. In short, I don't see any reason to fork some process to simply "touch" a file (is a filesystem writable) when built-in shell i/o does this: --- /etc/rc.d/tmp.orig Mon Aug 1 23:20:24 2005 +++ /etc/rc.d/tmp Mon Aug 1 23:22:07 2005 @@ -48,8 +48,8 @@ [Nn][Oo]) ;; *) - if (/bin/mkdir -p /tmp/.diskless 2> /dev/null); then - rmdir /tmp/.diskless + if ( > /tmp/.diskless 2> /dev/null); then + rm /tmp/.diskless else if [ -h /tmp ]; then echo "*** /tmp is a symlink to a non-writable area!" I grep'ed the entire rc.d dir, and found that the same technique is used elsewhere in accounting, and cleanvar. So I feel justified this time, although please review, and thanks for the look. While I understand the need to want a fork program to touch, or otherwise create an inode, I feel forking for such an effort is weird and a bit over-engineered. -Jon ___ freebsd-hackers@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-hackers To unsubscribe, send any mail to "[EMAIL PROTECTED]"
Re: [patch] rc.d/tmp (silly mkdir usage)
All the while I point to code example of this exact same usage being deployed in the system already, and in the same exact situation. I see no reason why you must bikeshed on this. Correctness is always correct, despite established bad'ism, and in this case I am carefull to use an already approved shell code commit even if that is bad, it is approved bad or at least commited previously. -Jon > On Mon, 1 Aug 2005 23:37:05 -0500 (CDT) > [EMAIL PROTECTED] wrote: >> I grep'ed the entire rc.d dir, and found that the same technique is used >> elsewhere in accounting, and cleanvar. So I feel justified this time, >> although please review, and thanks for the look. While I understand the >> need to want a fork program to touch, or otherwise create an inode, I >> feel >> forking for such an effort is weird and a bit over-engineered. > > Well, while one prefers a system to boot as quickly as reasonably > possible, > it's not like startup is particularly performance-critical. This is > another > place where I feel that clarity more than compensates for (very minor) > slowness, > particularly when coupled with the fact that the cost of a fork()/exec() > is > overwhelmed by the cost of cranking up some of the heavy-weight daemons. > > It seems to me that you're looking at things from a very "hacky" > perspective. > That is, it seems that you know a lot of shortcuts that add a bit of speed > here and there and that's what you're trying to apply. (Please correct me > if I'm wrong.) I would suggest that you look at the startup scripts > somewhat > differently, by asking yourself what is _broken_ there. I've seen at > least a > couple of suggestions along this line in reply to you. The mkdir usage > isn't > really broken, it seems to me. While I'm by no stretch of the imagination > an > expert regarding the rc scripts (they work and I ignore them), there are > others > around here that are, and they can, I am certain, give you some relevant, > useful and very challenging suggestions. > -- > Frank Mayhar [EMAIL PROTECTED]http://www.exit.com/ > Exit Consulting http://www.gpsclock.com/ > http://www.exit.com/blog/frank/ > ___ freebsd-hackers@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-hackers To unsubscribe, send any mail to "[EMAIL PROTECTED]"
Re: [patch] rc.d/tmp (silly mkdir usage)
> On Mon, Aug 01, 2005 at 11:37:05PM -0500, [EMAIL PROTECTED] wrote: >> Howdy hackers, >> >> I'm sorry for the previous patch, so here is at least one item that >> really >> bugs me that isn't obfuscation. In short, I don't see any reason to fork >> some process to simply "touch" a file (is a filesystem writable) when >> built-in shell i/o does this: >> >> --- /etc/rc.d/tmp.orig Mon Aug 1 23:20:24 2005 >> +++ /etc/rc.d/tmp Mon Aug 1 23:22:07 2005 >> @@ -48,8 +48,8 @@ >> [Nn][Oo]) >> ;; >> *) >> - if (/bin/mkdir -p /tmp/.diskless 2> /dev/null); then >> - rmdir /tmp/.diskless >> + if ( > /tmp/.diskless 2> /dev/null); then >> + rm /tmp/.diskless >> else >> if [ -h /tmp ]; then >> echo "*** /tmp is a symlink to a non-writable >> area!" >> > > The thing you suggest is bloody insecure. Just imagine some baduser > doing ln -s /etc/passwd /tmp/.diskless before rc.d/tmp gets executed. > I guess this is the reason why directory creation is used instead of > file creation. Well these notions have nothing todo with the way it works, but they are interesting still. I would imagine a dir could be linked too if somebody managed to insert a rc.d script in that was ordered sufficiently early enough to do the evil tasks you are thinking about. Even if mktemp(1) were available at this stage, I wouldn't use it here. > > I just wonder why a new shell is forked for this test. Simply > if /bin/mkdir -p /tmp/.diskless 2> /dev/null ; then > would do the same thing without forking a new shell that only executes > /bin/mkdir Let me be clear about this, the ONLY reason mkdir is used here is because touch is under /usr somewhere which isn't even mounted at this point (assuming /usr is mounted seperatly, as is the case on nfs diskless systems). So we are left with what is availabile in /bin, /sbin, /rescue. Therefore mkdir was used as a work-around. What I'm saying is this entire thought process is overly-engineered when the shell can simply "touch" a file with stdout or stderr. This is indeed the most minor of optimizations. > > Even we can use > if [ -d /tmp -a -w /tmp ] ; then > or (which is equivalent) > if [ -d /tmp ] && [ -w /tmp ] ; then > and save external commands (mkdir) execution and directory > creation/deletion at all. > ___ freebsd-hackers@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-hackers To unsubscribe, send any mail to "[EMAIL PROTECTED]"
