[arch-projects] [initscripts] review of remaining patches from kujub

[Tom Gundersen]

Hi Kurt,

I'm in the process of reviewing your remaining patches. Most of it
looks good, but I have a few hesitations.

I think it would be best if you rebased your remaining patches along
these lines ("git rebase -i origin/master"):

All the general whitespace cleanup can be squashed into one patch
(maybe it already is). A quick check to see that nothnig
non-whitespace-related snuck into something that should be
"indentation" or "whitespace" is:  "git show <patch-in-question> -w".

Also, please squash together patches that do the same thing (e.g., the
conversion of "if"'s to &&), as well as patches that fix eachother
(there is at least one patch that fixes a problem introduced earlier
in the patch series (I believe to do with fsckret) , these should be
squashed (technically, you should move the latter patch directly after
the former and mark it as a "fixup" when rebasing).

The need for eval et al: all this stuff should be last in the patch
series as it is the most non-trivial, and it needs the most
explanation.

I please correct me if I got the wrong end of the stick here, but this
is the way I see this status business:

if eval/write_file/install_file is NEEDED in order to make status()
work correctly, it means there is a bug in status(). The semantics of
status must be that it does whatever it wants with the string passed
to it, and the other arguments are executed as if status and the
string was not there at all. If I understand correctly, our own
status() does this, but a possible replacement does not. Is that
correct? If so, let's drop any workarounds, and stick with the
redirections.

Some general comments:

1) You do some cleanup of hooks relative to status. The current code
is not consistent, but I have now had a look at this, and this is how
I think it should be done:

run_hook pre_foo
if [[$WE_WANT_TO_DO_FOO]]]; then
  stat_busy "Doing foo"
    if [[$PRECONDITIONS_FOR_FOO_NOT_SATISFIED]]; then
      stat_fail
    else
      ...
      stat_done
    fi
fi
run hook post_foo

Generally (though there are exceptions), I think the hooks and the
status message should be in rc.{sysinit,shutdown}, and the functions
implementing it should be in functions. E.g.,

rc.sysinit
-----------
run_hook pre_foo
[[$WE_WANT_TO_DO_FOO]] && status "Doing foo" foo
run hook post_foo

functions
------------
foo() {
  [[$PRECONDITIONS_FOR_FOO_NOT_SATISFIED]] && return 0
  ...
}

Examples where I belive we are currently doing the wrong thing:

fsck_all, mount_all

Examples of where this does not apply:

kill_everything, udev_modprobe


I think some of your fixups followed this scheme, but some went in the
other direction. Do you think what I suggest makes sense?

Lastly: thanks for your patches, they are much appreciated!

Cheers,

Tom