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

[Kurt J. Bosch]

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