Re: [arch-projects] [initscripts] [PATCH 00/13] A new rag rug roll (rebased)

Tom Gundersen, 2011-07-09 13:24:

Hi Kurt,

I'm just reviewing this. Before I get started, could I ask you to add a few more details to your commit messages? Many things that look obvious to us now might not be obvious to future contributors, so I would prefer too many rather than to few comments.

What I have in mind:

On Sun, Jul 3, 2011 at 10:31 PM, Kurt J. Bosch wrote:

...

Kurt J. Bosch (13): functions/rc.single: Clean up whitespace

That's ok.

...

rc.sysinit: Fix stat_busy block indentation

A sentence explaining the "policy" would be nice: "Align stat_{fail,done} with stat_busy".

...

functions: Get rid of superfluous braces in udevd_modprobe()

Why are they superfluous? This has been pointed out on the list, and it is pretty basic, but I prefer if these kind of things are pointed out explicitly.

...

functions: Add missing quotes in mount_all()

Why are they needed? It is nice to know the difference between "this is the right way to do it, even if it does not fix a bug" and a bug fix.

...

functions/rc.multi: Strip paths from binaries rc.sysinit/functions: Refactor kill_everything/fsck_all/mount_all

Those are good.

...

rc.sysinit: Declare $FORCEFSCK read-only to prevent custom hooks from messing

Fine I guess, but it would be better if you split the summary and justification in two lines:

Declare $FORCEFSCK read-only

This prevents<name of hooks> from overwriting the value.

...

functions/rc.sysinit: Refactor fsck-redirection to prevent breakage

Good comments. Question: Is it necessary to declare the empty variables?

...

functions: Sanitize status() behaviour to make it more intuitive

Good comments. I agree with returning the error code. Not sure about the redirection. If you have a usecase (not the one below), then maybe.

...

rc.sysinit: Simplify setting hostname by using status() even with redirection now

I don't think this works. Before this patch the status encapsulates the writing to /proc, after this patch it only encapsulates the echo command, so in case we cannot write to /proc the status message does not behave as intended.

...

functions: Deprecate in_array(), Add replacement is_in_array()

I agree with changing to use ck_daemon, probably about the deprecation too, not sure about adding replacement function without a user though.

...

functions: Add stop_all_daemons() and related *_prestopdaemons hook functions: Speed up reboot/shutdown by recognizing killall5 exit code 2

Good comments, I'll have to look into the killall5 stuff before deciding though.

Cheers,

Tom

Rebased again as suggested. Please have a look at the new thread as this one is a bit chaotic meanwhile. -- Kurt