Date: Wed, 19 Aug 2026 08:47:51 -0700
From: Jason Thorpe <[email protected]>
Message-ID: <[email protected]>
| https://www.netbsd.org/~thorpej/rcorder-cache-diff-v3.txt
Just a couple of minor comments about this version, and only (at least
before I start on it in detail) one actual change suggested.
+ if [ $_rc_update_rcorder_cache = YES ]; then
That expansion isn't quoted -- but that one is (should be) OK, as the
var is always set to either YES or NO, so unless IFS happened to acquire
one (or more) of 'Y' 'E' 'S' 'N' or 'O' in its value (very unlikely as
IFS can't be imported, it has to be set by the script if it is to be
changed) that one is safe enough.
However, as mentioned again below, quoting expansions, even when
not required, is generally the better thing to do.
if ! checkyesno ${rcvar}; then
That one is clearly there already, not part of your changes, but
that should be quoted, rcvar comes from the script, and could be
set to anything (to work it must be a 'name' (as defined by sh), but
if some package were not interested in it working, anything is possible).
+ skipstart)
+ if [ -n "$rcvar" ] && ! checkyesno ${rcvar}; then
And then the same there. For the first reference, the (already there)
quoting is essential, in case rcvar is not set, & while the test ensures
it has a value before the 2nd reference, it still might be anything.
+ command echo YES
There I won't complain about the use of echo (though printf always
is generally better, just for the habit using it produces) as any
version of echo which doesn't properly output a simple alphabetic word
would be too broken to exist. (Same for the NO case just after).
&& _has_rcorder_keyword interactive $_file
Another expansion that is there already (ie: not your change),
but should be quoted.
+ if _rc_order_use_cache && [ -f /etc/rcorder.cache ]; then
+ # Listed here in order of most-likely-to-change.
+ allf="/etc/rc.conf /etc/rc.conf.d /etc/rc.conf.d/"*
+ for d in ${rc_directories:-/etc/rc.d}; do
+ allf="$allf ${d}/"*
Those two var assignments to allf work just fine as written
but they would work just the same way, and look a little less
peculiar, if written:
+ allf="/etc/rc.conf /etc/rc.conf.d /etc/rc.conf.d/*"
and
+ allf="$allf ${d}/*"
The quotes in these (or something similar) are needed to avoid the
arrignment word ending at the white space, but that's all they achieve here.
No field splitting or filename expansions happen in assignments, so
the quotes are not protecting against that, nor does leaving the '*'
unquoted cause filename expansion to happen. The two forms are
entirely equivalent (hence no change required).
That last one could even be written
allf=$allf\ ${d}/*
with exactly the same effect, the only char in the line affected
by the quoting is the space (the " could not be changed to ' though
as then the $allf and $d would not be expanded, those need to be
either unquoted, or "" quoted to work). On the other hand, the first
of these 2, which has no expansions, just words, could use '' quoting
(which is marginally more efficient inside sh).
[Aside: writing $allf (no braces) and ${d} (with braces) in the
same command like that, when there is no reason to include the
braces in either case (there is never a reason, except perhaps
line length, to exclude them) is a kind of confusing style.
]
The quotes are stripped (as always) as just about the last operation
before a command is run (not quite last, as redirections, etc, happen
even later, but that's not relevant here) so what is assigned to allf
by the second of those is just the value of allf followed by a space,
then the value of d with /* appended to it. No quotes.
+ for f in ${allf}; do
That's where the * gets expanded, ${allf} isn't quoted there, and
so undergoes field splitting, then each resulting field that contains
a shell meta character ('*' '?' or '[') gets subjected to filename expansion.
This is when the $d/* from above gets turned into a list of filenames.
Whether the '*' was quoted, or not, when it was assigned to allf, makes
no difference to anything, it is unquoted here, so gets expanded (if
there are any matches). The only way to prevent that would be if the
character before the '*' in the value of allf was a '\' (which we know
it isn't, as it is, when assigned, a '/' instead).
So, no change needed there, it all works as intended, just looks a bit odd,
perhaps gives a misleading impression of what is happening.
+ allf=$(for d in ${rc_directories:-/etc/rc.d}; do
+ test -d "$d" && command echo "${d}"/*;
+ done)
In contract to the above, to work as planned, that * does need to
be unquoted (the '/' does not, but that makes no difference to anything),
that is a word which is an arg for a command, and is subjected to
everything, no field splitting will happen, as the only expansion
is properly quoted (this is the "${d}"/* word I am discussing)
but the resulting (single) field contains a meta char (unquoted),
so filename expansion happens, so the result of that loop is a list
of filenames - in this case separated by newlines.
And even though that works just as planned, this is my one actual
suggestion for a change, in two ways, for different reasons.
First, as the script doesn't control ${rc_directories} it doesn't
control the values of $d so this is a place where printf should be
used rather than echo.
so, we could replace the middle line of that with
+ test -d "$d" && command printf '%s ' "${d}"/*;
[Aside: the ';' is not useful, and could be deleted, this is not C,
expressions don't need to end in a ';' to turn them into commands,
a newline works just as well as the terminator - but that's harmless.
The ';' would be needed if the "done" were moved up to the same line -
just the same as the ';' in the "for d ..." line, if the "do" were on the
next line, that ';' wouldn't be needed - and none is needed, or
even allowed, immediately after the "do" - that would make an empty
command, and sh syntax in general does not allow those.
]
One change there, is that now the list of filenames is separated
by spaces, rather than newlines. That makes no difference to
anything, as both space and newline are in the normal value of IFS,
so both work the same way when we get to do field splitting in
the following line.
+ orderedf=$(rcorder -s nostart ${rc_rcorder_flags} ${allf})
This is one case where quotes are not wanted anywhere. They're
not wanted for ${rc_rcorder_flags} for 2 reasons - first if there
are no flags, that arg needs to vanish completely, whereas quoting
it would leave a "" string instead, not what is desired; and second
to allow ${rc_rcorder_flags} to expand to multiple words if needed.
And for ${allf} we have a list of filenames we need to pass to
rcorder to consider, that list needs to be field split. As
currently written in -v3 we really don't want filename expansion
any more though, as that has already happened. If one of the rc.d
files happened to be named 'foo*' we want to rcorder 'foo*' not all
the files that have names starting with 'foo'.
There are two ways to prevent that, one would be to make the cmdsub
for the assignment to orderdf be
+ orderedf=$(set -f; rcorder -s nostart ${rc_rcorder_flags} ${allf})
which in some ways would be safest, as it would also prevent filename
expansion of the results of field splitting ${rc_rcorder_flags} - but
as that is a very low risk, I'd suggest leaving filename expansion
enabled there (+f - the default) and instead skipping the filename
expansion in the construction of the value of allf, by changing the
line above to:
+ test -d "$d" && command printf '%s ' "${d}/*"
so that allf ends up being just "rc.d/* " in the usual case
(rc_directories not set). That is then field split with the
expansion of ${allf} (unquoted) in the command substitution,
(which in this case simply deletes the space, and leaves the
single field rc.d/* which then gets filename expanded (just
once, rather than twice, as the current -v3 version is doing).
Which of those 2 solutions gets adopted makes not a lot of difference,
but either the "set -f" version, or the quoted '*' in the earlier
assignment needs to happen for general safety. I'd do the quoted
'*' version, as the risk from filename expansion of ${rc_rcorder_flags}
is minimal, and it causes the size of what is assigned to allf to be
much smaller (and that happens at in a higher level shell, the command
substitution still needs to expand it, but once done, that forked
shell simply vanishes, with its much longer string allocation along
with it, and the way sh works, no malloc() ever happens just for it,
saving just a tiny bit more).
+ if ! _rc_order_use_cache; then
+ _rc_ordered_script_list="$orderedf"
Another place where the quotes are harmless, and meaningless.
+ if [ x"$canskip" != xYES ]; then
The 'x' chars there are an ancient workaround, and no longer
serve any practical purpose, as long as one sticks to the defined
usages of test (which this is). Once upon a time, those served
a purpose, now they are just annoying to read (still works the same
of course - but with longer strings to compare!)
+ if (command printf "" > /etc/rcorder.cache.tmp) 2>/dev/null; then
Here you don't need the 'command printf ""' at all, this is just
testing whether the file can be opened for writing, and creating it,
though that isn't essential, and for that
+ if (> /etc/rcorder.cache.tmp) 2>/dev/null; then
would work just fine. Unfortunately I think the () (subshell) is
needed, as redirection errors in scripts tend to cause the shell to
exit ... the subshell exiting is harmless, it had no more work to
do anyway, but the shell running the script can't be allowed to
just exit there.
+ command printf "${f}\n" >> /etc/rcorder.cache.tmp
Oh, I didn't see this one earlier, this is another that should be
fixed, (so I am actyally suggesting 2 changes that should be made, not
just the one (above) I thought I'd be making).
Used that way, printf is no better than echo would be.
Make that one:
+ command printf '%s\n' "${f}" >> /etc/rcorder.cache.tmp
so that "${f}" is just a string, no embedded formatting conversions applied.
(this is the same as in C, "printf(fmt)" with a 'char *fmt' string that comes
from who knows where). The \n needs to be in the format in this case
to generate a newline, rather than the 2 chars '\' 'n' which it would be
if written as:
+ command printf %s "${f}\n" >> /etc/rcorder.cache.tmp
Though:
+ command printf %s "${f}"$'\n' >> /etc/rcorder.cache.tmp
would work if, for some obscure reason I can't imagine, that was
considered better (puts a newline character at the end of the string
handed to printf %s (which then is not adding one).
Then in the new rd.d script:
+name="rcorder_cache"
(quoting 100% useless there)
+rcvar=$name
[....]
+rcorder_cache_status()
+{
+ if checkyesno ${rcvar}; then
In that one, as this scipt has total control over the value of
rcvar, it is acceptable to not quote it. But it would be better
if it were, again, more for getting into the habit of always quoting
expansions, unless able to justify why quoting one wouldn't work.
+ if [ -f /etc/rcorder.cache ]; then
+ echo "/etc/rcorder.cache is in use."
For that, while (rc.subr's) echo() is fine, '' rather than "" would
be better (minimally better, but better) - less work is needed to
be done by sh with '' quoted strings than for "" ones (no looking
for \ or $ inside for example). This is a case where the quoting
is not really needed at all, echo (and its printf based implementation)
handle multiple args just fine - but quoting it is slightly better, just
one arg to deal withm rather than 4 (in that case), and for the quoting
'' is better than "" when no embedded expansions are needed.
This is another case where sh is not C, in C '' makes ints, "" makes
arrays of char, in sh, everything (except operators) is a string
(internally an array of char) whether quoted or not, and if quoted
what kind of quoting was used merely affects what expansions can happen
inside the string.
There is no need to change this (or the other two in this function)
they will work as written with no issues, but as the aim of all of
this set of changes is to be as fast as possible, using '' over ""
might save a few microseconds on a slow processor (probably < 1 of
those on a fast one).
+load_rc_config $name
One more example where (becase the script owns the value of name)
quoting the expansion is not essential, but would be better, just
on general principle.
kre
