Tom Gundersen, 2011-06-27 02:33:

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.

Will do. (Could take some time.)

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.

Currently status just executes "$command" "$arg"... as given. A redirection can't be passed this way. If we would change it to execute eval "shell-string"... we would introduce possible problems because of unexpected behavior because eval does word splitting. In that case we would need to make sure to always call it with extra quoting like so: status "$msg" "echo '$something' > '$file'" which wouldn't make life easier IMHO.

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

With my patches i can find only two violations of this: * sysinit_{pre,post}fsck within the stat_{busy,done} block This allows to save the message string, redirect fsck progress into a splash status line restoring the original message whenever fsck stops to output progress status (currently supported for ext[234] only AFAIK). This is done in fbsplash-extras (AUR). * sysinit_{pre,post}mount within the stat_{busy,done} block Don't know why, but my feeling is something like for the fsck thing. (Maybe a fsck done by mount (as with ntfs-3g) will output progress status in the future.) To get some more coding style consistency we could do something like: stat_busy_hook() { stat_busy "$2" run_hook "$1" } stat_done_hook() { stat_done run_hook "$1" } stat_fail_hook() { stat_fail run_hook "$1" } run_hook pre_foo_wanted # <- only when unconditional hook needed if [[ $WE_WANT_TO_DO_FOO ]]; then stat_busy_hook pre_foo "Doing foo" if [[$PRECONDITIONS_FOR_FOO_NOT_SATISFIED]]; then stat_fail_hook post_foo else ... stat_done_hook post_foo fi fi (Leaving stat_{busy,done,fail} untouched to avoid breakage because of the custom stat_{busy,done,fail} overrides out there.)

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?

See above.

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

Cheers,

Tom Nice to hear this.

-- Kurt